Message ID | 20231016070211.39502-1-gmazyland@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp3281414vqb; Mon, 16 Oct 2023 00:02:42 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHzBeK3f+9I3CF8uZnrrqjmZQmcodg73/EIR1C8SikfiMhpM/bhEe9Tfxe6wNWCNrAIfbg9 X-Received: by 2002:a17:902:da87:b0:1c6:2902:24f9 with SMTP id j7-20020a170902da8700b001c6290224f9mr38378119plx.1.1697439762113; Mon, 16 Oct 2023 00:02:42 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697439762; cv=none; d=google.com; s=arc-20160816; b=LDmCdLznYUS0jLqgss34T16ML33qW8BwhWRKkhowhsRTeD8Z1b30knkYPj58dZb56Y J0IfjAL1ilDBOAVrj2mz6KSC73sloS/gR1hJW4og+5mI6fMZir6UcWXDtAs8HfBBBuvc YfpL95DntKX3+k7n97VTq9YuC6x0kg272t5bZOCzm81YCujIiGxW0OJ1c45S09mH57rb 8kKRxQfbyzAXK8+xwe963E850s8jxERhFbTTc4yY52n72exWzOii6JsVPETVFnNHC9eE cj9jOlWHDdXmn26SzYXQ29me5Hi7Cd4tiT8f7sP+LBZ5JxzTW1uX4BkXViyEKh6hAKXk Pq8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=JIhJBlbfwa6Zp2zLHD2ldrf9h2gYA0FwmEhnEtZ+bAA=; fh=myBpXdpjkIxHysyhYgJecrZMCTFj/MqYrBFXrDgc3Ko=; b=RdqK1yvBkIPHj7RI3RURztjevC1YouxcMbY5wsu/6CPQZp5rk9M4z7ScmhQ3EmSmyS Qhqop8+K/naODevuXkFCQxe2EKxYIX9pCqXPc1BUW9k2GJQs2KR/zdozCxYbyhNel4kr sbKItl2qReu53vx7BO1nku+04goVbRmbV/Til0UgIiOQ/VtWjWmhKF6kcA6vG7yN9iOV UYIXFEK9/uEY3xyz1fqPxRPWlboZNdJq8pg8sXYbkXRpKmiYOoqYD0hYM0j/CiSUVWY3 GKNd9CmUHmIiI6GfkoHualGmrO2SAlbZExjCepIt1obK6+juZJOmUtCk33AZPsgQgCvo KX2A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=MgXEtRK4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from howler.vger.email (howler.vger.email. [23.128.96.34]) by mx.google.com with ESMTPS id kg5-20020a170903060500b001c5bb1f0cccsi9372013plb.275.2023.10.16.00.02.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Oct 2023 00:02:42 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=MgXEtRK4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id E6ECB8055AD3; Mon, 16 Oct 2023 00:02:39 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231526AbjJPHCY (ORCPT <rfc822;hjfbswb@gmail.com> + 18 others); Mon, 16 Oct 2023 03:02:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46916 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229639AbjJPHCX (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 16 Oct 2023 03:02:23 -0400 Received: from mail-ej1-x629.google.com (mail-ej1-x629.google.com [IPv6:2a00:1450:4864:20::629]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 83975D9; Mon, 16 Oct 2023 00:02:21 -0700 (PDT) Received: by mail-ej1-x629.google.com with SMTP id a640c23a62f3a-98377c5d53eso632127766b.0; Mon, 16 Oct 2023 00:02:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697439740; x=1698044540; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=JIhJBlbfwa6Zp2zLHD2ldrf9h2gYA0FwmEhnEtZ+bAA=; b=MgXEtRK4mXbs+M0Z8vSkYzPI7bTAv+bs8lG8kNrSHR1IjueQH6IMrixUGB1y/eafF4 RlKBERIZmqCJ0bjOWpGY7PS0sZZojGX1BuWOn6Md+xUfOnhx2UhDk3fw+w1GmaLzdSze NIdcPncMnFRrKGN4xS1QcTHBiJd3SIHJ8J/n0y6bCnJ0jb0d9iEBCCIObefpqdZowNMM QPH+Mwn1uH7MygrXLHqT8jxJMzbSA230HQACXho8lCN/2hL6VBlV46wCkJuhMtbQGPKr TFvLBNDKojtreGlRxv1I9Cqkx6AkAsgrevAGTIJJuyQpXB+LNE4ENqSF/8fJB9inJUrt vuIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697439740; x=1698044540; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=JIhJBlbfwa6Zp2zLHD2ldrf9h2gYA0FwmEhnEtZ+bAA=; b=oAKV/2oWditLkt0jwtxEUjQvq2ATp2Y6KxL5CpjiqEDK4p4TndT5wbdgx9TqR85E9O NY9Qva1nUdEqqKPrvlPM1dewFKoRlIHfB2UGW0RGEjAdDkDSZ5J+OvOrUwDFEjY33mup vUWsQHPSFkR6Xql+RRWsfO/R1tZ+kfzHF3GgTCjbkdgT9CA8BtvVxhZaK1EovCxiBal9 X/GsxtqGufJw1/C4wFuuEvaUfAOaA0D1ZJxX4BtrOJiMq8H8EE/XJCzt0f0pFqEWNheq x5L6kyg/BiJcjlNKgLI/2SauvLNX2V8URNA3asYD196A+lz5NVnTR3tm2BvEZx2yLshF Vtag== X-Gm-Message-State: AOJu0Yw2+N1/uti18kZHVpwgGVGztCdcNWs4OKzy6Om5IoWWrLkcl9JM vhCn5GZCP+sHuyN/0BoPuAJXTgku8NQ= X-Received: by 2002:a17:907:7804:b0:9ad:9225:ced2 with SMTP id la4-20020a170907780400b009ad9225ced2mr27086794ejc.62.1697439739572; Mon, 16 Oct 2023 00:02:19 -0700 (PDT) Received: from sauvignon.fi.muni.cz ([2001:718:801:22c:bdcb:518:be8f:6a76]) by smtp.gmail.com with ESMTPSA id e16-20020a170906749000b0099bd0b5a2bcsm3447276ejl.101.2023.10.16.00.02.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Oct 2023 00:02:18 -0700 (PDT) From: Milan Broz <gmazyland@gmail.com> To: linux-scsi@vger.kernel.org Cc: jejb@linux.ibm.com, martin.petersen@oracle.com, hch@lst.de, linux-kernel@vger.kernel.org, Milan Broz <gmazyland@gmail.com> Subject: [PATCH] scsi: use ATA-12 pass-thru for OPAL as fallback Date: Mon, 16 Oct 2023 09:02:11 +0200 Message-ID: <20231016070211.39502-1-gmazyland@gmail.com> X-Mailer: git-send-email 2.42.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Mon, 16 Oct 2023 00:02:39 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779894596222892700 X-GMAIL-MSGID: 1779894596222892700 |
Series |
scsi: use ATA-12 pass-thru for OPAL as fallback
|
|
Commit Message
Milan Broz
Oct. 16, 2023, 7:02 a.m. UTC
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 <gmazyland@gmail.com>
---
drivers/scsi/sd.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
Comments
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.
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
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
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"); }