[PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer

[PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer

[Tony Battersby]

This patch fixes code such as the following with scsi-mq enabled:

    rq = blk_get_request(...);
    blk_rq_set_block_pc(rq);

    rq->cmd = my_cmd_buffer; /* separate CDB buffer */

    blk_execute_rq_nowait(...);

Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for
large CDBs only).  Without this patch, scsi_mq_prep_fn() will set
rq->cmd back to rq->__cmd, causing the wrong CDB to be sent to the device.

Signed-off-by: Tony Battersby <tonyb@cybernetics.com>

---

For inclusion in 3.17 only.

diff -urpN linux-3.17.0-rc1-a/block/blk-core.c linux-3.17.0-rc1-b/block/blk-core.c
--- linux-3.17.0-rc1-a/block/blk-core.c	2014-08-16 12:40:26.000000000 -0400
+++ linux-3.17.0-rc1-b/block/blk-core.c	2014-08-22 14:03:33.000000000 -0400
@@ -1252,7 +1252,6 @@ void blk_rq_set_block_pc(struct request 
 	rq->__sector = (sector_t) -1;
 	rq->bio = rq->biotail = NULL;
 	memset(rq->__cmd, 0, sizeof(rq->__cmd));
-	rq->cmd = rq->__cmd;
 }
 EXPORT_SYMBOL(blk_rq_set_block_pc);
 
diff -urpN linux-3.17.0-rc1-a/block/blk-mq.c linux-3.17.0-rc1-b/block/blk-mq.c
--- linux-3.17.0-rc1-a/block/blk-mq.c	2014-08-16 12:40:26.000000000 -0400
+++ linux-3.17.0-rc1-b/block/blk-mq.c	2014-08-22 14:02:32.000000000 -0400
@@ -172,6 +172,8 @@ static void blk_mq_rq_ctx_init(struct re
 	/* tag was already set */
 	rq->errors = 0;
 
+	rq->cmd = rq->__cmd;
+
 	rq->extra_len = 0;
 	rq->sense_len = 0;
 	rq->resid_len = 0;
diff -urpN linux-3.17.0-rc1-a/drivers/scsi/scsi_lib.c linux-3.17.0-rc1-b/drivers/scsi/scsi_lib.c
--- linux-3.17.0-rc1-a/drivers/scsi/scsi_lib.c	2014-08-16 12:40:26.000000000 -0400
+++ linux-3.17.0-rc1-b/drivers/scsi/scsi_lib.c	2014-08-22 14:02:32.000000000 -0400
@@ -1808,7 +1808,6 @@ static int scsi_mq_prep_fn(struct reques
 
 	cmd->tag = req->tag;
 
-	req->cmd = req->__cmd;
 	cmd->cmnd = req->cmd;
 	cmd->prot_op = SCSI_PROT_NORMAL;
 

Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer

[Douglas Gilbert]

On 14-08-22 03:53 PM, Tony Battersby wrote:
> This patch fixes code such as the following with scsi-mq enabled:
>
>      rq = blk_get_request(...);
>      blk_rq_set_block_pc(rq);
>
>      rq->cmd = my_cmd_buffer; /* separate CDB buffer */
>
>      blk_execute_rq_nowait(...);
>
> Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for
> large CDBs only).  Without this patch, scsi_mq_prep_fn() will set
> rq->cmd back to rq->__cmd, causing the wrong CDB to be sent to the device.

Still looking at this one. 'sg_write_same --32' is my
tool of choice. The target is scsi_debug which needs
dif=2 (because mkp read the draft and only allows
WRITE SAME(32) when the protection_level=2). Turned
on command tracing and this is what I saw:

sd 7:0:0:0: scsi_debug: cmd 9e 10 00 00 00 00 00 00 00 00 00 00 00 20 00 00
sd 7:0:0:0: scsi_debug: cmd 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 08 
24 6b d3 00 88 ff ff 20 00 c7 e8 00 00 00 00

So the WS(32) command did get through, it is the second
command that arrived on its tail that is a worry. So yes,
IMO it is broken.


Now the >16 byte cdb length in the sg driver
introduced in lk 3.17-rc1 was copied directly
from the bsg driver which has had that capability
for some time. Since your patch touches two files
in drivers/block I'm wondering the bsg driver's
 >16 byte cdb length capability is broken in
lk 3.16 ?

Doug Gilbert

Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer

[Douglas Gilbert]

On 14-08-22 08:52 PM, Douglas Gilbert wrote:
> On 14-08-22 03:53 PM, Tony Battersby wrote:
>> This patch fixes code such as the following with scsi-mq enabled:
>>
>>      rq = blk_get_request(...);
>>      blk_rq_set_block_pc(rq);
>>
>>      rq->cmd = my_cmd_buffer; /* separate CDB buffer */
>>
>>      blk_execute_rq_nowait(...);
>>
>> Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for
>> large CDBs only).  Without this patch, scsi_mq_prep_fn() will set
>> rq->cmd back to rq->__cmd, causing the wrong CDB to be sent to the device.
>
> Still looking at this one. 'sg_write_same --32' is my
> tool of choice. The target is scsi_debug which needs
> dif=2 (because mkp read the draft and only allows
> WRITE SAME(32) when the protection_level=2). Turned
> on command tracing and this is what I saw:
>
> sd 7:0:0:0: scsi_debug: cmd 9e 10 00 00 00 00 00 00 00 00 00 00 00 20 00 00
> sd 7:0:0:0: scsi_debug: cmd 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 08
> 24 6b d3 00 88 ff ff 20 00 c7 e8 00 00 00 00
>
> So the WS(32) command did get through, it is the second
> command that arrived on its tail that is a worry. So yes,
> IMO it is broken.

Got that wrong, that is a READ CAPACITY(16) that got
through properly (issued by sg_write_same) followed
by a 32 byte command of random bytes instead of the
requested WS(32), the first 6 bytes of which will
cause it to be parsed as a TEST UNIT READY.

> Now the >16 byte cdb length in the sg driver
> introduced in lk 3.17-rc1 was copied directly
> from the bsg driver which has had that capability
> for some time. Since your patch touches two files
> in drivers/block I'm wondering the bsg driver's
>  >16 byte cdb length capability is broken in
> lk 3.16 ?
>
> Doug Gilbert
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer

[Douglas Gilbert]

On 14-08-22 03:53 PM, Tony Battersby wrote:
> This patch fixes code such as the following with scsi-mq enabled:
>
>      rq = blk_get_request(...);
>      blk_rq_set_block_pc(rq);
>
>      rq->cmd = my_cmd_buffer; /* separate CDB buffer */
>
>      blk_execute_rq_nowait(...);
>
> Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for
> large CDBs only).  Without this patch, scsi_mq_prep_fn() will set
> rq->cmd back to rq->__cmd, causing the wrong CDB to be sent to the device.
>
> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>

Tested-by: Douglas Gilbert <dgilbert@interlog.com>


Patch tested on lk 3.17-rc1 with the sg driver using
the READ(32) command.

> For inclusion in 3.17 only.

May want to check if blk-mq work in lk 3.16 and earlier
breaks the bsg driver's capability to send SCSI cdbs
that are longer than 16 bytes.

> diff -urpN linux-3.17.0-rc1-a/block/blk-core.c linux-3.17.0-rc1-b/block/blk-core.c
> --- linux-3.17.0-rc1-a/block/blk-core.c	2014-08-16 12:40:26.000000000 -0400
> +++ linux-3.17.0-rc1-b/block/blk-core.c	2014-08-22 14:03:33.000000000 -0400
> @@ -1252,7 +1252,6 @@ void blk_rq_set_block_pc(struct request
>   	rq->__sector = (sector_t) -1;
>   	rq->bio = rq->biotail = NULL;
>   	memset(rq->__cmd, 0, sizeof(rq->__cmd));
> -	rq->cmd = rq->__cmd;
>   }
>   EXPORT_SYMBOL(blk_rq_set_block_pc);
>
> diff -urpN linux-3.17.0-rc1-a/block/blk-mq.c linux-3.17.0-rc1-b/block/blk-mq.c
> --- linux-3.17.0-rc1-a/block/blk-mq.c	2014-08-16 12:40:26.000000000 -0400
> +++ linux-3.17.0-rc1-b/block/blk-mq.c	2014-08-22 14:02:32.000000000 -0400
> @@ -172,6 +172,8 @@ static void blk_mq_rq_ctx_init(struct re
>   	/* tag was already set */
>   	rq->errors = 0;
>
> +	rq->cmd = rq->__cmd;
> +
>   	rq->extra_len = 0;
>   	rq->sense_len = 0;
>   	rq->resid_len = 0;
> diff -urpN linux-3.17.0-rc1-a/drivers/scsi/scsi_lib.c linux-3.17.0-rc1-b/drivers/scsi/scsi_lib.c
> --- linux-3.17.0-rc1-a/drivers/scsi/scsi_lib.c	2014-08-16 12:40:26.000000000 -0400
> +++ linux-3.17.0-rc1-b/drivers/scsi/scsi_lib.c	2014-08-22 14:02:32.000000000 -0400
> @@ -1808,7 +1808,6 @@ static int scsi_mq_prep_fn(struct reques
>
>   	cmd->tag = req->tag;
>
> -	req->cmd = req->__cmd;
>   	cmd->cmnd = req->cmd;
>   	cmd->prot_op = SCSI_PROT_NORMAL;
>
>
> --

Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer

[Tony Battersby]

On 08/23/2014 03:09 PM, Douglas Gilbert wrote:
>> For inclusion in 3.17 only.
> May want to check if blk-mq work in lk 3.16 and earlier
> breaks the bsg driver's capability to send SCSI cdbs
> that are longer than 16 bytes.
>
>

I think 3.16 and earlier are OK.  The problem was caused by
scsi_mq_prep_fn(), which is new in 3.17.

Thanks for testing.

Tony