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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2013-01-22 at 10:08 -0500, Ewan Milne wrote:
> On Fri, 2013-01-18 at 16:46 +0000, 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.
> 
> Unfortunately, I am not sure that the allocation length that was sent
> to the device is always available.

Well, yes it is, we just don't store it.  scsi_normalize_sense() takes
as input the length of the sense buffer we gave it.  If we want an
overflow indication, we can certainly compare that against the
additional length (assuming we have enough bytes to get the additional
length).

>   I will look into this more closely
> but it appeared to me that e.g. FC drivers like qla2xxx get the sense
> data automatically from the HBA firmware.  In the case of that driver
> the host sense buffer size looks like it is hard-coded to 32 bytes,
> for all I know the firmware might only asking for 18 bytes.

You mean for their ACA emulation in the transport?  They have to be
picking up at least the standard minimum in order not to be in
violation, surely?

> Of course, for a normal REQUEST SENSE command where the allocation
> length is in the CDB, it would indeed be easy to add a check against
> the additional sense length.
> 
> > 
> > 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.
> 
> That would indeed be a problem.  I didn't see this behavior when testing
> the changes but I'll need to investigate this further.
> 
> The purpose of detecting the sense data overflow was to provide some
> visibility that a device is returning a large amount of sense data that
> is currently being silently ignored.  In the case of descriptor format
> sense data, it is possible that a descriptor we want to examine is
> located after one or more other descriptors, and we might not get it
> at all if the buffer isn't large enough.

But why should we care?  A lot of the time it will be spewing
descriptors with irrelevant FRU information.  I really think printing
there's been an overflow isn't a good idea.  I'm not sure there's much
use in the sshdr indicating there's been one.  It *may* be useful to
indicate how big the allocation length should have been, but I'm not
even convinced of that, since the data is now lost.

James