linux-kernel - Re: [PATCH] nvme: Force ns info updates on validation if NID is bogus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14b31e97-d67c-4dd2-949f-0666fb30eb54@suse.de>
Date: Tue, 10 Sep 2024 14:45:09 +0200
From: Hannes Reinecke <hare@...e.de>
To: Yihan Xin <xyh1996@...il.com>, Keith Busch <kbusch@...nel.org>,
 Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
 Sagi Grimberg <sagi@...mberg.me>
Cc: linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvme: Force ns info updates on validation if NID is bogus

On 9/10/24 11:50, Yihan Xin wrote:
> When validating a namespace, nvme_update_ns_info()
> would be skipped if nsid changed. However, this
> happens everytime the in-use controller is
> reattached if NID is bogus, causing nsid not being
> restored to the previous one, eg /dev/nvme0n2 ->
> /dev/nvme0n1.
> 
> Don't skip ns info updates on this circumstance.
> 
> Signed-off-by: Yihan Xin <xyh1996@...il.com>
> ---
>   drivers/nvme/host/core.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1236e3aa00ed..c0875fb93b8d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3979,11 +3979,24 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_info *info)
>   	int ret = NVME_SC_INVALID_NS | NVME_STATUS_DNR;
>   
>   	if (!nvme_ns_ids_equal(&ns->head->ids, &info->ids)) {
> -		dev_err(ns->ctrl->device,
> -			"identifiers changed for nsid %d\n", ns->head->ns_id);
> -		goto out;
> +		/*
> +		 * Don't skip ns info updates if the NID is bogus as it
> +		 * changes everytime the in-use controller is reattached
> +		 * to the bus and thus the namespace is recognized as
> +		 * another one.
> +		 */
> +		if (ns->ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
> +			dev_info(ns->ctrl->device,
> +				 "Ignoring nsid change for bogus ns\n");
> +		} else {
> +			dev_err(ns->ctrl->device,
> +				"identifiers changed for nsid %d\n",
> +				ns->head->ns_id);
> +			goto out;
> +		}
>   	}
>   
> +
>   	ret = nvme_update_ns_info(ns, info);
>   out:
>   	/*

Nope. A namespace is identified by both, the NSID and the namespace 
identifiers (GUID, UUID, you name it).
In our implementation we identify the namespace by NSID, and then
validate that the identifers match.
If you have a device which changes NSIDs you would need to swizzle
that logic around, ie identify the namespace by GUID/UUID, and then
check if the NSID matches.

But you'll run into issues here, as the NSID is stored in head->ns_id,
so if you attach a namespace with a different NSID you'll end up 
addressing the wrong namespace when sending commands.

So this 'fix' is actually wrong ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@...e.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich