USB attached storage (handled by UAS or usb-storage driver) does not
support SCSI SECURITY IN/OUT commands. (USB adapters usually do not
support it, and USB drivers also turn off SCSI command enumeration).
The original sedutils (used to configure OPAL from userspace)
always used the ATA-12 pass-through command.
Internal kernel implementation for OPAL interface (sed-opal ioctls)
currently supports only SCSI SECURITY IN/OUT command wrapper.
This patch adds an optional wrapper to the SCSI layer for the ATA-12
pass-thru command as an alternative if SECURITY IN/OUT is unavailable.
The wrapper has the same structure as tools that talk to OPAL drives
through ioctl from userspace.
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).
Signed-off-by: Milan Broz <[email protected]>
---
drivers/scsi/sd.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 83b6a3f3863b..3782556cb461 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -686,6 +686,32 @@ static int sd_sec_submit(void *data, u16 spsp, u8 secp, void *buffer,
&exec_args);
return ret <= 0 ? ret : -EIO;
}
+
+static int sd_ata12_submit(void *data, u16 spsp, u8 secp, void *buffer,
+ size_t len, bool send)
+{
+ struct scsi_disk *sdkp = data;
+ struct scsi_device *sdev = sdkp->device;
+ u8 cdb[12] = { 0, };
+ const struct scsi_exec_args exec_args = {
+ .req_flags = BLK_MQ_REQ_PM,
+ };
+ int ret;
+
+ 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 */;
+
+ ret = scsi_execute_cmd(sdev, cdb, send ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
+ buffer, len, SD_TIMEOUT, sdkp->max_retries,
+ &exec_args);
+ return ret <= 0 ? ret : -EIO;
+}
+
#endif /* CONFIG_BLK_SED_OPAL */
/*
@@ -3699,8 +3725,11 @@ static int sd_probe(struct device *dev)
goto out;
}
- if (sdkp->security) {
- sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit);
+ if (sdp->security_supported) {
+ if (sdkp->security)
+ sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit);
+ else
+ sdkp->opal_dev = init_opal_dev(sdkp, &sd_ata12_submit);
if (sdkp->opal_dev)
sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
}
--
2.42.0
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.
> + 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.
> + 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.
besides the fact that the statement is fundamentally wrong and you'll
start sending ATA command to random devices.
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.
Milan
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.
Note that nowhere in your patch do you test if you are talking to an ATA device.
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.
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.
--
Damien Le Moal
Western Digital Research
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.)
> 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).
IMO it is quite similar to discard/TRIM support...
Thanks,
Milan
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
On Mon, Oct 16, 2023 at 09:24:16AM +0200, Milan Broz wrote:
> I think it is blocked in USB layer as not running command enumeration,
> SCSI SECURITY will be never sent to the adapter through USB.
What is blocked?
>> 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.
If the USB maintainers are fine with it add the translation to
usb-storage as that's where we deal with these quirky devices usually.
On Mon, Oct 16, 2023 at 02:46:03PM +0200, Milan Broz wrote:
> 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
No, it doesn't. It uses the properly specified protocol for each
layer. That is NVMe uses NVMe Security Send/Receive, SCSI uses the
SCSI protocol, and libata translats for ATA devices.
> that can use ATA-12 just cannot work with this kernel interface (unlike userspace which
> can decide which wrapper to use).
It supports all devices that actually speak ATA perfectly fine, take
a look at ata_scsi_security_inout_xlat.
>
> 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.
Let's get off your crack pipe before we continue. It is designed and
implemented to support the security protocols exactly as spec'ed.
You seem to have found devices that claim to be SCSI, but actually
require ATA passthrough for security. That's no secret cabal to lock
out non-server hardware but just proper protocol design.
> For USB, it actually works quite nice with the patch (ignoring usual bugs in firmware).
So move it into usb if you can convince the usb maintainers that they
are fine with it.
>
>>
>> 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.
Good luck. Cheap storage hardware trips up on unknown commands all the
time.
> IMO it is quite similar to discard/TRIM support...
Where we also don't support weird ATA commands directly from sd
for good reason.
On 10/16/23 15:27, Christoph Hellwig wrote:
> On Mon, Oct 16, 2023 at 02:46:03PM +0200, Milan Broz wrote:
>> 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
>
> No, it doesn't. It uses the properly specified protocol for each
> layer. That is NVMe uses NVMe Security Send/Receive, SCSI uses the
> SCSI protocol, and libata translats for ATA devices.
>
>> that can use ATA-12 just cannot work with this kernel interface (unlike userspace which
>> can decide which wrapper to use).
>
> It supports all devices that actually speak ATA perfectly fine, take
> a look at ata_scsi_security_inout_xlat.
Yes, I have several of them in my test machine. The comment was about (S)ATA connected
through USB bridge only.
>>
>> 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.
>
> Let's get off your crack pipe before we continue. It is designed and
> implemented to support the security protocols exactly as spec'ed.
>
> You seem to have found devices that claim to be SCSI, but actually
> require ATA passthrough for security. That's no secret cabal to lock
> out non-server hardware but just proper protocol design.
*grin* I just bought several NVMe to USB adapters that presents NVMe device as SCSI, this
is pretty common.
(And Thunderbolt adapters - that present NMVe as real NVMe is another story too.
But once configured, it is doing it correctly.)
But yes, you are right - except the USB hw is here (in huge quantities) and I want to use it.
It is quite possible that there is not way to do it clearly - fine, that's why I sent the patch
for review.
>
>> For USB, it actually works quite nice with the patch (ignoring usual bugs in firmware).
>
> So move it into usb if you can convince the usb maintainers that they
> are fine with it.
Yep, fair enough. My initial motivation was just understand WTF is going there.
Put the support on a proper layer is step #2.
>>> 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.
>
> Good luck. Cheap storage hardware trips up on unknown commands all the
> time.
... And my tests for TCG OPAL commands shows that it can be even worse on this layer :-)
(To be fair, recent NVMe devices looks much better. Anyway, yes, I know what you mean.)
>
>> IMO it is quite similar to discard/TRIM support...
>
> Where we also don't support weird ATA commands directly from sd
> for good reason.
ok, I am actually quite happy that I get some response to this patch.
Supporting it is a mess, but I still believe we can do it (if fw is not completely bogus).
Thanks,
Milan