Wrong resetting of Logical Unit Number field in CDB

Wrong resetting of Logical Unit Number field in CDB

[bodo.stroesser]

Hi,

We use TCMUser to run userspace tape and changer emulations.

Current tests with a new version of backup SW failed, due to that application
using SECURITY PROTOCOL IN / OUT  SCSI commands.
The CDBs of these commands in byte 1 contain relevant information that
is overwritten in passthrough_parse_cdb() / target_core_device.c

This is the related part of the code:

        /*
         * Clear a lun set in the cdb if the initiator talking to use spoke
         * and old standards version, as we can't assume the underlying device
         * won't choke up on it.
         */
        switch (cdb[0]) {
        case READ_10: /* SBC - RDProtect */
        case READ_12: /* SBC - RDProtect */
        case READ_16: /* SBC - RDProtect */
        case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
        case VERIFY: /* SBC - VRProtect */
        case VERIFY_16: /* SBC - VRProtect */
        case WRITE_VERIFY: /* SBC - VRProtect */
        case WRITE_VERIFY_12: /* SBC - VRProtect */
        case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
                break;
        default:
                cdb[1] &= 0x1f; /* clear logical unit number */
                break;
        }

Obviously the list of command codes for which bits 5,6,7 of byte 1 are _not_ reset
is not complete. Now I'm wondering what would be the right fix:

1) Scan the SCSI specs and add all other missing command codes to the list of
   Codes to skip

2) Create a list of commands that definitely have the LUN field in byte 1 and
   reset the bits only in those. (Might be better than 1), because I expect new
   commands to not have the LUN field.)

3) Remove the code entirely, because it is no longer needed / useful (?)

For 1) and 2) an additionally question is, based on what SCSI version the lists would
have to be created?

Based on your help and hints I'd like to create a patch.

Thank you,
Bodo

Re: Wrong resetting of Logical Unit Number field in CDB

[Bart Van Assche]

On 10/8/19 1:20 PM, bodo.stroesser@ts.fujitsu.com wrote:
> 3) Remove the code entirely, because it is no longer needed / useful (?)

I'd like to hear the opinion of Mike Christie. My favorite solution is 
(3) because I think the days of embedding LUN numbers in CDBs are long 
behind us.

Bart.

Re: Wrong resetting of Logical Unit Number field in CDB

[Christoph Hellwig]

On Tue, Oct 08, 2019 at 08:20:12PM +0000, bodo.stroesser@ts.fujitsu.com wrote:
> 1) Scan the SCSI specs and add all other missing command codes to the list of
>    Codes to skip
> 
> 2) Create a list of commands that definitely have the LUN field in byte 1 and
>    reset the bits only in those. (Might be better than 1), because I expect new
>    commands to not have the LUN field.)
> 
> 3) Remove the code entirely, because it is no longer needed / useful (?)
> 
> For 1) and 2) an additionally question is, based on what SCSI version the lists would
> have to be created?

(4) limit the clearing of the LUN field to devices that claim
a compliance to SCSI-2 or earlier, as those use the LUN field.

Re: Wrong resetting of Logical Unit Number field in CDB

[Bodo Stroesser]

On 10/09/19 09:00, Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 08:20:12PM +0000, bodo.stroesser@ts.fujitsu.com wrote:
>> 1) Scan the SCSI specs and add all other missing command codes to the list of
>>     Codes to skip
>>
>> 2) Create a list of commands that definitely have the LUN field in byte 1 and
>>     reset the bits only in those. (Might be better than 1), because I expect new
>>     commands to not have the LUN field.)
>>
>> 3) Remove the code entirely, because it is no longer needed / useful (?)
>>
>> For 1) and 2) an additionally question is, based on what SCSI version the lists would
>> have to be created?
> 
> (4) limit the clearing of the LUN field to devices that claim
> a compliance to SCSI-2 or earlier, as those use the LUN field.
> 
I'm not sure that (4) would work.
The comment in the code says:

   /*
    * Clear a lun set in the cdb if the initiator talking to use spoke
    * and old standards version, as we can't assume the underlying device
    * won't choke up on it.
    */

If I understand correctly, the purpose of the code is just the opposite 
from what you suggest, as it should reset useless LUN bits coming from
old (SCSI-2) initiators, that otherwise could confuse SCSI-3 or higher 
targets.
SCSI-2 compliant targets should not run into trouble due to LUN bits 
being set, I think.

Re: Wrong resetting of Logical Unit Number field in CDB

[Mike Christie]

On 10/08/2019 03:20 PM, bodo.stroesser@ts.fujitsu.com wrote:
> Hi,
> 
> We use TCMUser to run userspace tape and changer emulations.
> 
> Current tests with a new version of backup SW failed, due to that application
> using SECURITY PROTOCOL IN / OUT  SCSI commands.
> The CDBs of these commands in byte 1 contain relevant information that
> is overwritten in passthrough_parse_cdb() / target_core_device.c
> 
> This is the related part of the code:
> 
>         /*
>          * Clear a lun set in the cdb if the initiator talking to use spoke
>          * and old standards version, as we can't assume the underlying device
>          * won't choke up on it.
>          */
>         switch (cdb[0]) {
>         case READ_10: /* SBC - RDProtect */
>         case READ_12: /* SBC - RDProtect */
>         case READ_16: /* SBC - RDProtect */
>         case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
>         case VERIFY: /* SBC - VRProtect */
>         case VERIFY_16: /* SBC - VRProtect */
>         case WRITE_VERIFY: /* SBC - VRProtect */
>         case WRITE_VERIFY_12: /* SBC - VRProtect */
>         case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
>                 break;
>         default:
>                 cdb[1] &= 0x1f; /* clear logical unit number */
>                 break;
>         }
> 
> Obviously the list of command codes for which bits 5,6,7 of byte 1 are _not_ reset
> is not complete. Now I'm wondering what would be the right fix:
> 
> 1) Scan the SCSI specs and add all other missing command codes to the list of
>    Codes to skip
> 
> 2) Create a list of commands that definitely have the LUN field in byte 1 and
>    reset the bits only in those. (Might be better than 1), because I expect new
>    commands to not have the LUN field.)
> 
> 3) Remove the code entirely, because it is no longer needed / useful (?)
> 

My preference would be to remove it like Bart suggested. Maybe we should
make it configurable via a device attribute or backend module flag.

For the 2 users:

pscsi - It seems this code was there from the beginning via
transport_generic_prepare_cdb in the original commit. Nick must have
found some initiator where the workaround was needed and I am not sure
if we would break them or not now. Based on the original code's comment
about iscsi my guess is there was some misc iscsi initiator probably in
a boot firmware or some offload card that supported old commands.

tcmu - We want the raw cdb. I think masking out the above bits was an
accident. It looks like when Andy converted tcmu to use common code the
behavior got brought in for tcmu.

Re: Wrong resetting of Logical Unit Number field in CDB

[Bodo Stroesser]

On 10/10/19 05:38, Mike Christie wrote:
> On 10/08/2019 03:20 PM, bodo.stroesser@ts.fujitsu.com wrote:
>> Hi,
>>
>> We use TCMUser to run userspace tape and changer emulations.
>>
>> Current tests with a new version of backup SW failed, due to that application
>> using SECURITY PROTOCOL IN / OUT  SCSI commands.
>> The CDBs of these commands in byte 1 contain relevant information that
>> is overwritten in passthrough_parse_cdb() / target_core_device.c
>>
>> This is the related part of the code:
>>
>>          /*
>>           * Clear a lun set in the cdb if the initiator talking to use spoke
>>           * and old standards version, as we can't assume the underlying device
>>           * won't choke up on it.
>>           */
>>          switch (cdb[0]) {
>>          case READ_10: /* SBC - RDProtect */
>>          case READ_12: /* SBC - RDProtect */
>>          case READ_16: /* SBC - RDProtect */
>>          case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
>>          case VERIFY: /* SBC - VRProtect */
>>          case VERIFY_16: /* SBC - VRProtect */
>>          case WRITE_VERIFY: /* SBC - VRProtect */
>>          case WRITE_VERIFY_12: /* SBC - VRProtect */
>>          case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
>>                  break;
>>          default:
>>                  cdb[1] &= 0x1f; /* clear logical unit number */
>>                  break;
>>          }
>>
>> Obviously the list of command codes for which bits 5,6,7 of byte 1 are _not_ reset
>> is not complete. Now I'm wondering what would be the right fix:
>>
>> 1) Scan the SCSI specs and add all other missing command codes to the list of
>>     Codes to skip
>>
>> 2) Create a list of commands that definitely have the LUN field in byte 1 and
>>     reset the bits only in those. (Might be better than 1), because I expect new
>>     commands to not have the LUN field.)
>>
>> 3) Remove the code entirely, because it is no longer needed / useful (?)
>>
> 
> My preference would be to remove it like Bart suggested. Maybe we should
> make it configurable via a device attribute or backend module flag.
> 
> For the 2 users:
> 
> pscsi - It seems this code was there from the beginning via
> transport_generic_prepare_cdb in the original commit. Nick must have
> found some initiator where the workaround was needed and I am not sure
> if we would break them or not now. Based on the original code's comment
> about iscsi my guess is there was some misc iscsi initiator probably in
> a boot firmware or some offload card that supported old commands.
> 
> tcmu - We want the raw cdb. I think masking out the above bits was an
> accident. It looks like when Andy converted tcmu to use common code the
> behavior got brought in for tcmu.
> 
I think, regarding TCMU you both are right, we should skip the code.
To avoid the need for an attribute, I'd prefer a new flag in the backend
ops. So pscsi can stay unchanged for the moment.

If you agree, I would prepare a patch.

Re: Wrong resetting of Logical Unit Number field in CDB

[Bart Van Assche]

On 10/10/19 5:07 AM, Bodo Stroesser wrote:
> On 10/10/19 05:38, Mike Christie wrote:
>> On 10/08/2019 03:20 PM, bodo.stroesser@ts.fujitsu.com wrote:
>>> Hi,
>>>
>>> We use TCMUser to run userspace tape and changer emulations.
>>>
>>> Current tests with a new version of backup SW failed, due to that 
>>> application
>>> using SECURITY PROTOCOL IN / OUT  SCSI commands.
>>> The CDBs of these commands in byte 1 contain relevant information that
>>> is overwritten in passthrough_parse_cdb() / target_core_device.c
>>>
>>> This is the related part of the code:
>>>
>>>          /*
>>>           * Clear a lun set in the cdb if the initiator talking to 
>>> use spoke
>>>           * and old standards version, as we can't assume the 
>>> underlying device
>>>           * won't choke up on it.
>>>           */
>>>          switch (cdb[0]) {
>>>          case READ_10: /* SBC - RDProtect */
>>>          case READ_12: /* SBC - RDProtect */
>>>          case READ_16: /* SBC - RDProtect */
>>>          case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
>>>          case VERIFY: /* SBC - VRProtect */
>>>          case VERIFY_16: /* SBC - VRProtect */
>>>          case WRITE_VERIFY: /* SBC - VRProtect */
>>>          case WRITE_VERIFY_12: /* SBC - VRProtect */
>>>          case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA 
>>> RTPG */
>>>                  break;
>>>          default:
>>>                  cdb[1] &= 0x1f; /* clear logical unit number */
>>>                  break;
>>>          }
>>>
>>> Obviously the list of command codes for which bits 5,6,7 of byte 1 
>>> are _not_ reset
>>> is not complete. Now I'm wondering what would be the right fix:
>>>
>>> 1) Scan the SCSI specs and add all other missing command codes to the 
>>> list of
>>>     Codes to skip
>>>
>>> 2) Create a list of commands that definitely have the LUN field in 
>>> byte 1 and
>>>     reset the bits only in those. (Might be better than 1), because I 
>>> expect new
>>>     commands to not have the LUN field.)
>>>
>>> 3) Remove the code entirely, because it is no longer needed / useful (?)
>>>
>>
>> My preference would be to remove it like Bart suggested. Maybe we should
>> make it configurable via a device attribute or backend module flag.
>>
>> For the 2 users:
>>
>> pscsi - It seems this code was there from the beginning via
>> transport_generic_prepare_cdb in the original commit. Nick must have
>> found some initiator where the workaround was needed and I am not sure
>> if we would break them or not now. Based on the original code's comment
>> about iscsi my guess is there was some misc iscsi initiator probably in
>> a boot firmware or some offload card that supported old commands.
>>
>> tcmu - We want the raw cdb. I think masking out the above bits was an
>> accident. It looks like when Andy converted tcmu to use common code the
>> behavior got brought in for tcmu.
>>
> I think, regarding TCMU you both are right, we should skip the code.
> To avoid the need for an attribute, I'd prefer a new flag in the backend
> ops. So pscsi can stay unchanged for the moment.
> 
> If you agree, I would prepare a patch.

SCSI-2 was introduced in 1994 and SCSI-3 was introduced in 1996 
according to https://en.wikipedia.org/wiki/Parallel_SCSI. Is embedding 
the LUN in byte one of the CDB something that is only relevant for 
SCSI-2 parallel adapters? Since the SCSI target code supports receiving 
SCSI commands over iSCSI, FC and SRP but not through a parallel port, 
can the SCSI target code ever receive a CDB with a LUN number in byte 
one of the CDB?

