| Message ID | 20170523234854.21452-5-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: > For VERIFY and WRITE AND VERIFY commands the size of the SCSI > Data-Out buffer can differ from the size of the data area on the > storage medium that is affected by the command. Make sure that > the Data-Out buffer size is computed correctly if the BYTCHK > field in the CDB is zero. This patch reverts commit 984a9d4c40be > and thereby restores commit 0e2eb7d12eaa. Additionally, > sbc_parse_cdb() is modified such that the data buffer size is > computed correctly for the affected commands if BYTCHK == 0. > This patch is the combination of two patches that got positive > reviews. > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"") > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > Cc: <stable@vger.kernel.org> > --- > drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------ > 1 file changed, 67 insertions(+), 12 deletions(-) > This patch ignored the review comments from the last round: http://www.spinics.net/lists/target-devel/msg15306.html http://www.spinics.net/lists/target-devel/msg15327.html Until these are addressed as requested, dropping this patch for now. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote: > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI > > Data-Out buffer can differ from the size of the data area on the > > storage medium that is affected by the command. Make sure that > > the Data-Out buffer size is computed correctly if the BYTCHK > > field in the CDB is zero. This patch reverts commit 984a9d4c40be > > and thereby restores commit 0e2eb7d12eaa. Additionally, > > sbc_parse_cdb() is modified such that the data buffer size is > > computed correctly for the affected commands if BYTCHK == 0. > > This patch is the combination of two patches that got positive > > reviews. > > > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"") > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Cc: Hannes Reinecke <hare@suse.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Andy Grover <agrover@redhat.com> > > Cc: David Disseldorp <ddiss@suse.de> > > Cc: <stable@vger.kernel.org> > > --- > > drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------ > > 1 file changed, 67 insertions(+), 12 deletions(-) > > > > This patch ignored the review comments from the last round: > > http://www.spinics.net/lists/target-devel/msg15306.html > http://www.spinics.net/lists/target-devel/msg15327.html > > Until these are addressed as requested, dropping this patch for now. Hello Nic, 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. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote: > On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote: > > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: > > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI > > > Data-Out buffer can differ from the size of the data area on the > > > storage medium that is affected by the command. Make sure that > > > the Data-Out buffer size is computed correctly if the BYTCHK > > > field in the CDB is zero. This patch reverts commit 984a9d4c40be > > > and thereby restores commit 0e2eb7d12eaa. Additionally, > > > sbc_parse_cdb() is modified such that the data buffer size is > > > computed correctly for the affected commands if BYTCHK == 0. > > > This patch is the combination of two patches that got positive > > > reviews. > > > > > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"") > > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > Cc: Hannes Reinecke <hare@suse.com> > > > Cc: Christoph Hellwig <hch@lst.de> > > > Cc: Andy Grover <agrover@redhat.com> > > > Cc: David Disseldorp <ddiss@suse.de> > > > Cc: <stable@vger.kernel.org> > > > --- > > > drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------ > > > 1 file changed, 67 insertions(+), 12 deletions(-) > > > > > > > This patch ignored the review comments from the last round: > > > > http://www.spinics.net/lists/target-devel/msg15306.html > > http://www.spinics.net/lists/target-devel/msg15327.html > > > > Until these are addressed as requested, dropping this patch for now. > > Hello Nic, > > 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. 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. Tthat said, I already reverted this patch once because you didn't want to make these very simple changes, so I'll not be merging it until the changes are made. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: > > On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote: > > > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: > > > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI > > > > Data-Out buffer can differ from the size of the data area on the > > > > storage medium that is affected by the command. Make sure that > > > > the Data-Out buffer size is computed correctly if the BYTCHK > > > > field in the CDB is zero. This patch reverts commit 984a9d4c40be > > > > and thereby restores commit 0e2eb7d12eaa. Additionally, > > > > sbc_parse_cdb() is modified such that the data buffer size is > > > > computed correctly for the affected commands if BYTCHK == 0. > > > > This patch is the combination of two patches that got positive > > > > reviews. > > > > > > > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"") > > > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > > Cc: Hannes Reinecke <hare@suse.com> > > > > Cc: Christoph Hellwig <hch@lst.de> > > > > Cc: Andy Grover <agrover@redhat.com> > > > > Cc: David Disseldorp <ddiss@suse.de> > > > > Cc: <stable@vger.kernel.org> > > > > --- > > > > drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------ > > > > 1 file changed, 67 insertions(+), 12 deletions(-) > > > > > > > > > > This patch ignored the review comments from the last round: > > > > > > http://www.spinics.net/lists/target-devel/msg15306.html > > > http://www.spinics.net/lists/target-devel/msg15327.html > > > > > > Until these are addressed as requested, dropping this patch for now. > > > > Hello Nic, > > > > 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. or rather, and only sets it (SCF_SCSI_DATA_CDB) for BYTCHK=1. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 5, 2017 at 10:49 AM, Bart Van Assche <Bart.VanAssche@sandisk.com> wrote: > 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. I think this is true for VERIFY; but WRITE VERIFY is not so clear. It looks to me like the SBC-4r13 description of WRITE VERIFY is broken. It refers to VERIFY for the definition of BYTCHK=01, but the definition in VERIFY refers to a CDB field that does not exist in the WRITE VERIFY CDB (VERIFICATION LENGTH). In WRITE VERIFY the most similar field is TRANSFER LENGTH. I take that to be intended as the amount of data to be written during the WRITE step. Since the spec refers to the nonexistent VERIFICATION LENGTH field for BYTCHK=01, we have to guess that they intend the TRANSFER LENGTH to be used also as the length of the VERIFY step, when BYTCHK=01. In WRITE VERIFY with BYTCHK=00, I think we must consider the TRANSFER LENGTH (usually nonzero) to apply to the WRITE step, no matter the setting of BYTCHK. Since the spec provides the BYTCHK field for WRITE VERIFY and refers to its definition in VERIFY, I take that to mean the intention is that BYTCHK=00 be treated as with VERIFY; in other words, the VERIFY step ignores TRANSFER LENGTH (even though the WRITE step uses it), and the VERIFY step checks the protection information based on the VRPROTECT field in the CDB (WRPROTECT in the WRITE CDB), as described in the VERIFY section. I think the spec is broken here, and this is my plausible interpretation of the intent. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-06-05 at 16:32 -0600, David Butterfield wrote: > Since the spec provides the BYTCHK field for WRITE VERIFY and refers to its > definition in VERIFY, I take that to mean the intention is that BYTCHK=00 be > treated as with VERIFY; in other words, the VERIFY step ignores TRANSFER > LENGTH (even though the WRITE step uses it), and the VERIFY step checks > the protection information based on the VRPROTECT field in the CDB > (WRPROTECT in the WRITE CDB), as described in the VERIFY section. > > I think the spec is broken here, and this is my plausible > interpretation of the intent. Hello Dave, That's a good point. As far as I know only AIX submits the WRITE AND VERIFY command. So it would be useful to know what value AIX sets the BYTCHK field to. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 4316f7b65fb7..51489d96cb31 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -831,12 +831,67 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb) return 0; } +/** + * sbc_parse_verify - parse VERIFY, VERIFY_16 and WRITE VERIFY commands + * @cmd: (in) structure that describes the SCSI command to be parsed. + * @sectors: (out) Number of logical blocks on the storage medium that will be + * affected by the SCSI command. + * @bufflen: (out) Expected length of the SCSI Data-Out buffer. + */ +static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors, + u32 *bufflen) +{ + struct se_device *dev = cmd->se_dev; + u8 *cdb = cmd->t_task_cdb; + u8 bytchk = (cdb[1] >> 1) & 3; + sense_reason_t ret; + + switch (cdb[0]) { + case VERIFY: + case WRITE_VERIFY: + *sectors = transport_get_sectors_10(cdb); + cmd->t_task_lba = transport_lba_32(cdb); + break; + case VERIFY_16: + case WRITE_VERIFY_16: + *sectors = transport_get_sectors_16(cdb); + cmd->t_task_lba = transport_lba_64(cdb); + break; + default: + WARN_ON_ONCE(true); + return TCM_UNSUPPORTED_SCSI_OPCODE; + } + + if (sbc_check_dpofua(dev, cmd, cdb)) + return TCM_INVALID_CDB_FIELD; + + ret = sbc_check_prot(dev, cmd, cdb, *sectors, true); + if (ret) + return ret; + + switch (bytchk) { + case 0: + *bufflen = 0; + break; + case 1: + *bufflen = sbc_get_size(cmd, *sectors); + cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; + break; + default: + pr_err("Unsupported BYTCHK value %d for SCSI opcode %#x\n", + bytchk, cdb[0]); + return TCM_INVALID_CDB_FIELD; + } + return TCM_NO_SENSE; +} + sense_reason_t sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) { + enum { INVALID_SIZE = 1 }; struct se_device *dev = cmd->se_dev; unsigned char *cdb = cmd->t_task_cdb; - unsigned int size; + unsigned int size = INVALID_SIZE; u32 sectors = 0; sense_reason_t ret; @@ -898,7 +953,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) cmd->execute_cmd = sbc_execute_rw; break; case WRITE_10: - case WRITE_VERIFY: sectors = transport_get_sectors_10(cdb); cmd->t_task_lba = transport_lba_32(cdb); @@ -912,6 +966,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; cmd->execute_cmd = sbc_execute_rw; break; + case WRITE_VERIFY: + case WRITE_VERIFY_16: + ret = sbc_parse_verify(cmd, §ors, &size); + if (ret) + return ret; + cmd->execute_cmd = sbc_execute_rw; + goto check_lba; case WRITE_12: sectors = transport_get_sectors_12(cdb); cmd->t_task_lba = transport_lba_32(cdb); @@ -927,7 +988,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) cmd->execute_cmd = sbc_execute_rw; break; case WRITE_16: - case WRITE_VERIFY_16: sectors = transport_get_sectors_16(cdb); cmd->t_task_lba = transport_lba_64(cdb); @@ -1110,14 +1170,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) break; case VERIFY: case VERIFY_16: - size = 0; - if (cdb[0] == VERIFY) { - sectors = transport_get_sectors_10(cdb); - cmd->t_task_lba = transport_lba_32(cdb); - } else { - sectors = transport_get_sectors_16(cdb); - cmd->t_task_lba = transport_lba_64(cdb); - } + ret = sbc_parse_verify(cmd, §ors, &size); + if (ret) + return ret; cmd->execute_cmd = sbc_emulate_noop; goto check_lba; case REZERO_UNIT: @@ -1158,7 +1213,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) return TCM_ADDRESS_OUT_OF_RANGE; } - if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) + if (size == INVALID_SIZE) size = sbc_get_size(cmd, sectors); }
For VERIFY and WRITE AND VERIFY commands the size of the SCSI Data-Out buffer can differ from the size of the data area on the storage medium that is affected by the command. Make sure that the Data-Out buffer size is computed correctly if the BYTCHK field in the CDB is zero. This patch reverts commit 984a9d4c40be and thereby restores commit 0e2eb7d12eaa. Additionally, sbc_parse_cdb() is modified such that the data buffer size is computed correctly for the affected commands if BYTCHK == 0. This patch is the combination of two patches that got positive reviews. References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"") References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Andy Grover <agrover@redhat.com> Cc: David Disseldorp <ddiss@suse.de> Cc: <stable@vger.kernel.org> --- drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 12 deletions(-)