Re: [PATCH] nvme: Do not drop the cdev reference twice when cdev_device_add() fails

[John Garry <john.garry@linux.dev>]

On 11/1/26 17:34, Yuqiang Sun wrote:
> Since commit 9952f3882ecb ("nvme: fix cdev lifetime"),
> nvme_add_ns_cdev() and nvme_add_ns_head_cdev() take a reference to the
> namespace (or namespace head) before calling nvme_cdev_add(), to be
> dropped by the char device's release, nvme_cdev_rel(), and drop it
> themselves when nvme_cdev_add() fails.  But when cdev_device_add()
> fails, nvme_cdev_add() has already put the device, and nvme_cdev_rel()
> has already dropped that reference: the caller drops it a second time.
> 
> The namespace (or head) is then freed while it is still in use.  With
> one allocation failure injected into cdev_device_add() for a namespace
> on an NVMe over loop controller, deleting the controller gives, with
> native multipath:
> 
>    BUG: KASAN: slab-use-after-free in nvme_put_ns_head+0x21/0x140
>    Write of size 4 at addr ff1100000a4ea078 by task init/1
>     nvme_put_ns_head
>     nvme_put_ns
>     nvme_remove_namespaces
>     nvme_do_delete_ctrl
>     nvme_delete_ctrl_sync
>     nvme_sysfs_delete
>    Freed by task 1:
>     kfree
>     nvme_ns_remove
>     nvme_remove_namespaces
>    refcount_t: underflow; use-after-free.
> 
> and without it, a use-after-free of the namespace in
> nvme_mpath_clear_ctrl_paths(), the namespace having been freed by the
> last close of its block device, followed by a general protection fault.
> 
> Take the reference in nvme_cdev_add() instead, right before
> cdev_device_add(), so that it is held exactly while the device can be
> released, and let the callers neither take nor drop it.  The earlier
> failures in nvme_cdev_add(), ida_alloc() and dev_set_name(), return
> before the device is initialized and so before the reference is taken.
> 
> The problem was found with kbound, a static analysis prototype that
> checks whether callers rely on an object after a callee returns a value
> for which the callee may have released it, on v7.2.12; it is present
> in mainline.
> 
> Build-tested with x86_64 and arm64 allmodconfig (W=1, sparse): no new
> warnings.  Run-tested on QEMU (x86_64, KVM) with KASAN: a loop target
> exporting a null_blk device, connected through /dev/nvme-fabrics, with
> failslab failing each allocation under nvme_cdev_add() in turn, with
> nvme_core.multipath=N and Y.  Before this patch, every failure in
> cdev_device_add() (7 of 7 with each setting) gives the reports above;
> with it nothing is reported, also with lockdep and kmemleak, which
> finds no leak either.  A failure in dev_set_name() gives no report,
> before and after.

Please ensure that you test my original issue reported. I am not sure if 
I added a blktests case for that...

BTW, I think that the most of the text after the KASAN stackframe, 
above, is unnecessary.

> 
> Fixes: 9952f3882ecb ("nvme: fix cdev lifetime")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuqiang Sun <izaiahsun@gmail.com>

Apart from small comments:
Reviewed-by: John Garry <john.garry@linux.dev>

> ---
> 
> Notes:
>      The bug was found with kbound, a research static analysis prototype.
>      The patch, the changelog and the test setup were written with the help
>      of an AI coding assistant (Claude); I have reviewed and tested them as
>      described above.
>      
>      The runs were on next-20261030; the code this patch touches is the same
>      there and in mainline, which it is based on.
> 
>   drivers/nvme/host/core.c      | 13 +++++++++++--
>   drivers/nvme/host/multipath.c |  2 --
>   2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index af71d02b8476..f9d23e9d35ec 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3978,6 +3978,17 @@ int nvme_cdev_add(struct cdev *cdev, struct device *cdev_device,
>   	device_initialize(cdev_device);
>   	cdev_init(cdev, fops);
>   	cdev->owner = owner;
> +	/*
> +	 * Held for the lifetime of the cdev and dropped in nvme_cdev_rel(),
> +	 * which also runs when cdev_device_add() fails.

I think that "which also runs when cdev_device_add() fails" becomes too 
verbose.

> +	 */
> +	if (cdev_device->parent->class == &nvme_class)

We have this check twice now. You could put this in a helper at this 
point, named like nvme_is_ns_cdev(). This would make the check clearer.

> +		nvme_get_ns(container_of(cdev_device, struct nvme_ns,
> +					 cdev_device));
> +	else
> +		nvme_get_ns_head(container_of(cdev_device,
> +					      struct nvme_ns_head,
> +					      cdev_device));
>   	ret = cdev_device_add(cdev, cdev_device);
>   	if (ret)
>   		put_device(cdev_device);
> @@ -4010,13 +4021,11 @@ static void nvme_add_ns_cdev(struct nvme_ns *ns)
>   {
>   	ns->cdev_device.parent = ns->ctrl->device;
>   
> -	nvme_get_ns(ns); /* Undone in nvme_cdev_rel() */
>   	if (nvme_cdev_add(&ns->cdev, &ns->cdev_device,
>   			&nvme_ns_chr_fops, ns->ctrl->ops->module,
>   			ns->ctrl->instance, ns->head->instance)) {
>   		dev_err(ns->ctrl->device, "Unable to create the ng%dn%d device\n",
>   			ns->ctrl->instance, ns->head->instance);
> -		nvme_put_ns(ns);
>   		return;
>   	}
>   	set_bit(NVME_NS_CDEV_LIVE, &ns->flags);
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9e0a0fd77c56..ae8059366b57 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -681,14 +681,12 @@ static void nvme_add_ns_head_cdev(struct nvme_ns_head *head)
>   {
>   	head->cdev_device.parent = &head->subsys->dev;
>   
> -	nvme_get_ns_head(head); /* Undone in nvme_cdev_rel() */
>   	if (nvme_cdev_add(&head->cdev, &head->cdev_device,
>   			&nvme_ns_head_chr_fops, THIS_MODULE,
>   			head->subsys->instance, head->instance)) {
>   		dev_err(disk_to_dev(head->disk),
>   			"Unable to create the ng%dn%d device\n",
>   			head->subsys->instance, head->instance);
> -		nvme_put_ns_head(head);
>   		return;
>   	}
>   	set_bit(NVME_NSHEAD_CDEV_LIVE, &head->flags);
> 
> base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e