Thanks,

Bart.

Re: Wrong resetting of Logical Unit Number field in CDB

[Bodo Stroesser]

On 10/10/19 17:41, Bart Van Assche wrote:
> On 10/10/19 5:07 AM, Bodo Stroesser wrote:
>> On 10/10/19 05:38, Mike Christie wrote:
>>> On 10/08/2019 03:20 PM, bodo.stroesser@ts.fujitsu.com wrote:
>>>> Hi,
>>>>
>>>> We use TCMUser to run userspace tape and changer emulations.
>>>>
>>>> Current tests with a new version of backup SW failed, due to that 
>>>> application
>>>> using SECURITY PROTOCOL IN / OUT  SCSI commands.
>>>> The CDBs of these commands in byte 1 contain relevant information that
>>>> is overwritten in passthrough_parse_cdb() / target_core_device.c
>>>>
>>>> This is the related part of the code:
>>>>
>>>>          /*
>>>>           * Clear a lun set in the cdb if the initiator talking to 
>>>> use spoke
>>>>           * and old standards version, as we can't assume the 
>>>> underlying device
>>>>           * won't choke up on it.
>>>>           */
>>>>          switch (cdb[0]) {
>>>>          case READ_10: /* SBC - RDProtect */
>>>>          case READ_12: /* SBC - RDProtect */
>>>>          case READ_16: /* SBC - RDProtect */
>>>>          case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
>>>>          case VERIFY: /* SBC - VRProtect */
>>>>          case VERIFY_16: /* SBC - VRProtect */
>>>>          case WRITE_VERIFY: /* SBC - VRProtect */
>>>>          case WRITE_VERIFY_12: /* SBC - VRProtect */
>>>>          case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA 
>>>> RTPG */
>>>>                  break;
>>>>          default:
>>>>                  cdb[1] &= 0x1f; /* clear logical unit number */
>>>>                  break;
>>>>          }
>>>>
>>>> Obviously the list of command codes for which bits 5,6,7 of byte 1 
>>>> are _not_ reset
>>>> is not complete. Now I'm wondering what would be the right fix:
>>>>
>>>> 1) Scan the SCSI specs and add all other missing command codes to 
>>>> the list of
>>>>     Codes to skip
>>>>
>>>> 2) Create a list of commands that definitely have the LUN field in 
>>>> byte 1 and
>>>>     reset the bits only in those. (Might be better than 1), because 
>>>> I expect new
>>>>     commands to not have the LUN field.)
>>>>
>>>> 3) Remove the code entirely, because it is no longer needed / useful 
>>>> (?)
>>>>
>>>
>>> My preference would be to remove it like Bart suggested. Maybe we should
>>> make it configurable via a device attribute or backend module flag.
>>>
>>> For the 2 users:
>>>
>>> pscsi - It seems this code was there from the beginning via
>>> transport_generic_prepare_cdb in the original commit. Nick must have
>>> found some initiator where the workaround was needed and I am not sure
>>> if we would break them or not now. Based on the original code's comment
>>> about iscsi my guess is there was some misc iscsi initiator probably in
>>> a boot firmware or some offload card that supported old commands.
>>>
>>> tcmu - We want the raw cdb. I think masking out the above bits was an
>>> accident. It looks like when Andy converted tcmu to use common code the
>>> behavior got brought in for tcmu.
>>>
>> I think, regarding TCMU you both are right, we should skip the code.
>> To avoid the need for an attribute, I'd prefer a new flag in the backend
>> ops. So pscsi can stay unchanged for the moment.
>>
>> If you agree, I would prepare a patch.
> 
> SCSI-2 was introduced in 1994 and SCSI-3 was introduced in 1996 
> according to https://en.wikipedia.org/wiki/Parallel_SCSI. Is embedding 
> the LUN in byte one of the CDB something that is only relevant for 
> SCSI-2 parallel adapters? Since the SCSI target code supports receiving 
> SCSI commands over iSCSI, FC and SRP but not through a parallel port, 
> can the SCSI target code ever receive a CDB with a LUN number in byte 
> one of the CDB?
Hmm. You are right. Ideally only SCSI-2 compliant initiators should
use the LUN field and they should run parallel SCSI only.

OTOH, like Mike already said, we can't know whether there is any SW, FW,
BIOS, ... out there, that still sends such old style CDBs.

