Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Fri, 2017-06-02 at 22:32 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote:
> > In this patch series I have addressed all comments that made sense to me. Sorry
> > if you feel offended because I had not addressed the two comments you referred to
> > above. The reason I had not addressed these comments is because these comments
> > are wrong in my opinion. Hence, please reconsider this patch.
> 
> Nope.  Here are the details again. 
> 
> First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases,
> and only sets it for BYTCHK=0.
> 
> Yes, I understand the spec says hosts are not supposed to send a payload
> when BYTCHK=0, but that doesn't stop some from trying.
> 
> Any CDB that can potentially allocate SGLS via target_alloc_sgl() must
> set this flag.  No other CDBs set SCF_SCSI_DATA_CDB based on bits in the
> CDB, and *_VERIFY is no exception.

A quote from the SBC-4 section about VERIFY(10): "If the byte check (BYTCHK)
field is set to 00b, then no Data-Out Buffer transfer shall occur". In other
words, if a VERIFY or WRITE VERIFY command is received with BYTCHK=0,
transferring the Data-Out buffer is not only superfluous it is also against
the SCSI specs. Today target_cmd_size_check() terminates SCSI commands for
which the size of the Data-Out buffer exceeds the expected size with
TCM_INVALID_CDB_FIELD so the data transfer doesn't happen anyway. Hence it
is not necessary to allocate an SGL with target_alloc_sgl() if BYTCHK=0.

> Secondly, the force setting of size in sbc_parse_verify(), instead of
> what was actually received over the write is totally wrong.  Like I said
> before, the size in sbc_parse_cdb() is what's extracted from the CDB
> transfer length, and not what the spec says the correct size should be.

Please take the SCSI specs seriously instead of ignoring the SCSI specs. I
think for VERIFY and WRITE VERIFY with BYTCHK=0, the size extracted from the
CDB should be zero bytes.

What's needed in my opinion to make VERIFY and WRITE VERIFY processing
compliant with the SCSI specs is the following:
- Patch 04/33 from this series that fixes the parsing of these commands.
- Patch 25/33 from this series that fixes handling of too large Data-Out
  buffers for the iSCSI target driver (the qla2xxx and ib_srpt target drivers
  already handle this case correctly).
- Patch 30/33 from this series that makes target_cmd_size_check() send the
  correct sense code to the initiator system for too large Data-Out buffers.

Bart.