| Message ID | 20170705134934.1703-1-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 07/05/2017 03:49 PM, Johannes Thumshirn wrote: > SG_DXFER_FROM_DEV transfers do not have a dxferp as we set it to NULL, > but must have a length bigger than 0. This fixes a regression introduced > by commit 28676d869bbb ("scsi: sg: check for valid direction before > starting the request") > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > Fixes: 28676d869bbb ("scsi: sg: check for valid direction before starting the request") > Reported-by: Chris Clayton <chris2553@googlemail.com> > Tested-by: Chris Clayton <chris2553@googlemail.com> > Cc: Doug Gilbert <dgilbert@interlog.com> > --- > drivers/scsi/sg.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index 21225d62b0c1..3c91593260aa 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp) > if (hp->dxferp || hp->dxfer_len > 0) > return false; > return true; > - case SG_DXFER_TO_DEV: > case SG_DXFER_FROM_DEV: > + if (hp->dxferp || hp->dxfer_len < 0) > + return false; > + return true; > + case SG_DXFER_TO_DEV: > case SG_DXFER_TO_FROM_DEV: > if (!hp->dxferp || hp->dxfer_len == 0) > return false; > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On 2017-07-05 09:49 AM, Johannes Thumshirn wrote: > SG_DXFER_FROM_DEV transfers do not have a dxferp as we set it to NULL, > but must have a length bigger than 0. This fixes a regression introduced > by commit 28676d869bbb ("scsi: sg: check for valid direction before > starting the request") It is not clear to me that dxferp is set to NULL for the newer sg_v3 interface. In the sg.c source of lk 4.12.0 around line 654 (in the sg_write(...) function) only the older interface passes through; the newer interface bypasses that section with a 'return sg_new_write(...)' on line 606. Can you check your patch with one of the utilities from sg3_utils such as sg_inq which will use SG_DXFER_FROM_DEV with the newer interface? BTW I'm not sure why dxferp is set to NULL for SG_DXFER_FROM_DEV transfers; perhaps some magic done by the block layer. Maybe a comment in the code (e.g. on line 654) would help. Also sg_is_valid_dxfer() is only called once and is more complex than it looks; so perhaps it could be inlined back in sg_common_write(). Doug Gilbert > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > Fixes: 28676d869bbb ("scsi: sg: check for valid direction before starting the request") > Reported-by: Chris Clayton <chris2553@googlemail.com> > Tested-by: Chris Clayton <chris2553@googlemail.com> > Cc: Doug Gilbert <dgilbert@interlog.com> > --- > drivers/scsi/sg.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index 21225d62b0c1..3c91593260aa 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp) > if (hp->dxferp || hp->dxfer_len > 0) > return false; > return true; > - case SG_DXFER_TO_DEV: > case SG_DXFER_FROM_DEV: > + if (hp->dxferp || hp->dxfer_len < 0) > + return false; > + return true; > + case SG_DXFER_TO_DEV: > case SG_DXFER_TO_FROM_DEV: > if (!hp->dxferp || hp->dxfer_len == 0) > return false; >
On Thu, Jul 06, 2017 at 02:47:22PM -0400, Douglas Gilbert wrote: > Can you check your patch with one of the utilities from sg3_utils > such as sg_inq which will use SG_DXFER_FROM_DEV with the newer > interface? Correct, this patch broke sg_inq. I'll send a corrected v2. > BTW I'm not sure why dxferp is set to NULL for SG_DXFER_FROM_DEV > transfers; perhaps some magic done by the block layer. Maybe a > comment in the code (e.g. on line 654) would help. This is due to: commit fad7f01e61bf737fe8a3740d803f000db57ecac6 Author: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Date: Tue Sep 2 16:20:20 2008 +0900 sg: set dxferp to NULL for READ with the older SG interface With the older SG interface, we don't know a user-space address to trasfer data when executing a SCSI command. So we can't pass a user-space address to blk_rq_map_user. This patch fixes sg to pass a NULL user-space address to blk_rq_map_user so that it just sets up a request and bios with page frames propely without data transfer. Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Signed-off-by: Jens Axboe <jens.axboe@oracle.com> > > Also sg_is_valid_dxfer() is only called once and is more complex > than it looks; so perhaps it could be inlined back in > sg_common_write(). The compiler will inline it anyways (at least the one I checked with) and inlining it into sg_common_write() won't make the code more readable IMHO. But ultimately it's your driver so if you insist I'll do.
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index 21225d62b0c1..3c91593260aa 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp) if (hp->dxferp || hp->dxfer_len > 0) return false; return true; - case SG_DXFER_TO_DEV: case SG_DXFER_FROM_DEV: + if (hp->dxferp || hp->dxfer_len < 0) + return false; + return true; + case SG_DXFER_TO_DEV: case SG_DXFER_TO_FROM_DEV: if (!hp->dxferp || hp->dxfer_len == 0) return false;
SG_DXFER_FROM_DEV transfers do not have a dxferp as we set it to NULL, but must have a length bigger than 0. This fixes a regression introduced by commit 28676d869bbb ("scsi: sg: check for valid direction before starting the request") Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Fixes: 28676d869bbb ("scsi: sg: check for valid direction before starting the request") Reported-by: Chris Clayton <chris2553@googlemail.com> Tested-by: Chris Clayton <chris2553@googlemail.com> Cc: Doug Gilbert <dgilbert@interlog.com> --- drivers/scsi/sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)