For example: probably SW could send such CDBs by simply using SCSI
generic device on top of a modern initiator. (I hope that's true, I
didn't test ...)
That means, old code can produce old SCSI CDBs even when running
on top of modern HW.

Do we want to take the risk of breaking such "old stuff"?

Thank you,
Bodo

> 
> Thanks,
> 
> Bart.

Re: Wrong resetting of Logical Unit Number field in CDB

[Bart Van Assche]

On 10/10/19 11:57 AM, Bodo Stroesser wrote:
> Hmm. You are right. Ideally only SCSI-2 compliant initiators should
> use the LUN field and they should run parallel SCSI only.
> 
> OTOH, like Mike already said, we can't know whether there is any SW, FW,
> BIOS, ... out there, that still sends such old style CDBs.
> 
> For example: probably SW could send such CDBs by simply using SCSI
> generic device on top of a modern initiator. (I hope that's true, I
> didn't test ...)
> That means, old code can produce old SCSI CDBs even when running
> on top of modern HW.
> 
> Do we want to take the risk of breaking such "old stuff"?

Is blindly filtering out the LUN number correct? All initiator code that 
I found and that touches the highest three bits of that byte sets the 
LUN number in that byte. From the SCSI error handler:

if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
	scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f)|(sdev->lun << 5 & 0xe0);

Bart.

Re: Wrong resetting of Logical Unit Number field in CDB

[Mike Christie]

On 10/10/2019 10:41 AM, Bart Van Assche wrote:
> On 10/10/19 5:07 AM, Bodo Stroesser wrote:
>> On 10/10/19 05:38, Mike Christie wrote:
>>> On 10/08/2019 03:20 PM, bodo.stroesser@ts.fujitsu.com wrote:
>>>> Hi,
>>>>
>>>> We use TCMUser to run userspace tape and changer emulations.
>>>>
>>>> Current tests with a new version of backup SW failed, due to that
>>>> application
>>>> using SECURITY PROTOCOL IN / OUT  SCSI commands.
>>>> The CDBs of these commands in byte 1 contain relevant information that
>>>> is overwritten in passthrough_parse_cdb() / target_core_device.c
>>>>
>>>> This is the related part of the code:
>>>>
>>>>          /*
>>>>           * Clear a lun set in the cdb if the initiator talking to
>>>> use spoke
>>>>           * and old standards version, as we can't assume the
>>>> underlying device
>>>>           * won't choke up on it.
>>>>           */
>>>>          switch (cdb[0]) {
>>>>          case READ_10: /* SBC - RDProtect */
>>>>          case READ_12: /* SBC - RDProtect */
>>>>          case READ_16: /* SBC - RDProtect */
>>>>          case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
>>>>          case VERIFY: /* SBC - VRProtect */
>>>>          case VERIFY_16: /* SBC - VRProtect */
>>>>          case WRITE_VERIFY: /* SBC - VRProtect */
>>>>          case WRITE_VERIFY_12: /* SBC - VRProtect */
>>>>          case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA
>>>> RTPG */
>>>>                  break;
>>>>          default:
>>>>                  cdb[1] &= 0x1f; /* clear logical unit number */
>>>>                  break;
>>>>          }
>>>>
>>>> Obviously the list of command codes for which bits 5,6,7 of byte 1
>>>> are _not_ reset
>>>> is not complete. Now I'm wondering what would be the right fix:
>>>>
>>>> 1) Scan the SCSI specs and add all other missing command codes to
>>>> the list of
>>>>     Codes to skip
>>>>
>>>> 2) Create a list of commands that definitely have the LUN field in
>>>> byte 1 and
>>>>     reset the bits only in those. (Might be better than 1), because
>>>> I expect new
>>>>     commands to not have the LUN field.)
>>>>
>>>> 3) Remove the code entirely, because it is no longer needed / useful
>>>> (?)
>>>>
>>>
>>> My preference would be to remove it like Bart suggested. Maybe we should
>>> make it configurable via a device attribute or backend module flag.
>>>
>>> For the 2 users:
>>>
>>> pscsi - It seems this code was there from the beginning via
>>> transport_generic_prepare_cdb in the original commit. Nick must have
>>> found some initiator where the workaround was needed and I am not sure
>>> if we would break them or not now. Based on the original code's comment
>>> about iscsi my guess is there was some misc iscsi initiator probably in
>>> a boot firmware or some offload card that supported old commands.
>>>
>>> tcmu - We want the raw cdb. I think masking out the above bits was an
>>> accident. It looks like when Andy converted tcmu to use common code the
>>> behavior got brought in for tcmu.
>>>
>> I think, regarding TCMU you both are right, we should skip the code.
>> To avoid the need for an attribute, I'd prefer a new flag in the backend
>> ops. So pscsi can stay unchanged for the moment.
>>
>> If you agree, I would prepare a patch.
> 
> SCSI-2 was introduced in 1994 and SCSI-3 was introduced in 1996
> according to https://en.wikipedia.org/wiki/Parallel_SCSI. Is embedding
> the LUN in byte one of the CDB something that is only relevant for
> SCSI-2 parallel adapters? Since the SCSI target code supports receiving
> SCSI commands over iSCSI, FC and SRP but not through a parallel port,
> can the SCSI target code ever receive a CDB with a LUN number in byte
> one of the CDB?
> 

