Re: Wrong resetting of Logical Unit Number field in CDB

[Mike Christie <mchristi@redhat.com>]

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.
 */