Re: [PATCH RFC] nvme: fix NS head cdev lifetime

[Christoph Hellwig <hch@lst.de>]

On Mon, Jul 06, 2026 at 01:01:46PM +0000, John Garry wrote:
> Sashiko bot reported a potential problem for the NS head cdev lifetime in
> the libmultipath refactoring in [0].
> 
> Currently the NS head .open and .release file_operations methods take a
> reference to the nvme_ns_head to ensure that this structure does not
> disappear while we have files open.
> 
> When we teardown the NS head, we call nvme_cdev_del() -> cdev_device_del()
> -> cdev_del(). However after cdev_del() returns, cdevs already open will
> remain and their fops will still be callable. As such, we can still
> reference the cdev after the nvme_ns_head reference count drops to 0 (and
> is freed).
> 
> This can be shown with an application which delays between opening the cdev
> and issuing the ioctl while the NS head is being torn down:

Can you wire this up to blktests?

> When all fds for the cdev disappear, the cdev removal path puts a
> reference to the parent object, which is the nvme_ns_head.cdev_device - see
> cdev_default_release() -> kobject_put(parent). We can use the lifetime
> of the cdev_device to resolve this lifetime issue.
> 
> Fix the lifetime for the cdev by making adding the cdev add take a
> reference to the NS head and drop that reference in the
> nvme_ns_head.cdev_device release function.
> 
> [0] https://lore.kernel.org/linux-scsi/20260703102918.3723667-1-john.g.garry@oracle.com/T/#m67265e2906d617acd2743c0a00809246d0cfc506
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> Setting as RFC as I am not sure if this is the best solution.
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 453c1f0b2dd09..c7578b7ed6b76 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3887,7 +3887,7 @@ static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
>  	return 0;
>  }
>  
> -static void nvme_cdev_rel(struct device *dev)
> +void nvme_cdev_rel(struct device *dev)
>  {
>  	ida_free(&nvme_ns_chr_minor_ida, MINOR(dev->devt));
>  }

Can we unify this and the ns_head version by checking of which type
the device is somehow?  I think driver_data would still be free to
optionally point to the ns_head for eexample.
  
> +	if (nvme_tryget_ns_head(head)) {
> +		if (nvme_cdev_add(name, &head->cdev, &head->cdev_device,
> +			&nvme_ns_head_chr_fops, THIS_MODULE,
> +			nvme_cdev_ns_head_rel)) {

Messed up indenttation here as continuations in an conditional/loop
should not be indented the ssame way as the body.

This could be merged a bit anywya:

	if (nvme_tryget_ns_head(head) &&
	    nvme_cdev_add(name, &head->cdev, &head->cdev_device,
			&nvme_ns_head_chr_fops, THIS_MODULE)) {
		dev_err(disk_to_dev(head->disk),
			"Unable to create the %s device\n", name);
		nvme_put_ns_head(head);
		return;
	}

but shouldn't we warn for the nvme_tryget_ns_head failure as well?
Or can that even happen here?