The code's original said it was for a iscsi issue:


/*      transport_generic_prepare_cdb():
 *
 *      Since the Initiator sees iSCSI devices as LUNs,  the SCSI CDB will
 *      contain the iSCSI LUN in bits 7-5 of byte 1 as per SAM-2.
 *      The point of this is since we are mapping iSCSI LUNs to
 *      SCSI Target IDs having a non-zero LUN in the CDB will throw the
 *      devices and HBAs for a loop.
 */

Re: Wrong resetting of Logical Unit Number field in CDB

[Mike Christie]

On 10/10/2019 03:14 PM, Bart Van Assche wrote:
> On 10/10/19 11:57 AM, Bodo Stroesser wrote:
>> Hmm. You are right. Ideally only SCSI-2 compliant initiators should
>> use the LUN field and they should run parallel SCSI only.
>>
>> OTOH, like Mike already said, we can't know whether there is any SW, FW,
>> BIOS, ... out there, that still sends such old style CDBs.
>>
>> For example: probably SW could send such CDBs by simply using SCSI
>> generic device on top of a modern initiator. (I hope that's true, I
>> didn't test ...)
>> That means, old code can produce old SCSI CDBs even when running
>> on top of modern HW.
>>
>> Do we want to take the risk of breaking such "old stuff"?
> 
> Is blindly filtering out the LUN number correct? All initiator code that

I have no idea about other details other than the code comments. I think
in general that code is wrong:

1. The original comment mentions iscsi and SAM2 but I think the SBC,
SPC, etc versions iscsi supported no longer supported commands that had
the LUN in those bits.

2. If we got one of these old commands and we clear the LUN, then we
have LUN=0 in that field, but the physical (not the lio level hba struct
but the drivers/scsi one) HBA/driver for the physical device might have
the physical device at LUN != 0, so I would think firmware might have
had issues with that.

3. It does not make sense to me why that list is so incomplete. I do not
understand why only those commands are in that list and not others.


> I found and that touches the highest three bits of that byte sets the
> LUN number in that byte. From the SCSI error handler:
> 
> if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
>     scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f)|(sdev->lun << 5 & 0xe0);
> 
> Bart.

Re: Wrong resetting of Logical Unit Number field in CDB

[Hannes Reinecke]

