iscsi-target: Reject immediate data underflow larger than SCSI transfer length

Commit Message

Nicholas A. Bellinger July 13, 2017, 7:27 p.m. UTC

On Tue, 2017-07-11 at 16:17 +0000, Bart Van Assche wrote:
> On Tue, 2017-07-11 at 00:22 -0700, Nicholas A. Bellinger wrote:
> > So rejecting this case as already done in commit abb85a9b51 is the
> > correct approach for >= v4.3.y.
> 
> Hello Nic,
> 
> I hope that you agree that the current target_cmd_size_check() implementation
> is complicated and ugly. Patch 30/33 of the patch series I referred to in my
> e-mail removes a significant number of lines of code from that function. So
> my patch series not only makes target_cmd_size_check() easier to maintain and
> to verify but it makes that function also faster. Hence please reconsider the
> approach from my patch series. For patch 30/33, see also
> https://www.spinics.net/lists/target-devel/msg15384.html.

For reference, here is the patch your advocating:


The original code is not 'complicated' or 'ugly', and the proposed
change doesn't make anything 'faster' nor removes a 'significant' number
of LoC.

So no, I don't buy any of that line of reasoning.  ;-)

It comes down to two items.  First, if SCF_SCSI_DATA_CDBs with
underflow/overflow will be allowed, and second the block_size != 512
check.

For the former, I've still never seen a host environment in the wild
over the last 15 years that generates underflow/overflow for DATA CDBs
with an LBA.  So I'm reluctant to randomly allow this for all cases and
fabrics, considering no host environment actually requires it.

That said, I do understand libiscsi generates this for WRITE_VERIFY, but
I'm still undecided if that's a good enough a reason to enable it for
all cases in upstream.

For the latter item, it's fine to drop the legacy block_size != 512
check, and I'll take a patch for that separate from the other bit.

Comments

Bart Van Assche July 13, 2017, 11:24 p.m. UTC | #1

On Thu, 2017-07-13 at 12:27 -0700, Nicholas A. Bellinger wrote:
> For the former, I've still never seen a host environment in the wild
> over the last 15 years that generates underflow/overflow for DATA CDBs
> with an LBA.  So I'm reluctant to randomly allow this for all cases and
> fabrics, considering no host environment actually requires it.
> 
> That said, I do understand libiscsi generates this for WRITE_VERIFY, but
> I'm still undecided if that's a good enough a reason to enable it for
> all cases in upstream.
> 
> For the latter item, it's fine to drop the legacy block_size != 512
> check, and I'll take a patch for that separate from the other bit.

Hello Nic,

Thanks for considering to drop the block_size != 512 check.

There is one side effect of the current code for handling WRITE underflows
that has not yet been mentioned in this e-mail thread. The current
implementation of the iSCSI target driver does not discard the entire
immediate data buffer if the size of that buffer is larger than the data
buffer size derived from the SCSI CDB (EDTL). Because of this the iSCSI
target driver will attempt to parse some of the immediate data as an iSCSI
PDU. This can cause very weird failures and error messages. I think we
should address this and also that we should make sure that any iSCSI PDUs
that follow a too large immediate data buffer are parsed correctly.

Bart.

Patch

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c28e3b58150b..6cd49fe578a7 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1164,23 +1164,6 @@  target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 			" %u does not match SCSI CDB Length: %u for SAM Opcode:"
 			" 0x%02x\n", cmd->se_tfo->get_fabric_name(),
 				cmd->data_length, size, cmd->t_task_cdb[0]);
-
-		if (cmd->data_direction == DMA_TO_DEVICE &&
-		    cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
-			pr_err("Rejecting underflow/overflow WRITE data\n");
-			return TCM_INVALID_CDB_FIELD;
-		}
-		/*
-		 * Reject READ_* or WRITE_* with overflow/underflow for
-		 * type SCF_SCSI_DATA_CDB.
-		 */
-		if (dev->dev_attrib.block_size != 512)  {
-			pr_err("Failing OVERFLOW/UNDERFLOW for LBA op"
-				" CDB on non 512-byte sector setup subsystem"
-				" plugin: %s\n", dev->transport->name);
-			/* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
-			return TCM_INVALID_CDB_FIELD;
-		}
 		/*
 		 * For the overflow case keep the existing fabric provided
 		 * ->data_length.  Otherwise for the underflow case, reset