Re: [PATCH 10/10] scsi: Do not display buffer pointers in scsi_log_send()

[Hannes Reinecke <hare@suse.de>]

On 11/14/2014 11:53 PM, Elliott, Robert (Server Storage) wrote:
> 
> 
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:hare@suse.de]
>> Sent: Thursday, 06 November, 2014 2:31 AM
>> To: James Bottomley
>> Cc: Christoph Hellwig; Ewan Milne; Elliott, Robert (Server Storage);
>> linux-scsi@vger.kernel.org; Hannes Reinecke
>> Subject: [PATCH 10/10] scsi: Do not display buffer pointers in
>> scsi_log_send()
>>
>> scsi_log_send() would display buffer pointer for higher
>> logging levels. This is not only of questionable value
>> but also exposes kernel pointer to userspace, which is
>> discouraged in some setups. So drop this message
>> altogether.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/scsi.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 92d5912..9ec576d 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -531,7 +531,7 @@ void scsi_log_send(struct scsi_cmnd *cmd)
>>  	 *
>>  	 * 3: same as 2
>>  	 *
>> -	 * 4: same as 3 plus dump extra junk
>> +	 * 4: same as 3
>>  	 */
>>  	if (unlikely(scsi_logging_level)) {
>>  		level = SCSI_LOG_LEVEL(SCSI_LOG_MLQUEUE_SHIFT,
>> @@ -540,13 +540,6 @@ void scsi_log_send(struct scsi_cmnd *cmd)
>>  			scmd_printk(KERN_INFO, cmd,
>>  				    "Send: scmd 0x%p\n", cmd);
>>  			scsi_print_command(cmd);
>> -			if (level > 3) {
>> -				printk(KERN_INFO "buffer = 0x%p, bufflen =
>> %d,"
>> -				       " queuecommand 0x%p\n",
>> -					scsi_sglist(cmd), scsi_bufflen(cmd),
>> -					cmd->device->host->hostt-
>>> queuecommand);
>> -
>> -			}
>>  		}
>>  	}
>>  }
> 
> Reviewed-by: Robert Elliott <elliott@hp.com>
> 
> One more comment on the series:
> 
> After the tags, there are still a few scmd %p prints left
> including the one above.  Is this series supposed to get
> rid of the rest of them?
> 
> drivers/scsi/scsi.c:                                "Send: scmd 0x%p\n", cmd);
> drivers/scsi/scsi_error.c:                                  "scmd %p eh timeout, not aborting\n",
> drivers/scsi/scsi_error.c:                                  "aborting command %p\n", scmd));
> drivers/scsi/scsi_error.c:                                                  "scmd %p eh timeout, "
> drivers/scsi/scsi_error.c:                                                  "scmd %p retry "
> drivers/scsi/scsi_error.c:                                                  "scmd %p finish "
> drivers/scsi/scsi_error.c:                                          "scmd %p abort %s\n", scmd,
> drivers/scsi/scsi_error.c:                                  "scmd %p terminate "
> drivers/scsi/scsi_error.c:                                  "scmd %p previous abort failed\n", scmd));
> drivers/scsi/scsi_error.c:                                  "scmd %p not aborting, host in recovery\n",
> drivers/scsi/scsi_error.c:                          "scmd %p abort scheduled\n", scmd));
> drivers/scsi/scsi_error.c:                      "%s scmd: %p result: %x\n",
> drivers/scsi/scsi_error.c:                      "%s: scmd: %p, timeleft: %ld\n",
> drivers/scsi/scsi_error.c:                      "sense requested for %p result %x\n",
> drivers/scsi/scsi_error.c:              "%s: scmd %p rtn %x\n", __func__, scmd, rtn));
> drivers/scsi/scsi_error.c:                                           "%s: flush retry cmd: %p\n",
> drivers/scsi/scsi_error.c:                                           "%s: flush finish cmd: %p\n",
> drivers/scsi/scsi_lib.c:                "Inserting command %p into mlqueue\n", cmd));
> drivers/scsi/scsi_scan.c:               printk(KERN_WARNING "%s no slave_alloc function in hostt %p\n", __func__, shost->hostt);
> drivers/scsi/scsi_scan.c:               printk(KERN_WARNING "sdev %p scmd %p sending INQUIRY\n", sdev, scsi_cmd);
> drivers/scsi/scsi_scan.c:               printk(KERN_WARNING "%s sdev %p from lookup_by_target\n", __func__, sdev);
> drivers/scsi/scsi_scan.c:               printk(KERN_WARNING "%s sdev %p from alloc_sdev\n", __func__, sdev);
> drivers/scsi/scsi_scan.c:               printk(KERN_WARNING "%s sdev %p from alloc_sdev\n", __func__, sdev);
> drivers/scsi/scsi_scan.c:               printk(KERN_WARNING "%s sdev %p from lookup_by_target\n", __func__, sdev);
> drivers/scsi/scsi_scan.c:       printk(KERN_WARNING "%s sdev %p from alloc_sdev\n", __func__, sdev);
> 
Ideally, yes. However, for this to work we need an additional means
of identifying the command uniquely.
With the current infrastructure 'tags' are only ever assigned for
devices using the SCSI TCQ mechanism. Everything else is left in the
dark, even though it might be allowing for a queue depths > 1.
So I'll need to revisit this now that the tag rework from Christoph
Hellwig is in.
But I'd rather have this postponed to a next patch series. That
series would also included the proposed 'SUCCESS' to 'COMPLETE' change.

Cheers,

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