On 10/10/19 11:18 PM, Mike Christie wrote:
> On 10/10/2019 03:14 PM, Bart Van Assche wrote:
>> On 10/10/19 11:57 AM, Bodo Stroesser wrote:
>>> Hmm. You are right. Ideally only SCSI-2 compliant initiators should
>>> use the LUN field and they should run parallel SCSI only.
>>>
>>> OTOH, like Mike already said, we can't know whether there is any SW, FW,
>>> BIOS, ... out there, that still sends such old style CDBs.
>>>
>>> For example: probably SW could send such CDBs by simply using SCSI
>>> generic device on top of a modern initiator. (I hope that's true, I
>>> didn't test ...)
>>> That means, old code can produce old SCSI CDBs even when running
>>> on top of modern HW.
>>>
>>> Do we want to take the risk of breaking such "old stuff"?
>>
>> Is blindly filtering out the LUN number correct? All initiator code that
> 
> I have no idea about other details other than the code comments. I think
> in general that code is wrong:
> 
> 1. The original comment mentions iscsi and SAM2 but I think the SBC,
> SPC, etc versions iscsi supported no longer supported commands that had
> the LUN in those bits.
> 
> 2. If we got one of these old commands and we clear the LUN, then we
> have LUN=0 in that field, but the physical (not the lio level hba struct
> but the drivers/scsi one) HBA/driver for the physical device might have
> the physical device at LUN != 0, so I would think firmware might have
> had issues with that.
> 
> 3. It does not make sense to me why that list is so incomplete. I do not
> understand why only those commands are in that list and not others.
> 
> 
The iSCSI rfc is dated from 2004, whereas SPC-2 is dated 2001.
And SPC-2 makes no reference to the embedded LUN.
So one can safely assume there won't be any iSCSI devices embedding LUN
numbers in CDBs.
So by all intents and purposes we will only face this issue if we were
using pscsi to talk to old SCSI-2 devices.

I'd say strip it in the general case, and delegate it to pscsi if there
is a need.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: Wrong resetting of Logical Unit Number field in CDB

[Bodo Stroesser]

On 10/11/19 08:09, Hannes Reinecke wrote:
> On 10/10/19 11:18 PM, Mike Christie wrote:
>> On 10/10/2019 03:14 PM, Bart Van Assche wrote:
>>> On 10/10/19 11:57 AM, Bodo Stroesser wrote:
>>>> Hmm. You are right. Ideally only SCSI-2 compliant initiators should
>>>> use the LUN field and they should run parallel SCSI only.
>>>>
>>>> OTOH, like Mike already said, we can't know whether there is any SW, FW,
>>>> BIOS, ... out there, that still sends such old style CDBs.
>>>>
>>>> For example: probably SW could send such CDBs by simply using SCSI
>>>> generic device on top of a modern initiator. (I hope that's true, I
>>>> didn't test ...)
>>>> That means, old code can produce old SCSI CDBs even when running
>>>> on top of modern HW.
>>>>
>>>> Do we want to take the risk of breaking such "old stuff"?
>>>
>>> Is blindly filtering out the LUN number correct? All initiator code that
>>
>> I have no idea about other details other than the code comments. I think
>> in general that code is wrong:
>>
>> 1. The original comment mentions iscsi and SAM2 but I think the SBC,
>> SPC, etc versions iscsi supported no longer supported commands that had
>> the LUN in those bits.
>>
>> 2. If we got one of these old commands and we clear the LUN, then we
>> have LUN=0 in that field, but the physical (not the lio level hba struct
>> but the drivers/scsi one) HBA/driver for the physical device might have
>> the physical device at LUN != 0, so I would think firmware might have
>> had issues with that.
>>
>> 3. It does not make sense to me why that list is so incomplete. I do not
>> understand why only those commands are in that list and not others.
>>
>>
> The iSCSI rfc is dated from 2004, whereas SPC-2 is dated 2001.
> And SPC-2 makes no reference to the embedded LUN.
> So one can safely assume there won't be any iSCSI devices embedding LUN
> numbers in CDBs.
> So by all intents and purposes we will only face this issue if we were
> using pscsi to talk to old SCSI-2 devices.
> 
> I'd say strip it in the general case, and delegate it to pscsi if there
> is a need.

If I understood correctly, we all would prefer to make TCMU completely
transparent regarding the CDB.

For pscsi I think we all agree, that the code is wrong or incomplete.
But for pscsi up to now no one complained. So I'm wondering whether we
should spend much effort for pscsi to discuss / find out the right
solution? Especially as the original comment in the code, as posted by
Mike, as well as the current comment are not very clear. Thus 'fixing'
pscsi would mean to change or remove something we don't understand
completely.

So I agree to Hannes: we should simply move that code from
passthrough_parse_cdb() to pscsi_parse_cdb(), at least as a first step.

Regards,
Bodo

> 
> Cheers,
> 
> Hannes
> 

Re: Wrong resetting of Logical Unit Number field in CDB

[Bart Van Assche]

On 10/11/19 3:12 AM, Bodo Stroesser wrote:
> If I understood correctly, we all would prefer to make TCMU completely
> transparent regarding the CDB.
> 
> For pscsi I think we all agree, that the code is wrong or incomplete.
> But for pscsi up to now no one complained. So I'm wondering whether we
> should spend much effort for pscsi to discuss / find out the right
> solution? Especially as the original comment in the code, as posted by
> Mike, as well as the current comment are not very clear. Thus 'fixing'
> pscsi would mean to change or remove something we don't understand
> completely.
> 
> So I agree to Hannes: we should simply move that code from
> passthrough_parse_cdb() to pscsi_parse_cdb(), at least as a first step.

Hannes wrote "[ ... ] delegate it to pscsi if there is a need." Is there 
really a need for such code in the SCSI passthrough driver? The upstream 
SCSI target code is one of the four Linux SCSI target stacks that I know 
of. I haven't found any SCSI-2 LUN number filtering code in the tgt 
project (http://stgt.sourceforge.net/). I'm not sure about IET. SCST is 
around since 2006 and has SCSI-2 LUN number filtering code in its CDROM 
and MODISK passthrough drivers. That LUN number filtering code is there 
since the initial commit so it is not the result of a recent request of 
a user. SCST does not have LUN filtering code in its disk passthrough 
driver nor in its tape passthrough driver. In other words, I do not 
expect that anyone's setup will be broken by removing 
passthrough_parse_cdb() entirely.

Bart.

Re: Wrong resetting of Logical Unit Number field in CDB

[Mike Christie]

On 10/11/2019 12:13 PM, Bart Van Assche wrote:
> On 10/11/19 3:12 AM, Bodo Stroesser wrote:
>> If I understood correctly, we all would prefer to make TCMU completely
>> transparent regarding the CDB.
>>
>> For pscsi I think we all agree, that the code is wrong or incomplete.
>> But for pscsi up to now no one complained. So I'm wondering whether we
>> should spend much effort for pscsi to discuss / find out the right
>> solution? Especially as the original comment in the code, as posted by
>> Mike, as well as the current comment are not very clear. Thus 'fixing'
>> pscsi would mean to change or remove something we don't understand
>> completely.
>>
>> So I agree to Hannes: we should simply move that code from
>> passthrough_parse_cdb() to pscsi_parse_cdb(), at least as a first step.
> 
> Hannes wrote "[ ... ] delegate it to pscsi if there is a need." Is there
> really a need for such code in the SCSI passthrough driver? The upstream
> SCSI target code is one of the four Linux SCSI target stacks that I know
> of. I haven't found any SCSI-2 LUN number filtering code in the tgt
> project (http://stgt.sourceforge.net/). I'm not sure about IET. SCST is
> around since 2006 and has SCSI-2 LUN number filtering code in its CDROM
> and MODISK passthrough drivers. That LUN number filtering code is there
> since the initial commit so it is not the result of a recent request of
> a user. SCST does not have LUN filtering code in its disk passthrough
> driver nor in its tape passthrough driver. In other words, I do not
> expect that anyone's setup will be broken by removing
> passthrough_parse_cdb() entirely.

I originally thought it was for a really specific case Nick hit a long
time ago because the code is so odd. However, the original code's
comment sounds like Nick might have read the specs incorrectly or was
working off a older iscsi rfc draft. I think this happened in some
persistent reservation code that is wrong too.

I think if the code it was for a specific initiator then he would have
added that in the code comments. For example for a while we had a really
specific comment about the mac os initiator or qlogic on windows or
something, and there is still a comment about solaris. If on the other
hand it was a spec thing then it seems he writes the scsi spec name like
he did in the original comment.

Re: Wrong resetting of Logical Unit Number field in CDB

[Hannes Reinecke]

On 10/11/19 7:13 PM, Bart Van Assche wrote:
> On 10/11/19 3:12 AM, Bodo Stroesser wrote:
>> If I understood correctly, we all would prefer to make TCMU completely
>> transparent regarding the CDB.
>>
>> For pscsi I think we all agree, that the code is wrong or incomplete.
>> But for pscsi up to now no one complained. So I'm wondering whether we
>> should spend much effort for pscsi to discuss / find out the right
>> solution? Especially as the original comment in the code, as posted by
>> Mike, as well as the current comment are not very clear. Thus 'fixing'
>> pscsi would mean to change or remove something we don't understand
>> completely.
>>
>> So I agree to Hannes: we should simply move that code from
>> passthrough_parse_cdb() to pscsi_parse_cdb(), at least as a first step.
> 
> Hannes wrote "[ ... ] delegate it to pscsi if there is a need." Is there
> really a need for such code in the SCSI passthrough driver? The upstream
> SCSI target code is one of the four Linux SCSI target stacks that I know
> of. I haven't found any SCSI-2 LUN number filtering code in the tgt
> project (http://stgt.sourceforge.net/). I'm not sure about IET. SCST is
> around since 2006 and has SCSI-2 LUN number filtering code in its CDROM
> and MODISK passthrough drivers. That LUN number filtering code is there
> since the initial commit so it is not the result of a recent request of
> a user. SCST does not have LUN filtering code in its disk passthrough
> driver nor in its tape passthrough driver. In other words, I do not
> expect that anyone's setup will be broken by removing
> passthrough_parse_cdb() entirely.
> 
I guess Bart is right.
We should just drop it and see if someone's complaining.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer