[PATCH RFC] nvme: fix NS head cdev lifetime

[John Garry]

On 08/07/2026 10:27, Christoph Hellwig wrote:

note: subject should be nvme-multipath for all of these. I'll repost 
(with subject fixed) if anyone cares that much.

> 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?

Sure, but I think that we need KASAN to detect

> 
>> 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://urldefense.com/v3/__https://lore.kernel.org/linux-scsi/20260703102918.3723667-1-john.g.garry@oracle.com/T/*m67265e2906d617acd2743c0a00809246d0cfc506__;Iw!!ACWV5N9M2RV99hQ!LwK_OTvYSZQLpG5Nv_1ODqf95BdDoh-lkhsgbnFpKEnWh0bva-VU-DZdOOWPX5UR3G978T5cOhj61A$
>>
>> Signed-off-by: John Garry <john.g.garry at 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 we put the nvne_ns_head (vs nvme_ns) in the driver data, then I can't 
see a check to know is the driver data points to a nvne_ns_head or 
nvme_ns. Unless we keep NULL for one, but that is not so nice.

Other options could be to
a. check the dev parent
b. put cdev fops in driver data, that's not as nice and we also need to 
make the NS and NS HEAD fops public

I'll figure something out..

>    
>> +	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.

sure

> 
> 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?
> 
It should not happen as we have a pointer to nvme_ns_head. If the 
recount was zero and we referenced ns_head, then we have bigger problems.