Linux-NVME Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] nvme: fix cdev lifetime
@ 2026-07-13  9:15 John Garry
  2026-07-13  9:30 ` Christoph Hellwig
  0 siblings, 1 reply; 4+ messages in thread
From: John Garry @ 2026-07-13  9:15 UTC (permalink / raw)
  To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, John Garry

Sashiko bot reported a potential problem for the cdev lifetime in [0]
- the code there is heavily based on the NVMe code.

Currently the NS head .open and .release file_operations methods take and
put a reference to the nvme_ns_head to ensure that this structure does not
disappear while we open fds for that cdev.

In multipath mode, 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 a application which delays between opening the cdev
and issuing the ioctl while the NS head is being torn down:

# ./ioctl_file /dev/ng1n1 &
# waiting 10 seconds ....

# ./ini_nvme_teardown.sh
[   21.221718] nvme nvme1: Removing ctrl: NQN "nvme-test-target"
[   21.274609] nvme nvme2: Removing ctrl: NQN "nvme-test-target"
# now going to issue ioctl ....
[   26.549285] ==================================================================
[   26.550841] BUG: KASAN: slab-use-after-free in cdev_put.part.0+0x3d/0x40
[   26.552352] Read of size 8 at addr ffff88811e7fa170 by task ioctl_file/237
[   26.553805]
[   26.554227] CPU: 3 UID: 0 PID: 237 Comm: ioctl_file Not tainted 7.2.0-rc1-00004-g6852a10e32d4 #921 PREEMPT(lazy)
[   26.554236] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   26.554241] Call Trace:
[   26.554245]  <TASK>
[   26.554248]  dump_stack_lvl+0x68/0xa0
[   26.554266]  print_report+0x10d/0x5d0
[   26.554276]  ? __virt_addr_valid+0x21d/0x3f0
[   26.554287]  ? cdev_put.part.0+0x3d/0x40
[   26.554292]  kasan_report+0x96/0xd0
[   26.554300]  ? cdev_put.part.0+0x3d/0x40
[   26.554307]  cdev_put.part.0+0x3d/0x40
[   26.554313]  __fput+0x7bc/0xa70
[   26.554322]  fput_close_sync+0xd8/0x190
[   26.554328]  ? __pfx_fput_close_sync+0x10/0x10
[   26.554337]  __x64_sys_close+0x79/0xd0
[   26.554344]  do_syscall_64+0x117/0x6b0
[   26.554351]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   26.554358] RIP: 0033:0x7f938c067727
[   26.554364] Code: 48 89 fa 4c 89 df e8 28 ad 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44 24 10 0f 05 <5bf
[   26.554369] RSP: 002b:00007fff49f05980 EFLAGS: 00000202 ORIG_RAX: 0000000000000003
[   26.554376] RAX: ffffffffffffffda RBX: 00007f938bfd7780 RCX: 00007f938c067727
[   26.554380] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
[   26.554383] RBP: 00007fff49f05a10 R08: 0000000000000000 R09: 0000000000000000
[   26.554386] R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000000000
[   26.554389] R13: 00007fff49f05b40 R14: 00007f938c207000 R15: 000055c148471d78
[   26.554397]  </TASK>
[   26.554399]
[   26.575612] Allocated by task 100:
[   26.575871]  kasan_save_stack+0x24/0x50
[   26.576157]  kasan_save_track+0x14/0x30
[   26.576418]  __kasan_kmalloc+0x7f/0x90
[   26.576668]  __kmalloc_noprof+0x281/0x6c0
[   26.576938]  nvme_alloc_ns+0x7f7/0x3170
[   26.577206]  nvme_scan_ns+0x508/0x880
[   26.577449]  async_run_entry_fn+0x8c/0x350
[   26.577723]  process_scheduled_works+0xb6f/0x1a00
[   26.578034]  worker_thread+0x4ad/0xb40
[   26.578283]  kthread+0x34f/0x450
[   26.578501]  ret_from_fork+0x563/0x800
[   26.578752]  ret_from_fork_asm+0x1a/0x30
[   26.579012]
[   26.579124] Freed by task 237:
[   26.579335]  kasan_save_stack+0x24/0x50
[   26.579596]  kasan_save_track+0x14/0x30
[   26.579855]  kasan_save_free_info+0x3a/0x60
[   26.580131]  __kasan_slab_free+0x43/0x70
[   26.580388]  kfree+0x321/0x500
[   26.580591]  nvme_ns_head_chr_release+0x39/0x50
[   26.580883]  __fput+0x352/0xa70
[   26.581095]  fput_close_sync+0xd8/0x190
[   26.581350]  __x64_sys_close+0x79/0xd0
[   26.581595]  do_syscall_64+0x117/0x6b0
[   26.581842]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   26.582176]
[   26.582286] Last potentially related work creation:
[   26.582593]  kasan_save_stack+0x24/0x50
[   26.582841]  kasan_record_aux_stack+0x89/0xa0
[   26.583210]  insert_work+0x22/0x170
[   26.583442]  __queue_work+0x7b1/0xfa0
[   26.583682]  queue_work_on+0x77/0x80
[   26.583921]  kblockd_schedule_work+0x18/0x20
[   26.584207]  nvme_mpath_put_disk+0x42/0xa0
[   26.584632]  nvme_free_ns_head+0x1c/0x160
[   26.584904]  nvme_ns_head_chr_release+0x39/0x50
[   26.585208]  __fput+0x352/0xa70
[   26.585420]  fput_close_sync+0xd8/0x190
[   26.585677]  __x64_sys_close+0x79/0xd0
[   26.585924]  do_syscall_64+0x117/0x6b0
[   26.586173]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   26.586505]
[   26.586616] Second to last potentially related work creation:
[   26.586991]  kasan_save_stack+0x24/0x50
[   26.587246]  kasan_record_aux_stack+0x89/0xa0
[   26.587538]  insert_work+0x22/0x170
[   26.587770]  __queue_work+0x7b1/0xfa0
[   26.588010]  queue_work_on+0x77/0x80
[   26.588247]  kblockd_schedule_work+0x18/0x20
[   26.588529]  nvme_remove_head+0x3d/0xb0
[   26.588787]  nvme_ns_remove+0x4b2/0x930
[   26.589040]  nvme_remove_namespaces+0x29c/0x410
[   26.589340]  nvme_do_delete_ctrl+0xf3/0x190
[   26.589611]  nvme_delete_ctrl_sync+0x71/0x90
[   26.589889]  nvme_sysfs_delete+0x91/0xb0
[   26.590151]  kernfs_fop_write_iter+0x2fb/0x4a0
[   26.590452]  vfs_write+0x929/0xfc0
[   26.590688]  ksys_write+0xf2/0x1d0
[   26.590923]  do_syscall_64+0x117/0x6b0
[   26.591171]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   26.591498]
[   26.591605] The buggy address belongs to the object at ffff88811e7fa000
[   26.591605]  which belongs to the cache kmalloc-4k of size 4096
[   26.592409] The buggy address is located 368 bytes inside of
[   26.592409]  freed 4096-byte region [ffff88811e7fa000, ffff88811e7fb000)
[   26.593205]
[   26.593321] The buggy address belongs to the physical page:
[   26.593701] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x11e7f8
[   26.594250] head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[   26.594743] flags: 0x200000000000040(head|node=0|zone=2)
[   26.595093] page_type: f5(slab)
[   26.595312] raw: 0200000000000040 ffff888100043040 dead000000000122 0000000000000000
[   26.595810] raw: 0000000000000000 0000000000040004 00000000f5000000 0000000000000000
[   26.596309] head: 0200000000000040 ffff888100043040 dead000000000122 0000000000000000
[   26.596806] head: 0000000000000000 0000000000040004 00000000f5000000 0000000000000000
[   26.597313] head: 0200000000000003 fffffffffffffe01 00000000ffffffff 00000000ffffffff
[   26.597813] head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[   26.598317] page dumped because: kasan: bad access detected
[   26.598676]
[   26.598784] Memory state around the buggy address:
[   26.599093]  ffff88811e7fa000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   26.599559]  ffff88811e7fa080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   26.600023] >ffff88811e7fa100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   26.600485]                                                              ^
[   26.600921]  ffff88811e7fa180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   26.601390]  ffff88811e7fa200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   26.601855] ==================================================================
[   26.602374] Disabling lock debugging due to kernel taint

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

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.

The same problem exists for the NS cdev lifetime, so resolve that issue
through a similar method by taking a reference to the NS for the lifetime
of the cdev. Note that nvme_ns_chr_open() -> nvme_ns_open() also takes a
reference to the NS. Now that should not be needed, but that code is
common to bdev ioctl, so keep as is.

[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>
---
Differences to RFC:
- common release method
- fix NS cdev lifetime (as well as NS head cdev lifetime)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index db0c8ad4628a7..ef5bbd5ce9270 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3894,6 +3894,11 @@ static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
 static void nvme_cdev_rel(struct device *dev)
 {
 	ida_free(&nvme_ns_chr_minor_ida, MINOR(dev->devt));
+	if (dev->parent->class == &nvme_class)
+		nvme_put_ns(container_of(dev, struct nvme_ns, cdev_device));
+	else
+		nvme_put_ns_head(container_of(dev, struct nvme_ns_head,
+				cdev_device));
 }
 
 void nvme_cdev_del(struct cdev *cdev, struct device *cdev_device)
@@ -3959,10 +3964,12 @@ static void nvme_add_ns_cdev(struct nvme_ns *ns)
 	snprintf(name, sizeof(name), "ng%dn%d", ns->ctrl->instance,
 		 ns->head->instance);
 
+	nvme_get_ns(ns); /* Undone in nvme_cdev_rel() */
 	if (nvme_cdev_add(name, &ns->cdev, &ns->cdev_device,
 			&nvme_ns_chr_fops, ns->ctrl->ops->module)) {
 		dev_err(ns->ctrl->device, "Unable to create the %s device\n",
 			name);
+		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 56587ae59c7f7..820ec2576041a 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -630,28 +630,8 @@ const struct block_device_operations nvme_ns_head_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
-static inline struct nvme_ns_head *cdev_to_ns_head(struct cdev *cdev)
-{
-	return container_of(cdev, struct nvme_ns_head, cdev);
-}
-
-static int nvme_ns_head_chr_open(struct inode *inode, struct file *file)
-{
-	if (!nvme_tryget_ns_head(cdev_to_ns_head(inode->i_cdev)))
-		return -ENXIO;
-	return 0;
-}
-
-static int nvme_ns_head_chr_release(struct inode *inode, struct file *file)
-{
-	nvme_put_ns_head(cdev_to_ns_head(inode->i_cdev));
-	return 0;
-}
-
 static const struct file_operations nvme_ns_head_chr_fops = {
 	.owner		= THIS_MODULE,
-	.open		= nvme_ns_head_chr_open,
-	.release	= nvme_ns_head_chr_release,
 	.unlocked_ioctl	= nvme_ns_head_chr_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.uring_cmd	= nvme_ns_head_chr_uring_cmd,
@@ -666,10 +646,12 @@ static void nvme_add_ns_head_cdev(struct nvme_ns_head *head)
 	snprintf(name, sizeof(name), "ng%dn%d", head->subsys->instance,
 		 head->instance);
 
+	nvme_tryget_ns_head(head); /* Undone in nvme_cdev_rel() */
 	if (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;
 	}
 	set_bit(NVME_NSHEAD_CDEV_LIVE, &head->flags);
-- 
2.43.7



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] nvme: fix cdev lifetime
  2026-07-13  9:15 [PATCH v2] nvme: fix cdev lifetime John Garry
@ 2026-07-13  9:30 ` Christoph Hellwig
  2026-07-13  9:46   ` John Garry
  0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2026-07-13  9:30 UTC (permalink / raw)
  To: John Garry; +Cc: hch, kbusch, sagi, axboe, linux-nvme

On Mon, Jul 13, 2026 at 09:15:46AM +0000, John Garry wrote:
> Sashiko bot reported a potential problem for the cdev lifetime in [0]
> - the code there is heavily based on the NVMe code.
> 
> Currently the NS head .open and .release file_operations methods take and
> put a reference to the nvme_ns_head to ensure that this structure does not
> disappear while we open fds for that cdev.
> 
> In multipath mode, 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 a application which delays between opening the cdev
> and issuing the ioctl while the NS head is being torn down:
> 
> # ./ioctl_file /dev/ng1n1 &
> # waiting 10 seconds ....
> 
> # ./ini_nvme_teardown.sh
> [   21.221718] nvme nvme1: Removing ctrl: NQN "nvme-test-target"
> [   21.274609] nvme nvme2: Removing ctrl: NQN "nvme-test-target"
> # now going to issue ioctl ....
> [   26.549285] ==================================================================
> [   26.550841] BUG: KASAN: slab-use-after-free in cdev_put.part.0+0x3d/0x40
> [   26.552352] Read of size 8 at addr ffff88811e7fa170 by task ioctl_file/237
> [   26.553805]
> [   26.554227] CPU: 3 UID: 0 PID: 237 Comm: ioctl_file Not tainted 7.2.0-rc1-00004-g6852a10e32d4 #921 PREEMPT(lazy)
> [   26.554236] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [   26.554241] Call Trace:
> [   26.554245]  <TASK>
> [   26.554248]  dump_stack_lvl+0x68/0xa0
> [   26.554266]  print_report+0x10d/0x5d0
> [   26.554276]  ? __virt_addr_valid+0x21d/0x3f0
> [   26.554287]  ? cdev_put.part.0+0x3d/0x40
> [   26.554292]  kasan_report+0x96/0xd0
> [   26.554300]  ? cdev_put.part.0+0x3d/0x40
> [   26.554307]  cdev_put.part.0+0x3d/0x40
> [   26.554313]  __fput+0x7bc/0xa70
> [   26.554322]  fput_close_sync+0xd8/0x190
> [   26.554328]  ? __pfx_fput_close_sync+0x10/0x10
> [   26.554337]  __x64_sys_close+0x79/0xd0
> [   26.554344]  do_syscall_64+0x117/0x6b0
> [   26.554351]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [   26.554358] RIP: 0033:0x7f938c067727
> [   26.554364] Code: 48 89 fa 4c 89 df e8 28 ad 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44 24 10 0f 05 <5bf
> [   26.554369] RSP: 002b:00007fff49f05980 EFLAGS: 00000202 ORIG_RAX: 0000000000000003
> [   26.554376] RAX: ffffffffffffffda RBX: 00007f938bfd7780 RCX: 00007f938c067727
> [   26.554380] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
> [   26.554383] RBP: 00007fff49f05a10 R08: 0000000000000000 R09: 0000000000000000
> [   26.554386] R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000000000
> [   26.554389] R13: 00007fff49f05b40 R14: 00007f938c207000 R15: 000055c148471d78
> [   26.554397]  </TASK>
> [   26.554399]
> [   26.575612] Allocated by task 100:
> [   26.575871]  kasan_save_stack+0x24/0x50
> [   26.576157]  kasan_save_track+0x14/0x30
> [   26.576418]  __kasan_kmalloc+0x7f/0x90
> [   26.576668]  __kmalloc_noprof+0x281/0x6c0
> [   26.576938]  nvme_alloc_ns+0x7f7/0x3170
> [   26.577206]  nvme_scan_ns+0x508/0x880
> [   26.577449]  async_run_entry_fn+0x8c/0x350
> [   26.577723]  process_scheduled_works+0xb6f/0x1a00
> [   26.578034]  worker_thread+0x4ad/0xb40
> [   26.578283]  kthread+0x34f/0x450
> [   26.578501]  ret_from_fork+0x563/0x800
> [   26.578752]  ret_from_fork_asm+0x1a/0x30
> [   26.579012]
> [   26.579124] Freed by task 237:
> [   26.579335]  kasan_save_stack+0x24/0x50
> [   26.579596]  kasan_save_track+0x14/0x30
> [   26.579855]  kasan_save_free_info+0x3a/0x60
> [   26.580131]  __kasan_slab_free+0x43/0x70
> [   26.580388]  kfree+0x321/0x500
> [   26.580591]  nvme_ns_head_chr_release+0x39/0x50
> [   26.580883]  __fput+0x352/0xa70
> [   26.581095]  fput_close_sync+0xd8/0x190
> [   26.581350]  __x64_sys_close+0x79/0xd0
> [   26.581595]  do_syscall_64+0x117/0x6b0
> [   26.581842]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [   26.582176]
> [   26.582286] Last potentially related work creation:
> [   26.582593]  kasan_save_stack+0x24/0x50
> [   26.582841]  kasan_record_aux_stack+0x89/0xa0
> [   26.583210]  insert_work+0x22/0x170
> [   26.583442]  __queue_work+0x7b1/0xfa0
> [   26.583682]  queue_work_on+0x77/0x80
> [   26.583921]  kblockd_schedule_work+0x18/0x20
> [   26.584207]  nvme_mpath_put_disk+0x42/0xa0
> [   26.584632]  nvme_free_ns_head+0x1c/0x160
> [   26.584904]  nvme_ns_head_chr_release+0x39/0x50
> [   26.585208]  __fput+0x352/0xa70
> [   26.585420]  fput_close_sync+0xd8/0x190
> [   26.585677]  __x64_sys_close+0x79/0xd0
> [   26.585924]  do_syscall_64+0x117/0x6b0
> [   26.586173]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [   26.586505]
> [   26.586616] Second to last potentially related work creation:
> [   26.586991]  kasan_save_stack+0x24/0x50
> [   26.587246]  kasan_record_aux_stack+0x89/0xa0
> [   26.587538]  insert_work+0x22/0x170
> [   26.587770]  __queue_work+0x7b1/0xfa0
> [   26.588010]  queue_work_on+0x77/0x80
> [   26.588247]  kblockd_schedule_work+0x18/0x20
> [   26.588529]  nvme_remove_head+0x3d/0xb0
> [   26.588787]  nvme_ns_remove+0x4b2/0x930
> [   26.589040]  nvme_remove_namespaces+0x29c/0x410
> [   26.589340]  nvme_do_delete_ctrl+0xf3/0x190
> [   26.589611]  nvme_delete_ctrl_sync+0x71/0x90
> [   26.589889]  nvme_sysfs_delete+0x91/0xb0
> [   26.590151]  kernfs_fop_write_iter+0x2fb/0x4a0
> [   26.590452]  vfs_write+0x929/0xfc0
> [   26.590688]  ksys_write+0xf2/0x1d0
> [   26.590923]  do_syscall_64+0x117/0x6b0
> [   26.591171]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [   26.591498]
> [   26.591605] The buggy address belongs to the object at ffff88811e7fa000
> [   26.591605]  which belongs to the cache kmalloc-4k of size 4096
> [   26.592409] The buggy address is located 368 bytes inside of
> [   26.592409]  freed 4096-byte region [ffff88811e7fa000, ffff88811e7fb000)
> [   26.593205]
> [   26.593321] The buggy address belongs to the physical page:
> [   26.593701] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x11e7f8
> [   26.594250] head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> [   26.594743] flags: 0x200000000000040(head|node=0|zone=2)
> [   26.595093] page_type: f5(slab)
> [   26.595312] raw: 0200000000000040 ffff888100043040 dead000000000122 0000000000000000
> [   26.595810] raw: 0000000000000000 0000000000040004 00000000f5000000 0000000000000000
> [   26.596309] head: 0200000000000040 ffff888100043040 dead000000000122 0000000000000000
> [   26.596806] head: 0000000000000000 0000000000040004 00000000f5000000 0000000000000000
> [   26.597313] head: 0200000000000003 fffffffffffffe01 00000000ffffffff 00000000ffffffff
> [   26.597813] head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> [   26.598317] page dumped because: kasan: bad access detected
> [   26.598676]
> [   26.598784] Memory state around the buggy address:
> [   26.599093]  ffff88811e7fa000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [   26.599559]  ffff88811e7fa080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [   26.600023] >ffff88811e7fa100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [   26.600485]                                                              ^
> [   26.600921]  ffff88811e7fa180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [   26.601390]  ffff88811e7fa200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [   26.601855] ==================================================================
> [   26.602374] Disabling lock debugging due to kernel taint
> 
> 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).
> 
> 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.
> 
> The same problem exists for the NS cdev lifetime, so resolve that issue
> through a similar method by taking a reference to the NS for the lifetime
> of the cdev. Note that nvme_ns_chr_open() -> nvme_ns_open() also takes a
> reference to the NS. Now that should not be needed, but that code is
> common to bdev ioctl, so keep as is.
> 
> [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>
> ---
> Differences to RFC:
> - common release method
> - fix NS cdev lifetime (as well as NS head cdev lifetime)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index db0c8ad4628a7..ef5bbd5ce9270 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3894,6 +3894,11 @@ static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
>  static void nvme_cdev_rel(struct device *dev)
>  {
>  	ida_free(&nvme_ns_chr_minor_ida, MINOR(dev->devt));
> +	if (dev->parent->class == &nvme_class)
> +		nvme_put_ns(container_of(dev, struct nvme_ns, cdev_device));
> +	else
> +		nvme_put_ns_head(container_of(dev, struct nvme_ns_head,
> +				cdev_device));
>  }
>  
>  void nvme_cdev_del(struct cdev *cdev, struct device *cdev_device)
> @@ -3959,10 +3964,12 @@ static void nvme_add_ns_cdev(struct nvme_ns *ns)
>  	snprintf(name, sizeof(name), "ng%dn%d", ns->ctrl->instance,
>  		 ns->head->instance);
>  
> +	nvme_get_ns(ns); /* Undone in nvme_cdev_rel() */
>  	if (nvme_cdev_add(name, &ns->cdev, &ns->cdev_device,
>  			&nvme_ns_chr_fops, ns->ctrl->ops->module)) {
>  		dev_err(ns->ctrl->device, "Unable to create the %s device\n",
>  			name);
> +		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 56587ae59c7f7..820ec2576041a 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -630,28 +630,8 @@ const struct block_device_operations nvme_ns_head_ops = {
>  	.pr_ops		= &nvme_pr_ops,
>  };
>  
> -static inline struct nvme_ns_head *cdev_to_ns_head(struct cdev *cdev)
> -{
> -	return container_of(cdev, struct nvme_ns_head, cdev);
> -}
> -
> -static int nvme_ns_head_chr_open(struct inode *inode, struct file *file)
> -{
> -	if (!nvme_tryget_ns_head(cdev_to_ns_head(inode->i_cdev)))
> -		return -ENXIO;
> -	return 0;
> -}
> -
> -static int nvme_ns_head_chr_release(struct inode *inode, struct file *file)
> -{
> -	nvme_put_ns_head(cdev_to_ns_head(inode->i_cdev));
> -	return 0;
> -}
> -
>  static const struct file_operations nvme_ns_head_chr_fops = {
>  	.owner		= THIS_MODULE,
> -	.open		= nvme_ns_head_chr_open,
> -	.release	= nvme_ns_head_chr_release,
>  	.unlocked_ioctl	= nvme_ns_head_chr_ioctl,
>  	.compat_ioctl	= compat_ptr_ioctl,
>  	.uring_cmd	= nvme_ns_head_chr_uring_cmd,
> @@ -666,10 +646,12 @@ static void nvme_add_ns_head_cdev(struct nvme_ns_head *head)
>  	snprintf(name, sizeof(name), "ng%dn%d", head->subsys->instance,
>  		 head->instance);
>  
> +	nvme_tryget_ns_head(head); /* Undone in nvme_cdev_rel() */

I think we need to do a real get and not a tryget here.

Otherwise this looks good.



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] nvme: fix cdev lifetime
  2026-07-13  9:30 ` Christoph Hellwig
@ 2026-07-13  9:46   ` John Garry
  2026-07-13  9:48     ` Christoph Hellwig
  0 siblings, 1 reply; 4+ messages in thread
From: John Garry @ 2026-07-13  9:46 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: kbusch, sagi, axboe, linux-nvme

On 13/07/2026 10:30, Christoph Hellwig wrote:
>>   	.compat_ioctl	= compat_ptr_ioctl,
>>   	.uring_cmd	= nvme_ns_head_chr_uring_cmd,
>> @@ -666,10 +646,12 @@ static void nvme_add_ns_head_cdev(struct nvme_ns_head *head)
>>   	snprintf(name, sizeof(name), "ng%dn%d", head->subsys->instance,
>>   		 head->instance);
>>   
>> +	nvme_tryget_ns_head(head); /* Undone in nvme_cdev_rel() */
> I think we need to do a real get and not a tryget here.
> 

ok, np.

Then I am going to propose adding nvme_get_ns_head() also. It can be 
used elsewhere and it would match better with nvme_put_ns_head() (rather 
than using kref_fget(&head->kref) here)



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] nvme: fix cdev lifetime
  2026-07-13  9:46   ` John Garry
@ 2026-07-13  9:48     ` Christoph Hellwig
  0 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2026-07-13  9:48 UTC (permalink / raw)
  To: John Garry; +Cc: Christoph Hellwig, kbusch, sagi, axboe, linux-nvme

On Mon, Jul 13, 2026 at 10:46:26AM +0100, John Garry wrote:
>>>   +	nvme_tryget_ns_head(head); /* Undone in nvme_cdev_rel() */
>> I think we need to do a real get and not a tryget here.
>>
>
> ok, np.
>
> Then I am going to propose adding nvme_get_ns_head() also. It can be used 
> elsewhere and it would match better with nvme_put_ns_head() (rather than 
> using kref_fget(&head->kref) here)

Fine with me.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-07-13  9:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-13  9:15 [PATCH v2] nvme: fix cdev lifetime John Garry
2026-07-13  9:30 ` Christoph Hellwig
2026-07-13  9:46   ` John Garry
2026-07-13  9:48     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox