[PATCH] scsi: Fix printing of failed 32-byte commands

[PATCH] scsi: Fix printing of failed 32-byte commands

[Martin K. Petersen]

Having the large CDB allocation logic in sd.c means that
scsi_io_completion does not have access to the command buffer. That in
turn causes garbage to be printed when a 32-byte command fails. Move the
command printing to sd_done where the command buffer is intact and
inform scsi_lib.c that no printing is necessary.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d892768..d716ff2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -897,7 +897,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			scsi_print_result(cmd);
 			if (driver_byte(result) & DRIVER_SENSE)
 				scsi_print_sense("", cmd);
-			scsi_print_command(cmd);
+			if (!cmd->device->prints_failed_commands)
+				scsi_print_command(cmd);
 		}
 		if (blk_end_request_err(req, error))
 			scsi_requeue_command(q, cmd);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 255da53..ad15c2e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1214,6 +1214,9 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 		break;
 	}
  out:
+	if (result)
+		scsi_print_command(SCpnt);
+
 	if (rq_data_dir(SCpnt->request) == READ && scsi_prot_sg_count(SCpnt))
 		sd_dif_complete(SCpnt, good_bytes);
 
@@ -2071,6 +2074,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	}
 
 	sdkp->first_scan = 0;
+	sdp->prints_failed_commands = 1;
 
 	/*
 	 * We now have all cache related info, determine how we deal
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7c44499..0d551a3 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -149,6 +149,7 @@ struct scsi_device {
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
+	unsigned prints_failed_commands:1;	/* ULD prints failed commands */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

Re: [PATCH] scsi: Fix printing of failed 32-byte commands

[Boaz Harrosh]

On 01/19/2010 01:42 AM, Martin K. Petersen wrote:
> 
> Having the large CDB allocation logic in sd.c means that
> scsi_io_completion does not have access to the command buffer. That in
> turn causes garbage to be printed when a 32-byte command fails. Move the
> command printing to sd_done where the command buffer is intact and
> inform scsi_lib.c that no printing is necessary.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index d892768..d716ff2 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -897,7 +897,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			scsi_print_result(cmd);
>  			if (driver_byte(result) & DRIVER_SENSE)
>  				scsi_print_sense("", cmd);
> -			scsi_print_command(cmd);
> +			if (!cmd->device->prints_failed_commands)
> +				scsi_print_command(cmd);

I don't like that flag. If cmd->cmnd is invalid after the call to sd_done.
Then I prefer if sd_done could NULL the pointer and any access will BUG,
instead of a dangerous use after free, which you never know when it will
bite.

Then if so scsi_print_command() can just silently ignore any
cmd->cmnd == NULL cases. (And a fat comment somewhere)

>  		}
>  		if (blk_end_request_err(req, error))
>  			scsi_requeue_command(q, cmd);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 255da53..ad15c2e 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1214,6 +1214,9 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  		break;
>  	}
>   out:
> +	if (result)
> +		scsi_print_command(SCpnt);
> +
>  	if (rq_data_dir(SCpnt->request) == READ && scsi_prot_sg_count(SCpnt))
>  		sd_dif_complete(SCpnt, good_bytes);
>  

You must do this in any case
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 255da53..cd5196d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1218,8 +1218,10 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 		sd_dif_complete(SCpnt, good_bytes);
 
 	if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type)
-	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd)
+	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd) {
 		mempool_free(SCpnt->cmnd, sd_cdb_pool);
+		SCpnt->cmnd = NULL;
+	}
 
 	return good_bytes;
 }


> @@ -2071,6 +2074,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	}
>  
>  	sdkp->first_scan = 0;
> +	sdp->prints_failed_commands = 1;
>  
>  	/*
>  	 * We now have all cache related info, determine how we deal
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 7c44499..0d551a3 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -149,6 +149,7 @@ struct scsi_device {
>  	unsigned last_sector_bug:1;	/* do not use multisector accesses on
>  					   SD_LAST_BUGGY_SECTORS */
>  	unsigned is_visible:1;	/* is the device visible in sysfs */
> +	unsigned prints_failed_commands:1;	/* ULD prints failed commands */
>  
>  	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
>  	struct list_head event_list;	/* asserted events */
> --

Thanks
Boaz

Re: [PATCH] scsi: Fix printing of failed 32-byte commands

[Martin K. Petersen]

>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:

Boaz> I don't like that flag. If cmd->cmnd is invalid after the call to
Boaz> sd_done.  Then I prefer if sd_done could NULL the pointer and any
Boaz> access will BUG, instead of a dangerous use after free, which you
Boaz> never know when it will bite.

Boaz> Then if so scsi_print_command() can just silently ignore any
cmd-> cmnd == NULL cases. (And a fat comment somewhere)

Yeah, that's a better idea.  I considered that flag a wart from the
get-go...


scsi: Fix printing of failed 32-byte commands

Having the large CDB allocation logic in sd.c means that
scsi_io_completion does not have access to the command buffer. That in
turn causes garbage to be printed when a 32-byte command fails. Move the
command printing to sd_done where the command buffer is intact.  Clear
the command buffer pointer after the extended CDB has been freed.

Make scsi_print_command ignore commands with NULL CDB pointers to
inhibit printing of garbled command strings.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index db68e3b..93606bc 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -349,6 +349,9 @@ void scsi_print_command(struct scsi_cmnd *cmd)
 {
 	int k;
 
+	if (cmd->cmnd == NULL)
+		return;
+
 	scmd_printk(KERN_INFO, cmd, "CDB: ");
 	print_opcode_name(cmd->cmnd, cmd->cmd_len);
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 255da53..5f0f6c7 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1218,8 +1218,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 		sd_dif_complete(SCpnt, good_bytes);
 
 	if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type)
-	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd)
+	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd) {
+
+		/* We have to print a failed command here as the
+		 * extended CDB gets freed before scsi_io_completion()
+		 * is called.
+		 */
+		if (result)
+			scsi_print_command(SCpnt);
+
 		mempool_free(SCpnt->cmnd, sd_cdb_pool);
+		SCpnt->cmnd = NULL;
+		SCpnt->cmd_len = 0;
+	}
 
 	return good_bytes;
 }

Re: [PATCH] scsi: Fix printing of failed 32-byte commands

[Boaz Harrosh]

On 01/20/2010 09:20 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
> Boaz> I don't like that flag. If cmd->cmnd is invalid after the call to
> Boaz> sd_done.  Then I prefer if sd_done could NULL the pointer and any
> Boaz> access will BUG, instead of a dangerous use after free, which you
> Boaz> never know when it will bite.
> 
> Boaz> Then if so scsi_print_command() can just silently ignore any
> cmd-> cmnd == NULL cases. (And a fat comment somewhere)
> 
> Yeah, that's a better idea.  I considered that flag a wart from the
> get-go...
> 
> 
> scsi: Fix printing of failed 32-byte commands
> 
> Having the large CDB allocation logic in sd.c means that
> scsi_io_completion does not have access to the command buffer. That in
> turn causes garbage to be printed when a 32-byte command fails. Move the
> command printing to sd_done where the command buffer is intact.  Clear
> the command buffer pointer after the extended CDB has been freed.
> 
> Make scsi_print_command ignore commands with NULL CDB pointers to
> inhibit printing of garbled command strings.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 

Grate, thanks looks much better.
Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>

> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index db68e3b..93606bc 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -349,6 +349,9 @@ void scsi_print_command(struct scsi_cmnd *cmd)
>  {
>  	int k;
>  
> +	if (cmd->cmnd == NULL)
> +		return;
> +
>  	scmd_printk(KERN_INFO, cmd, "CDB: ");
>  	print_opcode_name(cmd->cmnd, cmd->cmd_len);
>  
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 255da53..5f0f6c7 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1218,8 +1218,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  		sd_dif_complete(SCpnt, good_bytes);
>  
>  	if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type)
> -	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd)
> +	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd) {
> +
> +		/* We have to print a failed command here as the
> +		 * extended CDB gets freed before scsi_io_completion()
> +		 * is called.
> +		 */
> +		if (result)
> +			scsi_print_command(SCpnt);
> +
>  		mempool_free(SCpnt->cmnd, sd_cdb_pool);
> +		SCpnt->cmnd = NULL;
> +		SCpnt->cmd_len = 0;
> +	}
>  
>  	return good_bytes;
>  }