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

[Ewan Milne <emilne@redhat.com>]

On Wed, 2013-01-23 at 13:06 +0000, James Bottomley wrote:
> 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?

I'm sure that's the case, I just don't know if the minimum is big
enough.  See below.

> 
> > 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.

My thinking was that it was important to log the fact that the device
had reported a Unit Attention queue overflow, which is reported in a
sense key specific descriptor, so I was concerned that if the sense
buffer wasn't big enough, the host would never see that there had
been a UA queue overflow.  That was why I had added the logging of
a sense buffer overflow.  I wasn't so much interested in reporting
sense buffer overflow for its own sake.

So, as it stands right now, I think I'll remove the 1/9 component
of the patch set, unless there is consensus that it is really needed.

> 
> James
> 
>