Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

[Boaz Harrosh]

We should switch this topic to the scsi mailing list

On 08/17/2012 01:49 PM, Boaz Harrosh wrote:

> On 08/17/2012 01:12 AM, Nicholas A. Bellinger wrote:
> 
>> On Thu, 2012-08-16 at 09:24 -0700, Roland Dreier wrote:
>>> Actually I'm not sure removing cmd_spdtl is the right answer.
>>>
>>> If /dev/sda is a device on an iSCSI initiator exported by an LIO
>>> target, try doing:
>>>
>>> # sg_raw -r512 /dev/sda 28 0 0 0 0 0 0 0 2 0
>>>
>>> This issues a READ (10) for 2 sectors, but only sends a length of 512
>>> at the transfer level.
>>>
>>> The target responds by setting the residual to 512 but transmits all
>>> 1024 bytes, 
> 
> 
> Is this the correct behavior from the Target? I would imagine that the
> target needs to only send 512 bytes (transfer level size) and set the
> OVERFLOW bit and residual to 512.
> 
> Not that it really matter because as you stated below the Initiator makes
> sure nothing bad happens.
> 
> BTW what target are we talking about, on the other side of the initiator
> here? (There are two targets in this setup right?)
> 
>>> and the Linux initiator at least rejects it because it
>>> hits:
>>>
>>> 	if (tcp_task->data_offset + tcp_conn->in.datalen > total_in_length) {
>>> 		ISCSI_DBG_TCP(conn, "data_offset(%d) + data_len(%d) > "
>>> 			      "total_length_in(%d)\n", tcp_task->data_offset,
>>> 			      tcp_conn->in.datalen, total_in_length);
>>> 		return ISCSI_ERR_DATA_OFFSET;
>>> 	}
>>>
>>
>> Ok, this is the 'overflow' case when the fabric ->data_length (expected
>> data transfer length of the initiator's buffer) is smaller than the SCSI
>> CDB allocation length or sectors*block_size (attempted transfer length)
>> the target has been asked to process.
>>
>> The following patch which appears to do the right thing from the
>> perspective of the target for overflow, but AFAICT open-iscsi still
>> returns GOOD status w/ no data-in payload (at least via sg_raw) when the
>> iscsi-target is signaling overflow bit in iSCSI Data IN PDU.  Not sure
>> yet why this is the case, but drivers/scsi/libiscsi.c:iscsi_data_in_rsp
>> code:
>>
>>        if (rhdr->flags & (ISCSI_FLAG_DATA_UNDERFLOW |
>>                            ISCSI_FLAG_DATA_OVERFLOW)) {
>>                 int res_count = be32_to_cpu(rhdr->residual_count);
>>
>>                 if (res_count > 0 &&
>>                     (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
>>                      res_count <= scsi_in(sc)->length))
>>                         scsi_in(sc)->resid = res_count;
>>                 else
>>                         sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
>>         }
>>
> 
> 
> OK I admit I kind of rearranged all this code a few years ago. Guilty ;-) 
> 
> Well now that I look at it again, I think it is totally wrong!!
> The scsi and block layer do not know anything about CMD_OVERFLOW
> If a scsi_in/out(sc)->resid is set it only ever means UNDERFLOW.
> 
> Both scsi and block expects to only do transfer_length - resid.
> This is why you get empty buffer back because the transfer_length=512
> minus resid=512 means zero bytes.
> 
> So the "|| ISCSI_FLAG_CMD_OVERFLOW" case is wrong.
> 
> Now the big question is what to do. Fail the all command, or say
> nothing?
> 
> The correct thing is to teach the middle layers about overflow,
> With some kind of warning level system.
> I was also thinking that we can make resid signed and signal
> an overflow with a negative resid. Now the math of
> transfer_length - resid will become correct since it means
> more, in the case above 512 - (-512) would be our 1024 CDB len.
> 
> For now this code must be fixed. And the command must fail.
> Do you need that I prepare a patch? (Please you do it, I'm
> swamped, it'll take me time)
> There are 3 more places like this.
> 
> BTW did you notice how in the code above we have mixed up the
> use of: ISCSI_FLAG_DATA_OVERFLOW and ISCSI_FLAG_CMD_OVERFLOW?
> That's another bug, here it should be ISCSI_FLAG_DATA_OVERFLOW
> only. The other places with the other flags.
> 
>> appears to be setting resid for non bidi cases correctly, right..? (mnc
>> + boaz CC'ed)
>>
>> How about the following to ensure we restrict overflow to keep the
>> existing (smaller) cmd->data_length assignment, and only re-assign this
>> value for the underflow case..?  (hch CC'ed)
>>
>> WDYT..?
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 0eaae23..63b3594 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -1183,15 +1183,20 @@ int target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
>>                         /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
>>                         goto out_invalid_cdb_field;
>>                 }
>> -
>> +               /*
>> +                * For the overflow case keep the existing fabric provided
>> +                * ->data_length.  Otherwise for the underflow case, reset
>> +                * ->data_length to the smaller SCSI expected data transfer
>> +                * length.
>> +                */
>>                 if (size > cmd->data_length) {
> 
> 
> I'm a bit out of context. Is this code exercised on the first target that is in
> pass-through over the initiator. Or this code is at the other target at the other
> side of the initiator? (The final real target)  
> 
> Because if it's at the pass-through target then this test might or might not
> be correct because we lost the overflow information by now. (Now if resid was
> negative that would be another thing)
> 
>>                         cmd->se_cmd_flags |= SCF_OVERFLOW_BIT;
>>                         cmd->residual_count = (size - cmd->data_length);
>>                 } else {
>>                         cmd->se_cmd_flags |= SCF_UNDERFLOW_BIT;
>>                         cmd->residual_count = (cmd->data_length - size);
>> +                       cmd->data_length = size;
>>                 }
>> -               cmd->data_length = size;
>>         }
>>  
>>         return 0;
>>
> 
> 
> Thanks
> Boaz

Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

[Nicholas A. Bellinger]

(CC'ing linux-scsi as per request by Boaz)

On Fri, 2012-08-17 at 13:49 +0300, Boaz Harrosh wrote:
> On 08/17/2012 01:12 AM, Nicholas A. Bellinger wrote:
> 
> > On Thu, 2012-08-16 at 09:24 -0700, Roland Dreier wrote:
> >> Actually I'm not sure removing cmd_spdtl is the right answer.
> >>
> >> If /dev/sda is a device on an iSCSI initiator exported by an LIO
> >> target, try doing:
> >>
> >> # sg_raw -r512 /dev/sda 28 0 0 0 0 0 0 0 2 0
> >>
> >> This issues a READ (10) for 2 sectors, but only sends a length of 512
> >> at the transfer level.
> >>
> >> The target responds by setting the residual to 512 but transmits all
> >> 1024 bytes, 
> 
> 
> Is this the correct behavior from the Target? I would imagine that the
> target needs to only send 512 bytes (transfer level size) and set the
> OVERFLOW bit and residual to 512.
> 

So my proposed target patch below to keep the smaller cmd->data_length
assignment for the SCSI overflow case in target_cmd_size_check() makes
the target do the right thing here, which is:

Transfer 512 bytes and set the OVERFLOW bit in se_cmd (to signal to the
fabric to also set OVERFLOW bit) with a residual count of 512 bytes
reported during the DATAIN sequence back across the wire..


> Not that it really matter because as you stated below the Initiator makes
> sure nothing bad happens.
> 
> BTW what target are we talking about, on the other side of the initiator
> here? (There are two targets in this setup right?)
> 

This is on the final target side running TCM + fabric driver

> >> and the Linux initiator at least rejects it because it
> >> hits:
> >>
> >> 	if (tcp_task->data_offset + tcp_conn->in.datalen > total_in_length) {
> >> 		ISCSI_DBG_TCP(conn, "data_offset(%d) + data_len(%d) > "
> >> 			      "total_length_in(%d)\n", tcp_task->data_offset,
> >> 			      tcp_conn->in.datalen, total_in_length);
> >> 		return ISCSI_ERR_DATA_OFFSET;
> >> 	}
> >>
> > 
> > Ok, this is the 'overflow' case when the fabric ->data_length (expected
> > data transfer length of the initiator's buffer) is smaller than the SCSI
> > CDB allocation length or sectors*block_size (attempted transfer length)
> > the target has been asked to process.
> > 
> > The following patch which appears to do the right thing from the
> > perspective of the target for overflow, but AFAICT open-iscsi still
> > returns GOOD status w/ no data-in payload (at least via sg_raw) when the
> > iscsi-target is signaling overflow bit in iSCSI Data IN PDU.  Not sure
> > yet why this is the case, but drivers/scsi/libiscsi.c:iscsi_data_in_rsp
> > code:
> > 
> >        if (rhdr->flags & (ISCSI_FLAG_DATA_UNDERFLOW |
> >                            ISCSI_FLAG_DATA_OVERFLOW)) {
> >                 int res_count = be32_to_cpu(rhdr->residual_count);
> > 
> >                 if (res_count > 0 &&
> >                     (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
> >                      res_count <= scsi_in(sc)->length))
> >                         scsi_in(sc)->resid = res_count;
> >                 else
> >                         sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
> >         }
> > 
> 
> 
> OK I admit I kind of rearranged all this code a few years ago. Guilty ;-) 
> 
> Well now that I look at it again, I think it is totally wrong!!
> The scsi and block layer do not know anything about CMD_OVERFLOW
> If a scsi_in/out(sc)->resid is set it only ever means UNDERFLOW.
> 
> Both scsi and block expects to only do transfer_length - resid.
> This is why you get empty buffer back because the transfer_length=512
> minus resid=512 means zero bytes.
> 
> So the "|| ISCSI_FLAG_CMD_OVERFLOW" case is wrong.
> 

Mmmm, your're right..  I could have sworn at one point that open-iscsi
returned DID_BAD_TARGET status for all OVERFLOW cases, and set the
proper residual + completed scsi_cmnd for UNDERFLOW cases.

Doing the reverse in the current code is definitely not correct.

> Now the big question is what to do. Fail the all command, or say
> nothing?
> 
> The correct thing is to teach the middle layers about overflow,
> With some kind of warning level system.
> I was also thinking that we can make resid signed and signal
> an overflow with a negative resid. Now the math of
> transfer_length - resid will become correct since it means
> more, in the case above 512 - (-512) would be our 1024 CDB len.
> 
> For now this code must be fixed. And the command must fail.
> Do you need that I prepare a patch? (Please you do it, I'm
> swamped, it'll take me time)
> There are 3 more places like this.
> 

Swamped here as well atm..  mnc/hannes, would you mind taking a look at
a patch to address these bugs in open-iscsi..?  I'm happy to test them
against target-pending to make sure we get these correct, and add them
into scsi-testsuite for your own use.

> BTW did you notice how in the code above we have mixed up the
> use of: ISCSI_FLAG_DATA_OVERFLOW and ISCSI_FLAG_CMD_OVERFLOW?
> That's another bug, here it should be ISCSI_FLAG_DATA_OVERFLOW
> only. The other places with the other flags.
> 

<nod>

> > appears to be setting resid for non bidi cases correctly, right..? (mnc
> > + boaz CC'ed)
> > 
> > How about the following to ensure we restrict overflow to keep the
> > existing (smaller) cmd->data_length assignment, and only re-assign this
> > value for the underflow case..?  (hch CC'ed)
> > 
> > WDYT..?
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 0eaae23..63b3594 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1183,15 +1183,20 @@ int target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
> >                         /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
> >                         goto out_invalid_cdb_field;
> >                 }
> > -
> > +               /*
> > +                * For the overflow case keep the existing fabric provided
> > +                * ->data_length.  Otherwise for the underflow case, reset
> > +                * ->data_length to the smaller SCSI expected data transfer
> > +                * length.
> > +                */
> >                 if (size > cmd->data_length) {
> 
> 
> I'm a bit out of context. Is this code exercised on the first target that is in
> pass-through over the initiator. Or this code is at the other target at the other
> side of the initiator? (The final real target)  
> 

Yes, the final real target.  ;)

> Because if it's at the pass-through target then this test might or might not
> be correct because we lost the overflow information by now. (Now if resid was
> negative that would be another thing)
> 
> >                         cmd->se_cmd_flags |= SCF_OVERFLOW_BIT;
> >                         cmd->residual_count = (size - cmd->data_length);
> >                 } else {
> >                         cmd->se_cmd_flags |= SCF_UNDERFLOW_BIT;
> >                         cmd->residual_count = (cmd->data_length - size);
> > +                       cmd->data_length = size;
> >                 }
> > -               cmd->data_length = size;
> >         }
> >  
> >         return 0;
> > 
> 

Thanks for your comments Boaz!

--nab

Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

[Nicholas A. Bellinger]

On Fri, 2012-08-17 at 11:46 -0700, Roland Dreier wrote:
> On Thu, Aug 16, 2012 at 3:12 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 0eaae23..63b3594 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1183,15 +1183,20 @@ int target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
> >                         /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
> >                         goto out_invalid_cdb_field;
> >                 }
> > -
> > +               /*
> > +                * For the overflow case keep the existing fabric provided
> > +                * ->data_length.  Otherwise for the underflow case, reset
> > +                * ->data_length to the smaller SCSI expected data transfer
> > +                * length.
> > +                */
> >                 if (size > cmd->data_length) {
> >                         cmd->se_cmd_flags |= SCF_OVERFLOW_BIT;
> >                         cmd->residual_count = (size - cmd->data_length);
> >                 } else {
> >                         cmd->se_cmd_flags |= SCF_UNDERFLOW_BIT;
> >                         cmd->residual_count = (cmd->data_length - size);
> > +                       cmd->data_length = size;
> >                 }
> > -               cmd->data_length = size;
> >         }
> >
> >         return 0;
> 
> I don't think this can work... if my memory of the code is right, if we do this,
> then the target core will only allocate the smaller (fabric) data length worth
> of buffer space, but then it will actually execute whatever the CDB says to
> do. 

No, or at least that is not what happens anymore with current target
core + iscsi-target code..

The se_cmd->data_length re-assignment here is what will be used by
iscsi-target fabric code for all iSCSI descriptor related allocations
+ransfer, instead of the original fabric 'expected transfer length' that
declares the size of the SCSI initiator's available buffer at the other
end.

At one point in v4.0 code iscsi-target had it's own
iscsi_cmd->data_length (which might be what your thinking of), but this
was removed during 3.4 by Agrover in favor of just using
se_cmd->data_length from the possibly underflow adjusted value.

>  So in my original example we're probably OK because we'll allocate a
> full page anyway, but I think something like:
> 
> # sg_raw -r512 /dev/sda 28 0 0 0 0 0 0 0 9 0
> 
> would cause the target core to allocate 1 page but then read 9 sectors.
> 

With the patch above to target_cmd_size_check() in place to skip
re-assignment of se_cmd->data_length for the overflow case, it appears
the target (at least) is doing the right thing now with your test case:

[root@initiator ~]# sg_raw -r512 /dev/sdi 28 0 0 0 0 0 0 0 9 0
sg_raw -r512 /dev/sdi 28 0 0 0 0 0 0 0 9 0
SCSI Status: Good 

Sense Information:
sense buffer empty

Received -3584 bytes of data:

Target side:

[82582.697668] Got SCSI Command, ITT: 0x7e000040, CmdSN: 0x000002d9, ExpXferLen: 512, Length: 0, CID: 0
[82582.697673] TARGET_CORE[iSCSI]: Expected Transfer Length: 512 does not match SCSI CDB Length: 4608 for SAM Opcode: 0x28
[82582.709696] Received CmdSN matches ExpCmdSN, incremented ExpCmdSN to: 0x000002da
[82582.709876] Updated MaxCmdSN to 0x000002f9
[82582.709881] Built DataIN ITT: 0x7e000040, StatSN: 0x905625fd, DataSN: 0x00000000, Offset: 0, Length: 512, CID: 0

So aside from the open-iscsi client bugs that Boaz has pointed out wrt
to overflow/underflow handling, I'll verify over the weekend with
loopback + tcm_qla2xxx fabrics and if everything looks fine will plan to
include this patch into master -> for next week's 3.6-rc-fixes PULL
request.

Thanks Roland!

--nab

Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

[Roland Dreier]

On Fri, Aug 17, 2012 at 6:02 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> No, or at least that is not what happens anymore with current target
> core + iscsi-target code..
>
> The se_cmd->data_length re-assignment here is what will be used by
> iscsi-target fabric code for all iSCSI descriptor related allocations
> +ransfer, instead of the original fabric 'expected transfer length' that
> declares the size of the SCSI initiator's available buffer at the other
> end.

