From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.hgst.iphmx.com ([216.71.153.141]:24120 "EHLO esa3.hgst.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753494AbdFEQty (ORCPT ); Mon, 5 Jun 2017 12:49:54 -0400 From: Bart Van Assche To: "nab@linux-iscsi.org" CC: "hch@lst.de" , "ddiss@suse.de" , "hare@suse.com" , "target-devel@vger.kernel.org" , "agrover@redhat.com" , "stable@vger.kernel.org" Subject: Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands Date: Mon, 5 Jun 2017 16:49:50 +0000 Message-ID: <1496681389.2623.11.camel@sandisk.com> References: <20170523234854.21452-1-bart.vanassche@sandisk.com> <20170523234854.21452-5-bart.vanassche@sandisk.com> <1496376930.27407.234.camel@haakon3.risingtidesystems.com> <1496422372.1214.9.camel@sandisk.com> <1496467944.27407.299.camel@haakon3.risingtidesystems.com> In-Reply-To: <1496467944.27407.299.camel@haakon3.risingtidesystems.com> Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Sender: stable-owner@vger.kernel.org List-ID: 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 m= e. Sorry > > if you feel offended because I had not addressed the two comments you r= eferred to > > above. The reason I had not addressed these comments is because these c= omments > > are wrong in my opinion. Hence, please reconsider this patch. >=20 > Nope. Here are the details again.=20 >=20 > First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases, > and only sets it for BYTCHK=3D0. >=20 > Yes, I understand the spec says hosts are not supposed to send a payload > when BYTCHK=3D0, but that doesn't stop some from trying. >=20 > 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 othe= r words, if a VERIFY or WRITE VERIFY command is received with BYTCHK=3D0, transferring the Data-Out buffer is not only superfluous it is also against the SCSI specs.=A0Today target_cmd_size_check() terminates SCSI commands fo= r 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=3D0. > 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=3D0, 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 drive= rs 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.=