[PATCH] nvmet-passthru: allow nvme ns-id descriptor list to clear EUID/NGUID/UUID

[PATCH] nvmet-passthru: allow nvme ns-id descriptor list to clear EUID/NGUID/UUID

[Nilay Shroff]

When nvme passthru is configured using loop target, the clear_ids
attribute is, by default, set to true. This attribute would ensure that
EUID/NGUID/UUID is cleared for the loop passthru target.

The newer NVMe disk supporting the NVMe spec 1.3 or higher, typically,
implements the support for "Namespace Identification Descriptor list"
command. This command when issued from host returns EUID/NGUID/UUID
assigned to the inquired namespace. Not clearing these values, while
using nvme passthru using loop target, would result in NVMe host driver
rejecting the namespace. This check was implemented in the commit
2079f41ec6ff ("nvme: check that EUI/GUID/UUID are globally unique").

The fix implemented in this commit ensure that when host issues ns-id
descriptor list command, the EUID/NGUID/UUID are cleared by passthru
target. In fact, the function nvmet_passthru_override_id_descs() which
clears those unique ids already exits, so we just need to ensure that
ns-id descriptor list command falls through the corretc code path. And
while we're at it, we also combines the three passthru admin command
cases together which shares the same code.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/passthru.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 24d0e2418d2e..d786d8dee131 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -535,10 +535,6 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 		break;
 	case nvme_admin_identify:
 		switch (req->cmd->identify.cns) {
-		case NVME_ID_CNS_CTRL:
-			req->execute = nvmet_passthru_execute_cmd;
-			req->p.use_workqueue = true;
-			return NVME_SC_SUCCESS;
 		case NVME_ID_CNS_CS_CTRL:
 			switch (req->cmd->identify.csi) {
 			case NVME_CSI_ZNS:
@@ -547,7 +543,11 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 				return NVME_SC_SUCCESS;
 			}
 			return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
+		case NVME_ID_CNS_CTRL:
+			fallthrough;
 		case NVME_ID_CNS_NS:
+			fallthrough;
+		case NVME_ID_CNS_NS_DESC_LIST:
 			req->execute = nvmet_passthru_execute_cmd;
 			req->p.use_workqueue = true;
 			return NVME_SC_SUCCESS;
-- 
2.45.2

Re: [PATCH] nvmet-passthru: allow nvme ns-id descriptor list to clear EUID/NGUID/UUID

[Nilay Shroff]

On 9/17/24 14:21, Nilay Shroff wrote:
> When nvme passthru is configured using loop target, the clear_ids
> attribute is, by default, set to true. This attribute would ensure that
> EUID/NGUID/UUID is cleared for the loop passthru target.
> 
> The newer NVMe disk supporting the NVMe spec 1.3 or higher, typically,
> implements the support for "Namespace Identification Descriptor list"
> command. This command when issued from host returns EUID/NGUID/UUID
> assigned to the inquired namespace. Not clearing these values, while
> using nvme passthru using loop target, would result in NVMe host driver
> rejecting the namespace. This check was implemented in the commit
> 2079f41ec6ff ("nvme: check that EUI/GUID/UUID are globally unique").
> 
> The fix implemented in this commit ensure that when host issues ns-id
> descriptor list command, the EUID/NGUID/UUID are cleared by passthru
> target. In fact, the function nvmet_passthru_override_id_descs() which
> clears those unique ids already exits, so we just need to ensure that
> ns-id descriptor list command falls through the corretc code path. And
> while we're at it, we also combines the three passthru admin command
> cases together which shares the same code.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  drivers/nvme/target/passthru.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index 24d0e2418d2e..d786d8dee131 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -535,10 +535,6 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
>  		break;
>  	case nvme_admin_identify:
>  		switch (req->cmd->identify.cns) {
> -		case NVME_ID_CNS_CTRL:
> -			req->execute = nvmet_passthru_execute_cmd;
> -			req->p.use_workqueue = true;
> -			return NVME_SC_SUCCESS;
>  		case NVME_ID_CNS_CS_CTRL:
>  			switch (req->cmd->identify.csi) {
>  			case NVME_CSI_ZNS:
> @@ -547,7 +543,11 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
>  				return NVME_SC_SUCCESS;
>  			}
>  			return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
> +		case NVME_ID_CNS_CTRL:
> +			fallthrough;
>  		case NVME_ID_CNS_NS:
> +			fallthrough;
> +		case NVME_ID_CNS_NS_DESC_LIST:
>  			req->execute = nvmet_passthru_execute_cmd;
>  			req->p.use_workqueue = true;
>  			return NVME_SC_SUCCESS;

On a sidenote: This issue could be reproduced (where we find passthru target namespeace being 
ignored on the host due to duplicate IDs) using blktest nvme/033. I have a system with NVMe disk
which supports NVMe spec 1.3. On this system, when I run this blktest I can always reproduce 
this issue:

# nvme list -v 
Subsystem        Subsystem-NQN                                                                                    Controllers
---------------- ------------------------------------------------------------------------------------------------ ----------------
nvme-subsys0     nqn.1994-11.com.samsung:nvme:PM1735:2.5-inch:S6EUNA0R500358                                      nvme0

Device   SN                   MN                                       FR       TxPort Asdress        Slot   Subsystem    Namespaces      
-------- -------------------- ---------------------------------------- -------- ------ -------------- ------ ------------ ----------------
nvme0    S6EUNA0R500358       1.6TB NVMe Gen4 U.2 SSD                  REV.SN49 pcie   0018:01:00.0          nvme-subsys0 nvme0n1

Device       Generic      NSID       Usage                      Format           Controllers     
------------ ------------ ---------- -------------------------- ---------------- ----------------
/dev/nvme0n1 /dev/ng0n1   0x1          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme0

# nvme show-regs /dev/nvme0 -H 
<snip>
version : 10300
	NVMe specification 1.3
<snip>

# ./check tests/nvme/033 
nvme/033 => nvme0n1 (tr=loop) (create and connect to an NVMeOF target with a passthru controller)

Here, the above blktest hangs indefinitely. 

And the relevant kernel logs:
# dmesg
nvmet: creating nvm controller 1 for subsystem blktests-subsystem-1 for NQN nqn.2014-08.org.nvmexpress:uuid:0f01fb42-9f7f-4856-b0b3-51e60b8de349.
nvme nvme1: D3 entry latency set to 10 seconds
nvme nvme1: creating 32 I/O queues.
nvme nvme1: new ctrl: "blktests-subsystem-1"
nvme nvme1: Failed to configure AEN (cfg 300)
nvme nvme1: VID:144d model:1.6TB NVMe Gen4 U.2 SSD firmware:REV.SN49
nvme nvme1: ignoring nsid 1 because of duplicate IDs

I have another system using an NVMe disk supporting NVMe spec 1.2 and on this system, running 
the above test would PASS.

Moreover, I think we should fix the blktest nvme/033 so that it would bail out 
after some time instead of hanging indefinitely. Also we don't have an nvme-cli 
command which supports ns-id descriptor list command. We may also want to add 
support of this new command in nvme-cli? 

Thanks,
--Nilay

Re: [PATCH] nvmet-passthru: allow nvme ns-id descriptor list to clear EUID/NGUID/UUID

[Christoph Hellwig]

Instead of saying allow I'd just clear 'clear ..'

Otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] nvmet-passthru: allow nvme ns-id descriptor list to clear EUID/NGUID/UUID

[Keith Busch]

On Tue, Sep 17, 2024 at 02:21:04PM +0530, Nilay Shroff wrote:
> @@ -547,7 +543,11 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
>  				return NVME_SC_SUCCESS;
>  			}
>  			return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
> +		case NVME_ID_CNS_CTRL:
> +			fallthrough;
>  		case NVME_ID_CNS_NS:
> +			fallthrough;
> +		case NVME_ID_CNS_NS_DESC_LIST:

I'm pretty sure you only need the "fallthrough" statement when you other
statements inbetween the cases.

Re: [PATCH] nvmet-passthru: allow nvme ns-id descriptor list to clear EUID/NGUID/UUID

[Nilay Shroff]

On 9/20/24 22:19, Keith Busch wrote:
> On Tue, Sep 17, 2024 at 02:21:04PM +0530, Nilay Shroff wrote:
>> @@ -547,7 +543,11 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
>>  				return NVME_SC_SUCCESS;
>>  			}
>>  			return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
>> +		case NVME_ID_CNS_CTRL:
>> +			fallthrough;
>>  		case NVME_ID_CNS_NS:
>> +			fallthrough;
>> +		case NVME_ID_CNS_NS_DESC_LIST:
> 
> I'm pretty sure you only need the "fallthrough" statement when you other
> statements inbetween the cases.
> 
Alright, I think you prefer combining all three cases together without any
fallthrough in-between them and yes I believe that would also work. I will 
send the next patch including yours as well Christoph review comment addressed.

Thank,
--Nilay