[PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

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;
-- 
2.12.3

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Douglas Gilbert]

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;
> 

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

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.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

On Tue, Jul 18, 2017 at 06:18:29PM +0200, Greg KH wrote:
> On Tue, Jul 18, 2017 at 11:54:31AM -0400, Martin K. Petersen wrote:
> > 
> > Greg,
> > 
> > > On Tue, Jul 18, 2017 at 09:42:21AM +0200, Johannes Thumshirn wrote:
> > >> (commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream)
> > >
> > > There is no such commit in Linus's tree :(
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/sg.c?id=68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47
> 
> {sigh} I hadn't updated my local tree.
> 
> As this isn't in a -rc kernel yet, is it ok for me to wait until it
> shows up in one before adding it to a stable tree?
> 
> And why wasn't it marked with a "cc: stable" in it so this would all
> happen automatically?

Because I forgot it. I'm sorry.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Greg Kroah-Hartman]

On Tue, Jul 18, 2017 at 11:54:31AM -0400, Martin K. Petersen wrote:
> 
> Greg,
> 
> > On Tue, Jul 18, 2017 at 09:42:21AM +0200, Johannes Thumshirn wrote:
> >> (commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream)
> >
> > There is no such commit in Linus's tree :(
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/sg.c?id=68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47

{sigh} I hadn't updated my local tree.

As this isn't in a -rc kernel yet, is it ok for me to wait until it
shows up in one before adding it to a stable tree?

And why wasn't it marked with a "cc: stable" in it so this would all
happen automatically?

thanks,

greg k-h

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Martin K. Petersen]

Greg,

> On Tue, Jul 18, 2017 at 09:42:21AM +0200, Johannes Thumshirn wrote:
>> (commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream)
>
> There is no such commit in Linus's tree :(

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/sg.c?id=68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Greg Kroah-Hartman]

On Tue, Jul 18, 2017 at 09:42:21AM +0200, Johannes Thumshirn wrote:
> (commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream)

There is no such commit in Linus's tree :(

Where did this come from?

thanks,

greg k-h

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

On Tue, Jul 18, 2017 at 09:50:07AM +0200, Greg KH wrote:
> What stable tree(s) do you want this to be applied to?

I only have reports for 4.12 and this is where the broken commit went in. So I
guess 4.12 should be sufficient.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Greg Kroah-Hartman]

On Tue, Jul 18, 2017 at 09:42:21AM +0200, Johannes Thumshirn wrote:
> (commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream)
> 
> SG_DXFER_FROM_DEV transfers do not necessarily have a dxferp as we set
> it to NULL for the old sg_io read/write interface, 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: Douglas Gilbert <dgilbert@interlog.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Tested-by: Chris Clayton <chris2553@googlemail.com>
> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/sg.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

What stable tree(s) do you want this to be applied to?

thanks,

greg k-h

[PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

(commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream)

SG_DXFER_FROM_DEV transfers do not necessarily have a dxferp as we set
it to NULL for the old sg_io read/write interface, 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: Douglas Gilbert <dgilbert@interlog.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Tested-by: Chris Clayton <chris2553@googlemail.com>
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.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..1e82d4128a84 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->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;
-- 
2.12.3