Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Ewan Milne <emilne@redhat.com>]

On Mon, 2013-01-21 at 08:26 +0100, Hannes Reinecke wrote:
> On 01/18/2013 05:46 PM, James Bottomley wrote:
> > On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> >> --- a/drivers/scsi/scsi_error.c
> >> +++ b/drivers/scsi/scsi_error.c
> >> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> >>   	if (! scsi_command_normalize_sense(scmd, &sshdr))
> >>   		return FAILED;	/* no valid sense data */
> >>
> >> +	if (sshdr.overflow)
> >> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> >> +
> >>   	if (scsi_sense_is_deferred(&sshdr))
> >>   		return NEEDS_RETRY;
> >>
> >> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> >>   			sshdr->asc = sense_buffer[2];
> >>   		if (sb_len > 3)
> >>   			sshdr->ascq = sense_buffer[3];
> >> +		if (sb_len > 4)
> >> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
> >>   		if (sb_len > 7)
> >>   			sshdr->additional_length = sense_buffer[7];
> >>   	} else {
> >>   		/*
> >>   		 * fixed format
> >>   		 */
> >> -		if (sb_len > 2)
> >> +		if (sb_len > 2) {
> >> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
> >>   			sshdr->sense_key = (sense_buffer[2] & 0xf);
> >> +		}
> >>   		if (sb_len > 7) {
> >>   			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> >>   					 sb_len : (sense_buffer[7] + 8);
> >
> > This isn't the right way to do it:  The overflow bit is a recent
> > introduction in SPC-4.  The correct way to tell if we have an overflow
> > or not is to look at the additional sense length and compare it to the
> > allocation length; this will work for everything.
> >
> > I'm not even convinced that overflow is important: for a lot of the
> > sense probes, we deliberately induce overflows by giving the request
> > sense command a short buffer.  Printing a warning in scsi_check_sense
> > will get very noisy very fast.
> >
> And indeed I would rather prefer to have it the other way round;
> we're using a fixed sense_buffer within the SCSI stack, which might
> not be large enough to hold all sense data.
> So I would prefer to have an indicator on whether _the internal_ 
> sense buffer overflowed; this would even give us some valid use-case 
> now.

So, if I understand what you're saying, we could check for overflow if
(sense_data[7] + 8) > SCSI_SENSE_BUFFERSIZE

We could do that, certainly.  I think, though, that overflow of sense
data is more likely to occur if a sense buffer smaller than
SCSI_SENSE_BUFFERSIZE bytes is used, e.g. by a call to
scsi_eh_prep_cmnd(), which is an exported symbol.

The existing 96-byte SCSI_SENSE_BUFFERSIZE may well be big enough.
(I did increase it to the SPC-4 defined value of 252 bytes in a later
patch in the series if the appropriate kernel config option is enabled.)

> Plus we can add the sense buffer overflow bit to that if required.
> 
> Cheers,
> 
> Hannes

Thanks for your comments.