Not sure I follow this.  Isn't cmd->data_length also what we use to
allocate the buffer used to store the data we're going to transfer?

I guess my example with READ(10) actually works, because
iblock_execute_rw() just uses the buffer allocated, rather than
paying attention to the sector count in the command.

But what if an initiator sends, say, REPORT LUNS or PR OUT
with an allocation length of 8192, but a transport-level length
of only 4096?  If the REPORT LUNS or PR OUT response is
bigger than 4096 bytes, we'll overflow the allocated buffer with
your patch, right?

 - R.

Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

[Nicholas A. Bellinger]

On Sat, 2012-08-18 at 10:13 -0700, Roland Dreier wrote:
> On Fri, Aug 17, 2012 at 6:02 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > No, or at least that is not what happens anymore with current target
> > core + iscsi-target code..
> >
> > The se_cmd->data_length re-assignment here is what will be used by
> > iscsi-target fabric code for all iSCSI descriptor related allocations
> > +ransfer, instead of the original fabric 'expected transfer length' that
> > declares the size of the SCSI initiator's available buffer at the other
> > end.
> 
> Not sure I follow this.  Isn't cmd->data_length also what we use to
> allocate the buffer used to store the data we're going to transfer?
> 

se_cmd->data_length is initially set up by transport_init_se_cmd() based
upon the fabric provided expected transfer length.  It then gets
adjusted (before any allocations happen) based upon underflow..   So in
the overflow case w/ the proposed patch, we want to keep the original
se_cmd->data_length because it's smaller than the CDB provided
allocation length.

> I guess my example with READ(10) actually works, because
> iblock_execute_rw() just uses the buffer allocated, rather than
> paying attention to the sector count in the command.
> 

Correct, this is true for all SCF_SCSI_DATA_CDB with virtual backends,
they no longer look into the CDB.

> But what if an initiator sends, say, REPORT LUNS or PR OUT
> with an allocation length of 8192, but a transport-level length
> of only 4096?  If the REPORT LUNS or PR OUT response is
> bigger than 4096 bytes, we'll overflow the allocated buffer with
> your patch, right?
> 

With virtual backends + TCM control CDB emulation, the emulation code
should be using (the possibly adjusted) se_cmd->data_length as it's
upper limit for building a response payload, and not the possibly
overflowed allocation length it may extracted from the CDB.

So as long as the control CDB emulation code is doing the right thing
here by using se_cmd->data_length, we should be OK for the virtual
backend cases.  (Need to audit this)

Now for pSCSI that may not be the case because we do expect underlying
layers to look into the CDB at some point, but currently we are passing
in SGLs based from the adjusted se_cmd->data_length, so this might end
up causing a problem in SCSI..  (Need to test this)

Otherwise, I'm tempted to just reject all CDB overflow cases for pSCSI
backends, as every host SCSI stack I've seen in practice only ends up
generating underflow cases for control CDBs anyways..

--nab