Re: [PATCH] scsi: use ATA-12 pass-thru for OPAL as fallback

[Damien Le Moal <dlemoal@kernel.org>]

On 10/16/23 21:46, Milan Broz wrote:
> On 10/16/23 13:54, Damien Le Moal wrote:
>> On 10/16/23 16:24, Milan Broz wrote:
>>> On 10/16/23 09:05, Christoph Hellwig wrote:
>>>> On Mon, Oct 16, 2023 at 09:02:11AM +0200, Milan Broz wrote:
>>>>> All common USB/SATA or USB/NVMe adapters I tested need this patch.
>>>>>
>>>>> In short, these steps are run for OPAL support check:
>>>>>     1) Storage driver enables security driver flag (security_supported).
>>>>>        USB-attached storage drivers will enable it in a separate patchset.
>>>>>        SCSI and NNVMe drivers do it already. If the flag is not enabled,
>>>>>        no following steps are run, and OPAL remains disabled.
>>>>>     2) SCSI device enumerates SECURITY IN/OUT command support. If detected,
>>>>>        SECURITY ON/OUT wrapper is used (as in the current code).
>>>>>        If not, new ATA-12 pass-thru wrapper is used instead.
>>>>>     3) SED OPAL code tries OPAL discovery command for the device.
>>>>>        If it receives a correct reply, OPAL is enabled for the device.
>>>>>        If SCSI SECURITY or ATA-12 command with discovery command is rejected,
>>>>>        OPAL remains disabled.
>>>>>
>>>>> Note, USB attached storage needs an additional patchset sent separately
>>>>> as requested by USB driver maintainers (it contains required changes
>>>>> related to USB quirk processing).
>>>>
>>>> This just feels wrong.  These adapters are broken if they can't
>>>> translated, and we should not put ATA command submission into
>>>> sd.c.
>>>
>>> I think it is blocked in USB layer as not running command enumeration,
>>> SCSI SECURITY will be never sent to the adapter through USB.
>>>
>>> I understand the problem, but if you configure OPAL from userspace, ATA-12 is sent
>>> to these devices already - so why kernel cannot use it too?
>>>
>>>>
>>>>> +	cdb[0] = ATA_12;
>>>>> +	cdb[1] = (send ? 5 /* ATA_PROTOCOL_PIO_DATA_IN */ : 4 /* ATA_PROTOCOL_PIO_DATA_OUT */) << 1;
>>>>> +	cdb[2] = 2 /* t_length */ | (1 << 2) /* byt_blok */ | ((send ?  0 : 1) << 3) /* t_dir */;
>>>>> +	cdb[3] = secp;
>>>>> +	put_unaligned_le16(len / 512, &cdb[4]);
>>>>> +	put_unaligned_le16(spsp, &cdb[6]);
>>>>> +	cdb[9] = send ? 0x5e /* ATA_CMD_TRUSTED_SND */: 0x5c /* ATA_CMD_TRUSTED_RCV */;
>>>>
>>>>
>>>> Also avoid all these crazy long lines, and please use the actual
>>>> constants.  Using a good old if/else is actually a very good way to
>>>> structure the code in a somewhat readable way.
>>>
>>> Sure, I was trying to no add additional includes that will mess this up, I'll reformat it if needed.
>>>
>>> Otherwise, this wrapper is exactly what is used is sedutils and also in our test utility
>>> that tries to work with OPAL commands directly
>>> https://github.com/mbroz/opal-toolset
>>>
>>>>
>>>>> +		if (sdkp->security)
>>>>> +		    sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit);
>>>>> +		else
>>>>> +		    sdkp->opal_dev = init_opal_dev(sdkp, &sd_ata12_submit);
>>>>
>>>> Messed up indentation here.
>>>
>>> sorry, my bad, I hate such formatting myself and missed it here :-)
>>>    
>>>> besides the fact that the statement is fundamentally wrong and you'll
>>>> start sending ATA command to random devices.
>>>
>>> So what do you suggest? As I said, this exactly happen if you configure it from userspace.
>>>
>>> Can this be somehow limited? I did not find and way how to do it.
>> The translation of SECURITY IN/OUT commands should go into usb uas.c, at the
>> very least. And even having it there is not great in my opinion. If the adapter
>> does not support Opal, don't use that feature, or use it only to lock/unlock the
>> drive from user space with passthrough.
> 
> I was resisting to support OPAL hw for long time, but once we decided to add it as
> an additional (and optional) layer for LUKS, I would like it to be supported also
> for external drives (if technically possible).
> 
> The problem is that we (for simplicity) decided to use kernel SED-ioctl interface that
> internally wraps OPAL command to SCSI SECURITY command only. It means, that all devices
> that can use ATA-12 just cannot work with this kernel interface (unlike userspace which
> can decide which wrapper to use).
> 
> And IMO it is not correct - if it was designed only for some servers with directly connected
> devices, then it is really not generic OPAL support. It should work for any hw that supports it.
> 
> For USB, it actually works quite nice with the patch (ignoring usual bugs in firmware).
> 
>>
>> Note that nowhere in your patch do you test if you are talking to an ATA device.
> 
> Yes, I know. I expected the command to be rejected if not supported.
> 
>> This can be done by testing for the existence of VPD page 89h. See
>> scsi_cdl_enable() in drivers/scsi/scsi.c for an example where we had to check
>> for that. But also note that we do not issue ATA commands based on that test. We
>> keep issuing SCSI commands and libata takes care of the translation. uas does
>> not use libata though, so if translation is needed, do it there.
> 
> So, you mean translate SCSI SECURITY to ATA-12 inside USB storage drivers?
> 
> (There are actually two places, UAS driver and then SCSI glue for mass-storage -
> unfortunately, we need both.)

In the generic uas code, yes. sd is scsi-disk driver. That has no business
issuing ATA commands with passthrough. The UAS based translation would be like a
complement to the adapter SAT layer, which is the one not supporting OPAL in the
first place.

>> But I have the same opinion as Christoph: working around USB adapters lack of
>> support for a feature with passthrough commands issued from the kernel is really
>> not ideal.
> 
> Well, I have several adapters and many OPAL drives, none works with SCSI commands
> if connected through USB. Partially it is missing support in USB layer, but the rest
> is mess in hw. I know it is **** but that's how it is; people have these and want
> to use it (including myself).

Yes, USB mass storage adapters are generally dumb beyond help... Even technology
several years old are still not supported (e.g. host managed SMR).

> 
> IMO it is quite similar to discard/TRIM support...

I do not think so. These have well defined translations. There are a lot of
drives with buggy implementations of these commands though.

> 
> Thanks,
> Milan

-- 
Damien Le Moal
Western Digital Research