Linux RDMA and InfiniBand development
 help / color / mirror / Atom feed
* [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind
@ 2026-08-06 21:18 Casey Chen
  2026-08-13 10:07 ` Leon Romanovsky
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Casey Chen @ 2026-08-06 21:18 UTC (permalink / raw)
  To: linux-nvme; +Cc: kbusch, hch, sagi, axboe, linux-rdma, linux-kernel

nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
flush_workqueue(nvme_delete_wq). A connect can publish a controller on
the same ib_device during that window: nvme_rdma_find_get_device()
matches on node GUID in nvme_rdma's private device_list and never
consults ib_core unregistration state. Such a controller is never
deleted, so its rdma_cm_ids keep a reference on the cma_device.

ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
cma_remove_one(), which then waits for that reference forever. Removing
the RDMA interface underneath live NVMe-oF connections:

  echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove

wedges the unbind permanently:

  INFO: task tee:164872 blocked for more than 200 seconds.
  task:tee             state:D stack:0     pid:164872 ppid:164870 flags:0x00004002
  Call Trace:
   <TASK>
   __schedule+0x4b4/0xf90
   schedule+0x5a/0xc0
   schedule_timeout+0x105/0x110
   ? cma_process_remove+0x1f9/0x240 [rdma_cm]
   __wait_for_common+0xc7/0x1f0
   ? usleep_range_state+0xb0/0xb0
   cma_remove_one+0x50/0xb0 [rdma_cm]
   remove_client_context+0x88/0xc0 [ib_core]
   disable_device+0x8a/0x160 [ib_core]
   __ib_unregister_device+0x42/0xa0 [ib_core]
   ib_unregister_device+0x22/0x30 [ib_core]
   mlx5r_remove+0x39/0x60 [mlx5_ib]
   auxiliary_bus_remove+0x18/0x30
   device_release_driver_internal+0x18f/0x1f0
   bus_remove_device+0xbc/0x120
   device_del+0x154/0x3d0
   ? devl_param_driverinit_value_get+0x29/0x90
   mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
   mlx5_unregister_device+0x34/0x50 [mlx5_core]
   mlx5_uninit_one+0x45/0x110 [mlx5_core]
   remove_one+0x4e/0xc0 [mlx5_core]
   pci_device_remove+0x39/0xa0
   device_release_driver_internal+0x18f/0x1f0
   pci_stop_bus_device+0x68/0x90
   pci_stop_and_remove_bus_device_locked+0x28/0x40
   remove_store+0x75/0x90
   kernfs_fop_write_iter+0x147/0x1d0
   vfs_write+0x2af/0x410
   ksys_write+0x5f/0xe0
   do_syscall_64+0x35/0x80
   entry_SYSCALL_64_after_hwframe+0x4b/0xb5
   </TASK>

Because the unbind stalls mid-teardown the netdev is never unregistered,
so userspace keeps reconnecting over the interface and loses the race
again.

Mark the nvme_rdma_device dying before sampling nvme_rdma_ctrl_list and
test it in two places:

 - nvme_rdma_find_get_device() refuses a dying device, so later connects
   fail early. A re-probed HCA (same GUID, new ib_device) gets a fresh
   nvme_rdma_device.

 - nvme_rdma_create_ctrl() re-tests it under nvme_rdma_ctrl_mutex before
   publishing and deletes the controller instead if set, covering a
   connect that obtained the device before the flag was stored.

->dying is stored before nvme_rdma_remove_one() takes
nvme_rdma_ctrl_mutex and the publisher tests it under that same mutex,
so a publisher either lands on the list before the walk or observes
->dying. One sweep remains sufficient.

Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
interface carrying the NVMe-oF RDMA connections in a loop, with IO
running and a userspace daemon reconnecting the controllers throughout.
The hang is racy: most removals complete normally, and only one that
lands while a connect is in flight leaves the sysfs write stuck in D
state with the trace above. With this patch the loop ran clean: removals
complete and the controllers reconnect after the following PCI rescan.

Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device removal")
Signed-off-by: Casey Chen <cachen@purestorage.com>
---
 drivers/nvme/host/core.c |  1 +
 drivers/nvme/host/rdma.c | 49 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cb93ada4376a..374968145f56 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -277,6 +277,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 		nvme_do_delete_ctrl(ctrl);
 	nvme_put_ctrl(ctrl);
 }
+EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
 
 static blk_status_t nvme_error_status(u16 status)
 {
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 01743ae01466..b3ea54280e50 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -53,6 +53,8 @@ struct nvme_rdma_device {
 	struct list_head	entry
 		__guarded_by(&device_list_mutex);
 	unsigned int		num_inline_segments;
+	/* set under device_list_mutex when removal starts */
+	bool			dying;
 };
 
 struct nvme_rdma_qe {
@@ -378,6 +380,18 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
 
 	mutex_lock(&device_list_mutex);
 	list_for_each_entry(ndev, &device_list, entry) {
+		if (READ_ONCE(ndev->dying)) {
+			/*
+			 * Removal has already sampled nvme_rdma_ctrl_list, so
+			 * a controller created here would never be deleted.
+			 * A re-probed device with the same node GUID is a
+			 * distinct ib_device and gets a fresh
+			 * nvme_rdma_device below.
+			 */
+			if (ndev->dev == cm_id->device)
+				goto out_err;
+			continue;
+		}
 		if (ndev->dev->node_guid == cm_id->device->node_guid &&
 		    nvme_rdma_dev_get(ndev))
 			goto out_unlock;
@@ -2378,6 +2392,24 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, opts->host->nqn);
 
 	mutex_lock(&nvme_rdma_ctrl_mutex);
+	if (READ_ONCE(ctrl->device->dying)) {
+		mutex_unlock(&nvme_rdma_ctrl_mutex);
+		/*
+		 * Removal already walked nvme_rdma_ctrl_list, so publishing
+		 * now would leave this controller behind and stall
+		 * cma_remove_one() forever. ->list is still empty, so
+		 * nvme_rdma_free_ctrl() leaves opts for nvmf_create_ctrl().
+		 * nvme_init_ctrl() left two references and
+		 * nvme_delete_ctrl_sync() consumes only the one that
+		 * nvme_uninit_ctrl() drops, so put the other here.
+		 */
+		dev_info(ctrl->ctrl.device,
+			 "hca %s is being removed, aborting connect\n",
+			 dev_name(ctrl->device->dev->dma_device));
+		nvme_delete_ctrl_sync(&ctrl->ctrl);
+		nvme_put_ctrl(&ctrl->ctrl);
+		return ERR_PTR(-ECONNREFUSED);
+	}
 	list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
@@ -2409,9 +2441,16 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 	struct nvme_rdma_device *ndev;
 	bool found = false;
 
+	/*
+	 * Stop handing this device out to new queues, and stop new controllers
+	 * from being published on it, before sampling nvme_rdma_ctrl_list
+	 * below. Pairs with the READ_ONCE() of ->dying in
+	 * nvme_rdma_find_get_device() and nvme_rdma_create_ctrl().
+	 */
 	mutex_lock(&device_list_mutex);
 	list_for_each_entry(ndev, &device_list, entry) {
 		if (ndev->dev == ib_device) {
+			WRITE_ONCE(ndev->dying, true);
 			found = true;
 			break;
 		}
@@ -2421,7 +2460,15 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 	if (!found)
 		return;
 
-	/* Delete all controllers using this device */
+	/*
+	 * Delete all controllers using this device. ->dying is stored before
+	 * nvme_rdma_ctrl_mutex is acquired here, and nvme_rdma_create_ctrl()
+	 * checks it while holding that same mutex, so the two orderings are
+	 * exhaustive: a publisher that got the mutex first is on the list and
+	 * is deleted below, and one that gets it afterwards observes ->dying
+	 * and tears its controller down itself. No controller can be added
+	 * behind this walk.
+	 */
 	mutex_lock(&nvme_rdma_ctrl_mutex);
 	list_for_each_entry(ctrl, &nvme_rdma_ctrl_list, list) {
 		if (ctrl->device->dev != ib_device)

base-commit: bf881dd20062db5e951a0d0703cb476df8c9fdee
-- 
2.34.1


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

* Re: [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-06 21:18 [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind Casey Chen
@ 2026-08-13 10:07 ` Leon Romanovsky
  2026-08-23  0:37 ` Sagi Grimberg
  2026-08-28 23:24 ` [PATCH v2] " Casey Chen
  2 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2026-08-13 10:07 UTC (permalink / raw)
  To: Casey Chen; +Cc: linux-nvme, kbusch, hch, sagi, axboe, linux-rdma, linux-kernel

On Thu, Aug 06, 2026 at 03:18:22PM -0600, Casey Chen wrote:
> nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
> flush_workqueue(nvme_delete_wq). A connect can publish a controller on
> the same ib_device during that window: nvme_rdma_find_get_device()
> matches on node GUID in nvme_rdma's private device_list and never
> consults ib_core unregistration state. Such a controller is never
> deleted, so its rdma_cm_ids keep a reference on the cma_device.
> 
> ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
> cma_remove_one(), which then waits for that reference forever. Removing
> the RDMA interface underneath live NVMe-oF connections:
> 
>   echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove
> 
> wedges the unbind permanently:
> 
>   INFO: task tee:164872 blocked for more than 200 seconds.
>   task:tee             state:D stack:0     pid:164872 ppid:164870 flags:0x00004002
>   Call Trace:
>    <TASK>
>    __schedule+0x4b4/0xf90
>    schedule+0x5a/0xc0
>    schedule_timeout+0x105/0x110
>    ? cma_process_remove+0x1f9/0x240 [rdma_cm]
>    __wait_for_common+0xc7/0x1f0
>    ? usleep_range_state+0xb0/0xb0
>    cma_remove_one+0x50/0xb0 [rdma_cm]
>    remove_client_context+0x88/0xc0 [ib_core]
>    disable_device+0x8a/0x160 [ib_core]
>    __ib_unregister_device+0x42/0xa0 [ib_core]
>    ib_unregister_device+0x22/0x30 [ib_core]
>    mlx5r_remove+0x39/0x60 [mlx5_ib]
>    auxiliary_bus_remove+0x18/0x30
>    device_release_driver_internal+0x18f/0x1f0
>    bus_remove_device+0xbc/0x120
>    device_del+0x154/0x3d0
>    ? devl_param_driverinit_value_get+0x29/0x90
>    mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
>    mlx5_unregister_device+0x34/0x50 [mlx5_core]
>    mlx5_uninit_one+0x45/0x110 [mlx5_core]
>    remove_one+0x4e/0xc0 [mlx5_core]
>    pci_device_remove+0x39/0xa0
>    device_release_driver_internal+0x18f/0x1f0
>    pci_stop_bus_device+0x68/0x90
>    pci_stop_and_remove_bus_device_locked+0x28/0x40
>    remove_store+0x75/0x90
>    kernfs_fop_write_iter+0x147/0x1d0
>    vfs_write+0x2af/0x410
>    ksys_write+0x5f/0xe0
>    do_syscall_64+0x35/0x80
>    entry_SYSCALL_64_after_hwframe+0x4b/0xb5
>    </TASK>
> 
> Because the unbind stalls mid-teardown the netdev is never unregistered,
> so userspace keeps reconnecting over the interface and loses the race
> again.
> 
> Mark the nvme_rdma_device dying before sampling nvme_rdma_ctrl_list and
> test it in two places:
> 
>  - nvme_rdma_find_get_device() refuses a dying device, so later connects
>    fail early. A re-probed HCA (same GUID, new ib_device) gets a fresh
>    nvme_rdma_device.
> 
>  - nvme_rdma_create_ctrl() re-tests it under nvme_rdma_ctrl_mutex before
>    publishing and deletes the controller instead if set, covering a
>    connect that obtained the device before the flag was stored.
> 
> ->dying is stored before nvme_rdma_remove_one() takes
> nvme_rdma_ctrl_mutex and the publisher tests it under that same mutex,
> so a publisher either lands on the list before the walk or observes
> ->dying. One sweep remains sufficient.
> 
> Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
> interface carrying the NVMe-oF RDMA connections in a loop, with IO
> running and a userspace daemon reconnecting the controllers throughout.
> The hang is racy: most removals complete normally, and only one that
> lands while a connect is in flight leaves the sysfs write stuck in D
> state with the trace above. With this patch the loop ran clean: removals
> complete and the controllers reconnect after the following PCI rescan.
> 
> Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device removal")
> Signed-off-by: Casey Chen <cachen@purestorage.com>
> ---
>  drivers/nvme/host/core.c |  1 +
>  drivers/nvme/host/rdma.c | 49 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cb93ada4376a..374968145f56 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -277,6 +277,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
>  		nvme_do_delete_ctrl(ctrl);
>  	nvme_put_ctrl(ctrl);
>  }
> +EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
>  
>  static blk_status_t nvme_error_status(u16 status)
>  {
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 01743ae01466..b3ea54280e50 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -53,6 +53,8 @@ struct nvme_rdma_device {
>  	struct list_head	entry
>  		__guarded_by(&device_list_mutex);
>  	unsigned int		num_inline_segments;
> +	/* set under device_list_mutex when removal starts */
> +	bool			dying;
>  };
>  
>  struct nvme_rdma_qe {
> @@ -378,6 +380,18 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
>  
>  	mutex_lock(&device_list_mutex);
>  	list_for_each_entry(ndev, &device_list, entry) {
> +		if (READ_ONCE(ndev->dying)) {
> +			/*
> +			 * Removal has already sampled nvme_rdma_ctrl_list, so
> +			 * a controller created here would never be deleted.
> +			 * A re-probed device with the same node GUID is a
> +			 * distinct ib_device and gets a fresh
> +			 * nvme_rdma_device below.
> +			 */
> +			if (ndev->dev == cm_id->device)
> +				goto out_err;
> +			continue;
> +		}
>  		if (ndev->dev->node_guid == cm_id->device->node_guid &&
>  		    nvme_rdma_dev_get(ndev))
>  			goto out_unlock;
> @@ -2378,6 +2392,24 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>  		nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, opts->host->nqn);
>  
>  	mutex_lock(&nvme_rdma_ctrl_mutex);
> +	if (READ_ONCE(ctrl->device->dying)) {

"The write to ->dying is protected by &device_list_mutex, whereas
this path relies on &nvme_rdma_ctrl_mutex.

I did not look closely enough to suggest a complete fix for this
function, but for the removal path it would be better to remove the
dying IB device from the device list and move it to a local removal
list. That would eliminate the need for ->dying, at least in this
path.

Thanks

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

* Re: [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-06 21:18 [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind Casey Chen
  2026-08-13 10:07 ` Leon Romanovsky
@ 2026-08-23  0:37 ` Sagi Grimberg
  2026-08-28 23:24 ` [PATCH v2] " Casey Chen
  2 siblings, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2026-08-23  0:37 UTC (permalink / raw)
  To: Casey Chen, linux-nvme; +Cc: kbusch, hch, axboe, linux-rdma, linux-kernel



On 07/08/2026 0:18, Casey Chen wrote:
> nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
> flush_workqueue(nvme_delete_wq). A connect can publish a controller on
> the same ib_device during that window: nvme_rdma_find_get_device()
> matches on node GUID in nvme_rdma's private device_list and never
> consults ib_core unregistration state. Such a controller is never
> deleted, so its rdma_cm_ids keep a reference on the cma_device.
>
> ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
> cma_remove_one(), which then waits for that reference forever. Removing
> the RDMA interface underneath live NVMe-oF connections:
>
>    echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove
>
> wedges the unbind permanently:
>
>    INFO: task tee:164872 blocked for more than 200 seconds.
>    task:tee             state:D stack:0     pid:164872 ppid:164870 flags:0x00004002
>    Call Trace:
>     <TASK>
>     __schedule+0x4b4/0xf90
>     schedule+0x5a/0xc0
>     schedule_timeout+0x105/0x110
>     ? cma_process_remove+0x1f9/0x240 [rdma_cm]
>     __wait_for_common+0xc7/0x1f0
>     ? usleep_range_state+0xb0/0xb0
>     cma_remove_one+0x50/0xb0 [rdma_cm]
>     remove_client_context+0x88/0xc0 [ib_core]
>     disable_device+0x8a/0x160 [ib_core]
>     __ib_unregister_device+0x42/0xa0 [ib_core]
>     ib_unregister_device+0x22/0x30 [ib_core]
>     mlx5r_remove+0x39/0x60 [mlx5_ib]
>     auxiliary_bus_remove+0x18/0x30
>     device_release_driver_internal+0x18f/0x1f0
>     bus_remove_device+0xbc/0x120
>     device_del+0x154/0x3d0
>     ? devl_param_driverinit_value_get+0x29/0x90
>     mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
>     mlx5_unregister_device+0x34/0x50 [mlx5_core]
>     mlx5_uninit_one+0x45/0x110 [mlx5_core]
>     remove_one+0x4e/0xc0 [mlx5_core]
>     pci_device_remove+0x39/0xa0
>     device_release_driver_internal+0x18f/0x1f0
>     pci_stop_bus_device+0x68/0x90
>     pci_stop_and_remove_bus_device_locked+0x28/0x40
>     remove_store+0x75/0x90
>     kernfs_fop_write_iter+0x147/0x1d0
>     vfs_write+0x2af/0x410
>     ksys_write+0x5f/0xe0
>     do_syscall_64+0x35/0x80
>     entry_SYSCALL_64_after_hwframe+0x4b/0xb5
>     </TASK>
>
> Because the unbind stalls mid-teardown the netdev is never unregistered,
> so userspace keeps reconnecting over the interface and loses the race
> again.
>
> Mark the nvme_rdma_device dying before sampling nvme_rdma_ctrl_list and
> test it in two places:
>
>   - nvme_rdma_find_get_device() refuses a dying device, so later connects
>     fail early. A re-probed HCA (same GUID, new ib_device) gets a fresh
>     nvme_rdma_device.
>
>   - nvme_rdma_create_ctrl() re-tests it under nvme_rdma_ctrl_mutex before
>     publishing and deletes the controller instead if set, covering a
>     connect that obtained the device before the flag was stored.
>
> ->dying is stored before nvme_rdma_remove_one() takes
> nvme_rdma_ctrl_mutex and the publisher tests it under that same mutex,
> so a publisher either lands on the list before the walk or observes
> ->dying. One sweep remains sufficient.
>
> Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
> interface carrying the NVMe-oF RDMA connections in a loop, with IO
> running and a userspace daemon reconnecting the controllers throughout.
> The hang is racy: most removals complete normally, and only one that
> lands while a connect is in flight leaves the sysfs write stuck in D
> state with the trace above. With this patch the loop ran clean: removals
> complete and the controllers reconnect after the following PCI rescan.
>
> Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device removal")
> Signed-off-by: Casey Chen <cachen@purestorage.com>
> ---
>   drivers/nvme/host/core.c |  1 +
>   drivers/nvme/host/rdma.c | 49 +++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cb93ada4376a..374968145f56 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -277,6 +277,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
>   		nvme_do_delete_ctrl(ctrl);
>   	nvme_put_ctrl(ctrl);
>   }
> +EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
>   
>   static blk_status_t nvme_error_status(u16 status)
>   {
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 01743ae01466..b3ea54280e50 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -53,6 +53,8 @@ struct nvme_rdma_device {
>   	struct list_head	entry
>   		__guarded_by(&device_list_mutex);
>   	unsigned int		num_inline_segments;
> +	/* set under device_list_mutex when removal starts */
> +	bool			dying;

No need for the comment IMO.

>   };
>   
>   struct nvme_rdma_qe {
> @@ -378,6 +380,18 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
>   
>   	mutex_lock(&device_list_mutex);
>   	list_for_each_entry(ndev, &device_list, entry) {
> +		if (READ_ONCE(ndev->dying)) {
> +			/*
> +			 * Removal has already sampled nvme_rdma_ctrl_list, so
> +			 * a controller created here would never be deleted.
> +			 * A re-probed device with the same node GUID is a
> +			 * distinct ib_device and gets a fresh
> +			 * nvme_rdma_device below.
> +			 */
> +			if (ndev->dev == cm_id->device)
> +				goto out_err;
> +			continue;
> +		}

Again, code is self explanatory.

>   		if (ndev->dev->node_guid == cm_id->device->node_guid &&
>   		    nvme_rdma_dev_get(ndev))
>   			goto out_unlock;
> @@ -2378,6 +2392,24 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>   		nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, opts->host->nqn);
>   
>   	mutex_lock(&nvme_rdma_ctrl_mutex);
> +	if (READ_ONCE(ctrl->device->dying)) {
> +		mutex_unlock(&nvme_rdma_ctrl_mutex);
> +		/*
> +		 * Removal already walked nvme_rdma_ctrl_list, so publishing
> +		 * now would leave this controller behind and stall
> +		 * cma_remove_one() forever. ->list is still empty, so
> +		 * nvme_rdma_free_ctrl() leaves opts for nvmf_create_ctrl().
> +		 * nvme_init_ctrl() left two references and
> +		 * nvme_delete_ctrl_sync() consumes only the one that
> +		 * nvme_uninit_ctrl() drops, so put the other here.
> +		 */
> +		dev_info(ctrl->ctrl.device,
> +			 "hca %s is being removed, aborting connect\n",
> +			 dev_name(ctrl->device->dev->dma_device));
> +		nvme_delete_ctrl_sync(&ctrl->ctrl);
> +		nvme_put_ctrl(&ctrl->ctrl);
> +		return ERR_PTR(-ECONNREFUSED);
> +	}

I think that this check can be called in nvme_rdma_setup_ctrl() before 
calling
nvme_start_ctrl() which already has an error path no?

Again, I don't think that the commends are helpful. You are checking 
"dying" and
failing if it is set, its kinda make sense. The explanations can stay in 
the commit msg.

>   	list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
>   	mutex_unlock(&nvme_rdma_ctrl_mutex);
>   
> @@ -2409,9 +2441,16 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
>   	struct nvme_rdma_device *ndev;
>   	bool found = false;
>   
> +	/*
> +	 * Stop handing this device out to new queues, and stop new controllers
> +	 * from being published on it, before sampling nvme_rdma_ctrl_list
> +	 * below. Pairs with the READ_ONCE() of ->dying in
> +	 * nvme_rdma_find_get_device() and nvme_rdma_create_ctrl().
> +	 */

Same comment for code comments.

>   	mutex_lock(&device_list_mutex);
>   	list_for_each_entry(ndev, &device_list, entry) {
>   		if (ndev->dev == ib_device) {
> +			WRITE_ONCE(ndev->dying, true);
>   			found = true;
>   			break;
>   		}
> @@ -2421,7 +2460,15 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
>   	if (!found)
>   		return;
>   
> -	/* Delete all controllers using this device */
> +	/*
> +	 * Delete all controllers using this device. ->dying is stored before
> +	 * nvme_rdma_ctrl_mutex is acquired here, and nvme_rdma_create_ctrl()
> +	 * checks it while holding that same mutex, so the two orderings are
> +	 * exhaustive: a publisher that got the mutex first is on the list and
> +	 * is deleted below, and one that gets it afterwards observes ->dying
> +	 * and tears its controller down itself. No controller can be added
> +	 * behind this walk.
> +	 */

same comment.

>   	mutex_lock(&nvme_rdma_ctrl_mutex);
>   	list_for_each_entry(ctrl, &nvme_rdma_ctrl_list, list) {
>   		if (ctrl->device->dev != ib_device)
>
> base-commit: bf881dd20062db5e951a0d0703cb476df8c9fdee


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

* [PATCH v2] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-06 21:18 [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind Casey Chen
  2026-08-13 10:07 ` Leon Romanovsky
  2026-08-23  0:37 ` Sagi Grimberg
@ 2026-08-28 23:24 ` Casey Chen
  2026-08-30 21:10   ` Sagi Grimberg
  2026-08-31 22:13   ` [PATCH v3] " Casey Chen
  2 siblings, 2 replies; 9+ messages in thread
From: Casey Chen @ 2026-08-28 23:24 UTC (permalink / raw)
  To: linux-nvme; +Cc: leon, sagi, kbusch, hch, axboe, linux-rdma, linux-kernel

nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
flush_workqueue(nvme_delete_wq). A connect can publish a controller on
the same ib_device during that window: nvme_rdma_find_get_device()
matches on node GUID in nvme_rdma's private device_list and never
consults ib_core unregistration state. Such a controller is never
deleted, so its rdma_cm_ids keep a reference on the cma_device.

ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
cma_remove_one(), which then waits for that reference forever. Removing
the RDMA interface underneath live NVMe-oF connections:

  echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove

wedges the unbind permanently:

  INFO: task tee:164872 blocked for more than 200 seconds.
  task:tee             state:D stack:0     pid:164872 ppid:164870 flags:0x00004002
  Call Trace:
   <TASK>
   __schedule+0x4b4/0xf90
   schedule+0x5a/0xc0
   schedule_timeout+0x105/0x110
   ? cma_process_remove+0x1f9/0x240 [rdma_cm]
   __wait_for_common+0xc7/0x1f0
   ? usleep_range_state+0xb0/0xb0
   cma_remove_one+0x50/0xb0 [rdma_cm]
   remove_client_context+0x88/0xc0 [ib_core]
   disable_device+0x8a/0x160 [ib_core]
   __ib_unregister_device+0x42/0xa0 [ib_core]
   ib_unregister_device+0x22/0x30 [ib_core]
   mlx5r_remove+0x39/0x60 [mlx5_ib]
   auxiliary_bus_remove+0x18/0x30
   device_release_driver_internal+0x18f/0x1f0
   bus_remove_device+0xbc/0x120
   device_del+0x154/0x3d0
   ? devl_param_driverinit_value_get+0x29/0x90
   mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
   mlx5_unregister_device+0x34/0x50 [mlx5_core]
   mlx5_uninit_one+0x45/0x110 [mlx5_core]
   remove_one+0x4e/0xc0 [mlx5_core]
   pci_device_remove+0x39/0xa0
   device_release_driver_internal+0x18f/0x1f0
   pci_stop_bus_device+0x68/0x90
   pci_stop_and_remove_bus_device_locked+0x28/0x40
   remove_store+0x75/0x90
   kernfs_fop_write_iter+0x147/0x1d0
   vfs_write+0x2af/0x410
   ksys_write+0x5f/0xe0
   do_syscall_64+0x35/0x80
   entry_SYSCALL_64_after_hwframe+0x4b/0xb5
   </TASK>

Because the unbind stalls mid-teardown the netdev is never unregistered,
so userspace keeps reconnecting over the interface and loses the race
again.

Close the window at both ends, each with state guarded by the lock that
already covers the list it belongs to:

 - nvme_rdma_device gains ->dying, set and tested under
   device_list_mutex. nvme_rdma_find_get_device() refuses a device that
   is going away, so no new queues, PD or QP are created on it once
   nvme_rdma_remove_one() has started. A re-probed HCA with the same node
   GUID is a distinct ib_device and gets a fresh nvme_rdma_device.

 - nvme_rdma_remove_one() records the ib_device on nvme_rdma_removing_list
   in the same nvme_rdma_ctrl_mutex section that walks
   nvme_rdma_ctrl_list, and nvme_rdma_create_ctrl() tests that list under
   the same mutex immediately before publishing. A connect that took the
   mutex first is on the list and is deleted by the walk; one that takes
   it afterwards sees the entry and deletes its own controller. No
   controller can be added behind the walk, so a single sweep suffices.

The controller is fully live at the point the connect is refused, so it
is torn down with nvme_delete_ctrl_sync(). ->list is still empty there,
leaving opts to nvmf_create_ctrl(), and nvme_init_ctrl() left two
references while nvme_delete_ctrl_sync() consumes only the one that
nvme_uninit_ctrl() drops, so the other is put explicitly.

Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
interface carrying the NVMe-oF RDMA connections in a loop, with IO
running and a userspace daemon reconnecting the controllers throughout.
The hang is racy: most removals complete normally, and only one that
lands while a connect is in flight leaves the sysfs write stuck in D
state with the trace above. With this patch the loop ran clean: removals
complete and the controllers reconnect after the following PCI rescan.

Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device removal")
Signed-off-by: Casey Chen <cachen@purestorage.com>
---

Changes since v1:
https://lore.kernel.org/all/20260806211822.317074-1-cachen@purestorage.com/

 - ->dying is no longer read under nvme_rdma_ctrl_mutex (Leon). It stays
   guarded by device_list_mutex and is only tested in
   nvme_rdma_find_get_device(), which already holds that mutex. The test at
   the publish point now uses a separate nvme_rdma_removing_list guarded by
   nvme_rdma_ctrl_mutex, so each piece of state is accessed only under the
   lock that declares it and the READ_ONCE()/WRITE_ONCE() pair is gone. The
   two mutexes are still never held at the same time.
 - Drop the code comments; the reasoning lives in the commit message (Sagi).

Leon, on moving the dying device off device_list onto a local removal list
instead of keeping ->dying: that does remove the flag, but it also lets a
connect racing the removal allocate a fresh nvme_rdma_device and call
ib_alloc_pd() on the device being unregistered, since
nvme_rdma_find_get_device() would no longer see anything to refuse. That
exposure exists upstream today, so unlinking is not a regression, but
keeping ->dying closes it as well, which seemed worth the one bool now that
it is no longer read across locks. A local (on-stack) removal list also
needs care: nvme_rdma_free_dev() does list_del() whenever the last kref
drops, and a connect holding a reference can outlive nvme_rdma_remove_one(),
so the list head would have to be static rather than on the stack. Happy to
switch to unlinking if you prefer it.

Sagi, on moving the test into nvme_rdma_setup_ctrl() before
nvme_start_ctrl(): that would let the existing destroy_io path do the
unwind, which is nicer, but I do not think it closes the race on its own.
setup_ctrl() returns before nvme_rdma_create_ctrl() takes
nvme_rdma_ctrl_mutex and calls list_add_tail(), so a removal landing in that
gap still walks nvme_rdma_ctrl_list before the controller is published. The
test has to be atomic with the publish, which is why it stayed under the
mutex. Happy to be told I am missing something.

One window is knowingly left open. Once nvme_rdma_remove_one() has returned,
the removal entry is gone and the nvme_rdma_device it marked has usually
been freed by the last nvme_rdma_dev_put(), taking ->dying with it. A
connect arriving between that point and cma_remove_one() unlinking the
cma_device allocates a fresh nvme_rdma_device and can still publish a
controller that nothing will delete. ib_clients are removed LIFO, so that
gap spans every remaining client's remove callback. It exists upstream today
and is strictly narrower with this patch, so I did not try to cover it here.
Closing it needs a test keyed on ib_core state rather than nvme_rdma's, for
example an ->add callback storing a token so that
ib_get_client_data(cm_id->device, &nvme_rdma_ib_client) == NULL identifies
"our remove callback has already returned". That reads client_data outside
what its kernel-doc permits, so it seemed better kept as a separate patch.
 drivers/nvme/host/core.c |  1 +
 drivers/nvme/host/rdma.c | 41 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 758245c799a1..bede16fe1ff5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -282,6 +282,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 		nvme_do_delete_ctrl(ctrl);
 	nvme_put_ctrl(ctrl);
 }
+EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
 
 static blk_status_t nvme_error_status(u16 status)
 {
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 29ecbe71bb2e..63453d902619 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -45,6 +45,24 @@ static LIST_HEAD_GUARDED(device_list, device_list_mutex);
 
 static DEFINE_MUTEX(nvme_rdma_ctrl_mutex);
 static LIST_HEAD_GUARDED(nvme_rdma_ctrl_list, nvme_rdma_ctrl_mutex);
+static LIST_HEAD_GUARDED(nvme_rdma_removing_list, nvme_rdma_ctrl_mutex);
+
+struct nvme_rdma_removing_device {
+	struct list_head	entry
+		__guarded_by(&nvme_rdma_ctrl_mutex);
+	struct ib_device	*dev;
+};
+
+static bool nvme_rdma_device_removing(struct ib_device *ib_device)
+	__must_hold(&nvme_rdma_ctrl_mutex)
+{
+	struct nvme_rdma_removing_device *removing;
+
+	list_for_each_entry(removing, &nvme_rdma_removing_list, entry)
+		if (removing->dev == ib_device)
+			return true;
+	return false;
+}
 
 struct nvme_rdma_device {
 	struct ib_device	*dev;
@@ -53,6 +71,8 @@ struct nvme_rdma_device {
 	struct list_head	entry
 		__guarded_by(&device_list_mutex);
 	unsigned int		num_inline_segments;
+	bool			dying
+		__guarded_by(&device_list_mutex);
 };
 
 struct nvme_rdma_qe {
@@ -378,6 +398,11 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
 
 	mutex_lock(&device_list_mutex);
 	list_for_each_entry(ndev, &device_list, entry) {
+		if (ndev->dying) {
+			if (ndev->dev == cm_id->device)
+				goto out_err;
+			continue;
+		}
 		if (ndev->dev->node_guid == cm_id->device->node_guid &&
 		    nvme_rdma_dev_get(ndev))
 			goto out_unlock;
@@ -2380,6 +2405,15 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, opts->host->nqn);
 
 	mutex_lock(&nvme_rdma_ctrl_mutex);
+	if (nvme_rdma_device_removing(ctrl->device->dev)) {
+		mutex_unlock(&nvme_rdma_ctrl_mutex);
+		dev_info(ctrl->ctrl.device,
+			 "hca %s is being removed, aborting connect\n",
+			 dev_name(ctrl->device->dev->dma_device));
+		nvme_delete_ctrl_sync(&ctrl->ctrl);
+		nvme_put_ctrl(&ctrl->ctrl);
+		return ERR_PTR(-ECONNREFUSED);
+	}
 	list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
@@ -2407,6 +2441,7 @@ static struct nvmf_transport_ops nvme_rdma_transport = {
 
 static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 {
+	struct nvme_rdma_removing_device removing = { .dev = ib_device };
 	struct nvme_rdma_ctrl *ctrl;
 	struct nvme_rdma_device *ndev;
 	bool found = false;
@@ -2414,6 +2449,7 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 	mutex_lock(&device_list_mutex);
 	list_for_each_entry(ndev, &device_list, entry) {
 		if (ndev->dev == ib_device) {
+			ndev->dying = true;
 			found = true;
 			break;
 		}
@@ -2425,6 +2461,7 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 
 	/* Delete all controllers using this device */
 	mutex_lock(&nvme_rdma_ctrl_mutex);
+	list_add(&removing.entry, &nvme_rdma_removing_list);
 	list_for_each_entry(ctrl, &nvme_rdma_ctrl_list, list) {
 		if (ctrl->device->dev != ib_device)
 			continue;
@@ -2433,6 +2470,10 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
 	flush_workqueue(nvme_delete_wq);
+
+	mutex_lock(&nvme_rdma_ctrl_mutex);
+	list_del(&removing.entry);
+	mutex_unlock(&nvme_rdma_ctrl_mutex);
 }
 
 static struct ib_client nvme_rdma_ib_client = {

base-commit: 9eabc91952f9821824ca0288a76b3aba57961c6b
-- 
2.34.1


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

* Re: [PATCH v2] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-28 23:24 ` [PATCH v2] " Casey Chen
@ 2026-08-30 21:10   ` Sagi Grimberg
  2026-08-30 21:15     ` Sagi Grimberg
  2026-08-31 22:13   ` [PATCH v3] " Casey Chen
  1 sibling, 1 reply; 9+ messages in thread
From: Sagi Grimberg @ 2026-08-30 21:10 UTC (permalink / raw)
  To: Casey Chen, linux-nvme; +Cc: leon, kbusch, hch, axboe, linux-rdma, linux-kernel



On 29/08/2026 2:24, Casey Chen wrote:
> nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
> flush_workqueue(nvme_delete_wq). A connect can publish a controller on
> the same ib_device during that window: nvme_rdma_find_get_device()
> matches on node GUID in nvme_rdma's private device_list and never
> consults ib_core unregistration state. Such a controller is never
> deleted, so its rdma_cm_ids keep a reference on the cma_device.
>
> ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
> cma_remove_one(), which then waits for that reference forever. Removing
> the RDMA interface underneath live NVMe-oF connections:
>
>    echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove
>
> wedges the unbind permanently:
>
>    INFO: task tee:164872 blocked for more than 200 seconds.
>    task:tee             state:D stack:0     pid:164872 ppid:164870 flags:0x00004002
>    Call Trace:
>     <TASK>
>     __schedule+0x4b4/0xf90
>     schedule+0x5a/0xc0
>     schedule_timeout+0x105/0x110
>     ? cma_process_remove+0x1f9/0x240 [rdma_cm]
>     __wait_for_common+0xc7/0x1f0
>     ? usleep_range_state+0xb0/0xb0
>     cma_remove_one+0x50/0xb0 [rdma_cm]
>     remove_client_context+0x88/0xc0 [ib_core]
>     disable_device+0x8a/0x160 [ib_core]
>     __ib_unregister_device+0x42/0xa0 [ib_core]
>     ib_unregister_device+0x22/0x30 [ib_core]
>     mlx5r_remove+0x39/0x60 [mlx5_ib]
>     auxiliary_bus_remove+0x18/0x30
>     device_release_driver_internal+0x18f/0x1f0
>     bus_remove_device+0xbc/0x120
>     device_del+0x154/0x3d0
>     ? devl_param_driverinit_value_get+0x29/0x90
>     mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
>     mlx5_unregister_device+0x34/0x50 [mlx5_core]
>     mlx5_uninit_one+0x45/0x110 [mlx5_core]
>     remove_one+0x4e/0xc0 [mlx5_core]
>     pci_device_remove+0x39/0xa0
>     device_release_driver_internal+0x18f/0x1f0
>     pci_stop_bus_device+0x68/0x90
>     pci_stop_and_remove_bus_device_locked+0x28/0x40
>     remove_store+0x75/0x90
>     kernfs_fop_write_iter+0x147/0x1d0
>     vfs_write+0x2af/0x410
>     ksys_write+0x5f/0xe0
>     do_syscall_64+0x35/0x80
>     entry_SYSCALL_64_after_hwframe+0x4b/0xb5
>     </TASK>
>
> Because the unbind stalls mid-teardown the netdev is never unregistered,
> so userspace keeps reconnecting over the interface and loses the race
> again.
>
> Close the window at both ends, each with state guarded by the lock that
> already covers the list it belongs to:
>
>   - nvme_rdma_device gains ->dying, set and tested under
>     device_list_mutex. nvme_rdma_find_get_device() refuses a device that
>     is going away, so no new queues, PD or QP are created on it once
>     nvme_rdma_remove_one() has started. A re-probed HCA with the same node
>     GUID is a distinct ib_device and gets a fresh nvme_rdma_device.
>
>   - nvme_rdma_remove_one() records the ib_device on nvme_rdma_removing_list
>     in the same nvme_rdma_ctrl_mutex section that walks
>     nvme_rdma_ctrl_list, and nvme_rdma_create_ctrl() tests that list under
>     the same mutex immediately before publishing. A connect that took the
>     mutex first is on the list and is deleted by the walk; one that takes
>     it afterwards sees the entry and deletes its own controller. No
>     controller can be added behind the walk, so a single sweep suffices.
>
> The controller is fully live at the point the connect is refused, so it
> is torn down with nvme_delete_ctrl_sync(). ->list is still empty there,
> leaving opts to nvmf_create_ctrl(), and nvme_init_ctrl() left two
> references while nvme_delete_ctrl_sync() consumes only the one that
> nvme_uninit_ctrl() drops, so the other is put explicitly.
>
> Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
> interface carrying the NVMe-oF RDMA connections in a loop, with IO
> running and a userspace daemon reconnecting the controllers throughout.
> The hang is racy: most removals complete normally, and only one that
> lands while a connect is in flight leaves the sysfs write stuck in D
> state with the trace above. With this patch the loop ran clean: removals
> complete and the controllers reconnect after the following PCI rescan.
>
> Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device removal")
> Signed-off-by: Casey Chen <cachen@purestorage.com>
> ---
>
> Changes since v1:
> https://lore.kernel.org/all/20260806211822.317074-1-cachen@purestorage.com/
>
>   - ->dying is no longer read under nvme_rdma_ctrl_mutex (Leon). It stays
>     guarded by device_list_mutex and is only tested in
>     nvme_rdma_find_get_device(), which already holds that mutex. The test at
>     the publish point now uses a separate nvme_rdma_removing_list guarded by
>     nvme_rdma_ctrl_mutex, so each piece of state is accessed only under the
>     lock that declares it and the READ_ONCE()/WRITE_ONCE() pair is gone. The
>     two mutexes are still never held at the same time.
>   - Drop the code comments; the reasoning lives in the commit message (Sagi).
>
> Leon, on moving the dying device off device_list onto a local removal list
> instead of keeping ->dying: that does remove the flag, but it also lets a
> connect racing the removal allocate a fresh nvme_rdma_device and call
> ib_alloc_pd() on the device being unregistered, since
> nvme_rdma_find_get_device() would no longer see anything to refuse. That
> exposure exists upstream today, so unlinking is not a regression, but
> keeping ->dying closes it as well, which seemed worth the one bool now that
> it is no longer read across locks. A local (on-stack) removal list also
> needs care: nvme_rdma_free_dev() does list_del() whenever the last kref
> drops, and a connect holding a reference can outlive nvme_rdma_remove_one(),
> so the list head would have to be static rather than on the stack. Happy to
> switch to unlinking if you prefer it.
>
> Sagi, on moving the test into nvme_rdma_setup_ctrl() before
> nvme_start_ctrl(): that would let the existing destroy_io path do the
> unwind, which is nicer, but I do not think it closes the race on its own.
> setup_ctrl() returns before nvme_rdma_create_ctrl() takes
> nvme_rdma_ctrl_mutex and calls list_add_tail(), so a removal landing in that
> gap still walks nvme_rdma_ctrl_list before the controller is published. The
> test has to be atomic with the publish, which is why it stayed under the
> mutex. Happy to be told I am missing something.
>
> One window is knowingly left open. Once nvme_rdma_remove_one() has returned,
> the removal entry is gone and the nvme_rdma_device it marked has usually
> been freed by the last nvme_rdma_dev_put(), taking ->dying with it. A
> connect arriving between that point and cma_remove_one() unlinking the
> cma_device allocates a fresh nvme_rdma_device and can still publish a
> controller that nothing will delete. ib_clients are removed LIFO, so that
> gap spans every remaining client's remove callback. It exists upstream today
> and is strictly narrower with this patch, so I did not try to cover it here.
> Closing it needs a test keyed on ib_core state rather than nvme_rdma's, for
> example an ->add callback storing a token so that
> ib_get_client_data(cm_id->device, &nvme_rdma_ib_client) == NULL identifies
> "our remove callback has already returned". That reads client_data outside
> what its kernel-doc permits, so it seemed better kept as a separate patch.
>   drivers/nvme/host/core.c |  1 +
>   drivers/nvme/host/rdma.c | 41 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 42 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 758245c799a1..bede16fe1ff5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -282,6 +282,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
>   		nvme_do_delete_ctrl(ctrl);
>   	nvme_put_ctrl(ctrl);
>   }
> +EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
>   
>   static blk_status_t nvme_error_status(u16 status)
>   {
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 29ecbe71bb2e..63453d902619 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -45,6 +45,24 @@ static LIST_HEAD_GUARDED(device_list, device_list_mutex);
>   
>   static DEFINE_MUTEX(nvme_rdma_ctrl_mutex);
>   static LIST_HEAD_GUARDED(nvme_rdma_ctrl_list, nvme_rdma_ctrl_mutex);
> +static LIST_HEAD_GUARDED(nvme_rdma_removing_list, nvme_rdma_ctrl_mutex);
> +
> +struct nvme_rdma_removing_device {
> +	struct list_head	entry
> +		__guarded_by(&nvme_rdma_ctrl_mutex);
> +	struct ib_device	*dev;
> +};

I don't understand why this is needed.

> +
> +static bool nvme_rdma_device_removing(struct ib_device *ib_device)
> +	__must_hold(&nvme_rdma_ctrl_mutex)
> +{
> +	struct nvme_rdma_removing_device *removing;
> +
> +	list_for_each_entry(removing, &nvme_rdma_removing_list, entry)
> +		if (removing->dev == ib_device)
> +			return true;
> +	return false;
> +}
>   
>   struct nvme_rdma_device {
>   	struct ib_device	*dev;
> @@ -53,6 +71,8 @@ struct nvme_rdma_device {
>   	struct list_head	entry
>   		__guarded_by(&device_list_mutex);
>   	unsigned int		num_inline_segments;
> +	bool			dying
> +		__guarded_by(&device_list_mutex);
>   };
>   
>   struct nvme_rdma_qe {
> @@ -378,6 +398,11 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
>   
>   	mutex_lock(&device_list_mutex);
>   	list_for_each_entry(ndev, &device_list, entry) {
> +		if (ndev->dying) {
> +			if (ndev->dev == cm_id->device)
> +				goto out_err;
> +			continue;
> +		}
>   		if (ndev->dev->node_guid == cm_id->device->node_guid &&
>   		    nvme_rdma_dev_get(ndev))
>   			goto out_unlock;
> @@ -2380,6 +2405,15 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>   		nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, opts->host->nqn);
>   
>   	mutex_lock(&nvme_rdma_ctrl_mutex);
> +	if (nvme_rdma_device_removing(ctrl->device->dev)) {
> +		mutex_unlock(&nvme_rdma_ctrl_mutex);
> +		dev_info(ctrl->ctrl.device,
> +			 "hca %s is being removed, aborting connect\n",
> +			 dev_name(ctrl->device->dev->dma_device));
> +		nvme_delete_ctrl_sync(&ctrl->ctrl);
> +		nvme_put_ctrl(&ctrl->ctrl);
> +		return ERR_PTR(-ECONNREFUSED);
> +	}

Why not instead of this, simply do in nvme_rdma_setup_ctrl:

>   	list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
>   	mutex_unlock(&nvme_rdma_ctrl_mutex);
>   
> @@ -2407,6 +2441,7 @@ static struct nvmf_transport_ops nvme_rdma_transport = {
>   
>   static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
>   {
> +	struct nvme_rdma_removing_device removing = { .dev = ib_device };
>   	struct nvme_rdma_ctrl *ctrl;
>   	struct nvme_rdma_device *ndev;
>   	bool found = false;
> @@ -2414,6 +2449,7 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
>   	mutex_lock(&device_list_mutex);
>   	list_for_each_entry(ndev, &device_list, entry) {
>   		if (ndev->dev == ib_device) {
> +			ndev->dying = true;
>   			found = true;
>   			break;
>   		}
> @@ -2425,6 +2461,7 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
>   
>   	/* Delete all controllers using this device */
>   	mutex_lock(&nvme_rdma_ctrl_mutex);
> +	list_add(&removing.entry, &nvme_rdma_removing_list);
>   	list_for_each_entry(ctrl, &nvme_rdma_ctrl_list, list) {
>   		if (ctrl->device->dev != ib_device)
>   			continue;
> @@ -2433,6 +2470,10 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
>   	mutex_unlock(&nvme_rdma_ctrl_mutex);
>   
>   	flush_workqueue(nvme_delete_wq);
> +
> +	mutex_lock(&nvme_rdma_ctrl_mutex);
> +	list_del(&removing.entry);
> +	mutex_unlock(&nvme_rdma_ctrl_mutex);
>   }
>   
>   static struct ib_client nvme_rdma_ib_client = {
>
> base-commit: 9eabc91952f9821824ca0288a76b3aba57961c6b


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

* Re: [PATCH v2] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-30 21:10   ` Sagi Grimberg
@ 2026-08-30 21:15     ` Sagi Grimberg
  2026-08-31 22:07       ` Casey Chen
  0 siblings, 1 reply; 9+ messages in thread
From: Sagi Grimberg @ 2026-08-30 21:15 UTC (permalink / raw)
  To: Casey Chen, linux-nvme; +Cc: leon, kbusch, hch, axboe, linux-rdma, linux-kernel



On 31/08/2026 0:10, Sagi Grimberg wrote:
>
>
> On 29/08/2026 2:24, Casey Chen wrote:
>> nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
>> flush_workqueue(nvme_delete_wq). A connect can publish a controller on
>> the same ib_device during that window: nvme_rdma_find_get_device()
>> matches on node GUID in nvme_rdma's private device_list and never
>> consults ib_core unregistration state. Such a controller is never
>> deleted, so its rdma_cm_ids keep a reference on the cma_device.
>>
>> ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
>> cma_remove_one(), which then waits for that reference forever. Removing
>> the RDMA interface underneath live NVMe-oF connections:
>>
>>    echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove
>>
>> wedges the unbind permanently:
>>
>>    INFO: task tee:164872 blocked for more than 200 seconds.
>>    task:tee             state:D stack:0     pid:164872 ppid:164870 
>> flags:0x00004002
>>    Call Trace:
>>     <TASK>
>>     __schedule+0x4b4/0xf90
>>     schedule+0x5a/0xc0
>>     schedule_timeout+0x105/0x110
>>     ? cma_process_remove+0x1f9/0x240 [rdma_cm]
>>     __wait_for_common+0xc7/0x1f0
>>     ? usleep_range_state+0xb0/0xb0
>>     cma_remove_one+0x50/0xb0 [rdma_cm]
>>     remove_client_context+0x88/0xc0 [ib_core]
>>     disable_device+0x8a/0x160 [ib_core]
>>     __ib_unregister_device+0x42/0xa0 [ib_core]
>>     ib_unregister_device+0x22/0x30 [ib_core]
>>     mlx5r_remove+0x39/0x60 [mlx5_ib]
>>     auxiliary_bus_remove+0x18/0x30
>>     device_release_driver_internal+0x18f/0x1f0
>>     bus_remove_device+0xbc/0x120
>>     device_del+0x154/0x3d0
>>     ? devl_param_driverinit_value_get+0x29/0x90
>>     mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
>>     mlx5_unregister_device+0x34/0x50 [mlx5_core]
>>     mlx5_uninit_one+0x45/0x110 [mlx5_core]
>>     remove_one+0x4e/0xc0 [mlx5_core]
>>     pci_device_remove+0x39/0xa0
>>     device_release_driver_internal+0x18f/0x1f0
>>     pci_stop_bus_device+0x68/0x90
>>     pci_stop_and_remove_bus_device_locked+0x28/0x40
>>     remove_store+0x75/0x90
>>     kernfs_fop_write_iter+0x147/0x1d0
>>     vfs_write+0x2af/0x410
>>     ksys_write+0x5f/0xe0
>>     do_syscall_64+0x35/0x80
>>     entry_SYSCALL_64_after_hwframe+0x4b/0xb5
>>     </TASK>
>>
>> Because the unbind stalls mid-teardown the netdev is never unregistered,
>> so userspace keeps reconnecting over the interface and loses the race
>> again.
>>
>> Close the window at both ends, each with state guarded by the lock that
>> already covers the list it belongs to:
>>
>>   - nvme_rdma_device gains ->dying, set and tested under
>>     device_list_mutex. nvme_rdma_find_get_device() refuses a device that
>>     is going away, so no new queues, PD or QP are created on it once
>>     nvme_rdma_remove_one() has started. A re-probed HCA with the same 
>> node
>>     GUID is a distinct ib_device and gets a fresh nvme_rdma_device.
>>
>>   - nvme_rdma_remove_one() records the ib_device on 
>> nvme_rdma_removing_list
>>     in the same nvme_rdma_ctrl_mutex section that walks
>>     nvme_rdma_ctrl_list, and nvme_rdma_create_ctrl() tests that list 
>> under
>>     the same mutex immediately before publishing. A connect that took 
>> the
>>     mutex first is on the list and is deleted by the walk; one that 
>> takes
>>     it afterwards sees the entry and deletes its own controller. No
>>     controller can be added behind the walk, so a single sweep suffices.
>>
>> The controller is fully live at the point the connect is refused, so it
>> is torn down with nvme_delete_ctrl_sync(). ->list is still empty there,
>> leaving opts to nvmf_create_ctrl(), and nvme_init_ctrl() left two
>> references while nvme_delete_ctrl_sync() consumes only the one that
>> nvme_uninit_ctrl() drops, so the other is put explicitly.
>>
>> Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
>> interface carrying the NVMe-oF RDMA connections in a loop, with IO
>> running and a userspace daemon reconnecting the controllers throughout.
>> The hang is racy: most removals complete normally, and only one that
>> lands while a connect is in flight leaves the sysfs write stuck in D
>> state with the trace above. With this patch the loop ran clean: removals
>> complete and the controllers reconnect after the following PCI rescan.
>>
>> Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device 
>> removal")
>> Signed-off-by: Casey Chen <cachen@purestorage.com>
>> ---
>>
>> Changes since v1:
>> https://lore.kernel.org/all/20260806211822.317074-1-cachen@purestorage.com/ 
>>
>>
>>   - ->dying is no longer read under nvme_rdma_ctrl_mutex (Leon). It 
>> stays
>>     guarded by device_list_mutex and is only tested in
>>     nvme_rdma_find_get_device(), which already holds that mutex. The 
>> test at
>>     the publish point now uses a separate nvme_rdma_removing_list 
>> guarded by
>>     nvme_rdma_ctrl_mutex, so each piece of state is accessed only 
>> under the
>>     lock that declares it and the READ_ONCE()/WRITE_ONCE() pair is 
>> gone. The
>>     two mutexes are still never held at the same time.
>>   - Drop the code comments; the reasoning lives in the commit message 
>> (Sagi).
>>
>> Leon, on moving the dying device off device_list onto a local removal 
>> list
>> instead of keeping ->dying: that does remove the flag, but it also 
>> lets a
>> connect racing the removal allocate a fresh nvme_rdma_device and call
>> ib_alloc_pd() on the device being unregistered, since
>> nvme_rdma_find_get_device() would no longer see anything to refuse. That
>> exposure exists upstream today, so unlinking is not a regression, but
>> keeping ->dying closes it as well, which seemed worth the one bool 
>> now that
>> it is no longer read across locks. A local (on-stack) removal list also
>> needs care: nvme_rdma_free_dev() does list_del() whenever the last kref
>> drops, and a connect holding a reference can outlive 
>> nvme_rdma_remove_one(),
>> so the list head would have to be static rather than on the stack. 
>> Happy to
>> switch to unlinking if you prefer it.
>>
>> Sagi, on moving the test into nvme_rdma_setup_ctrl() before
>> nvme_start_ctrl(): that would let the existing destroy_io path do the
>> unwind, which is nicer, but I do not think it closes the race on its 
>> own.
>> setup_ctrl() returns before nvme_rdma_create_ctrl() takes
>> nvme_rdma_ctrl_mutex and calls list_add_tail(), so a removal landing 
>> in that
>> gap still walks nvme_rdma_ctrl_list before the controller is 
>> published. The
>> test has to be atomic with the publish, which is why it stayed under the
>> mutex. Happy to be told I am missing something.
>>
>> One window is knowingly left open. Once nvme_rdma_remove_one() has 
>> returned,
>> the removal entry is gone and the nvme_rdma_device it marked has usually
>> been freed by the last nvme_rdma_dev_put(), taking ->dying with it. A
>> connect arriving between that point and cma_remove_one() unlinking the
>> cma_device allocates a fresh nvme_rdma_device and can still publish a
>> controller that nothing will delete. ib_clients are removed LIFO, so 
>> that
>> gap spans every remaining client's remove callback. It exists 
>> upstream today
>> and is strictly narrower with this patch, so I did not try to cover 
>> it here.
>> Closing it needs a test keyed on ib_core state rather than 
>> nvme_rdma's, for
>> example an ->add callback storing a token so that
>> ib_get_client_data(cm_id->device, &nvme_rdma_ib_client) == NULL 
>> identifies
>> "our remove callback has already returned". That reads client_data 
>> outside
>> what its kernel-doc permits, so it seemed better kept as a separate 
>> patch.
>>   drivers/nvme/host/core.c |  1 +
>>   drivers/nvme/host/rdma.c | 41 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 758245c799a1..bede16fe1ff5 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -282,6 +282,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
>>           nvme_do_delete_ctrl(ctrl);
>>       nvme_put_ctrl(ctrl);
>>   }
>> +EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
>>     static blk_status_t nvme_error_status(u16 status)
>>   {
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 29ecbe71bb2e..63453d902619 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -45,6 +45,24 @@ static LIST_HEAD_GUARDED(device_list, 
>> device_list_mutex);
>>     static DEFINE_MUTEX(nvme_rdma_ctrl_mutex);
>>   static LIST_HEAD_GUARDED(nvme_rdma_ctrl_list, nvme_rdma_ctrl_mutex);
>> +static LIST_HEAD_GUARDED(nvme_rdma_removing_list, 
>> nvme_rdma_ctrl_mutex);
>> +
>> +struct nvme_rdma_removing_device {
>> +    struct list_head    entry
>> +        __guarded_by(&nvme_rdma_ctrl_mutex);
>> +    struct ib_device    *dev;
>> +};
>
> I don't understand why this is needed.
>
>> +
>> +static bool nvme_rdma_device_removing(struct ib_device *ib_device)
>> +    __must_hold(&nvme_rdma_ctrl_mutex)
>> +{
>> +    struct nvme_rdma_removing_device *removing;
>> +
>> +    list_for_each_entry(removing, &nvme_rdma_removing_list, entry)
>> +        if (removing->dev == ib_device)
>> +            return true;
>> +    return false;
>> +}
>>     struct nvme_rdma_device {
>>       struct ib_device    *dev;
>> @@ -53,6 +71,8 @@ struct nvme_rdma_device {
>>       struct list_head    entry
>>           __guarded_by(&device_list_mutex);
>>       unsigned int        num_inline_segments;
>> +    bool            dying
>> +        __guarded_by(&device_list_mutex);

This should probably be a flag.

>>   };
>>     struct nvme_rdma_qe {
>> @@ -378,6 +398,11 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
>>         mutex_lock(&device_list_mutex);
>>       list_for_each_entry(ndev, &device_list, entry) {
>> +        if (ndev->dying) {
>> +            if (ndev->dev == cm_id->device)
>> +                goto out_err;
>> +            continue;
>> +        }
>>           if (ndev->dev->node_guid == cm_id->device->node_guid &&
>>               nvme_rdma_dev_get(ndev))
>>               goto out_unlock;
>> @@ -2380,6 +2405,15 @@ static struct nvme_ctrl 
>> *nvme_rdma_create_ctrl(struct device *dev,
>>           nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, 
>> opts->host->nqn);
>>         mutex_lock(&nvme_rdma_ctrl_mutex);
>> +    if (nvme_rdma_device_removing(ctrl->device->dev)) {
>> +        mutex_unlock(&nvme_rdma_ctrl_mutex);
>> +        dev_info(ctrl->ctrl.device,
>> +             "hca %s is being removed, aborting connect\n",
>> +             dev_name(ctrl->device->dev->dma_device));
>> +        nvme_delete_ctrl_sync(&ctrl->ctrl);
>> +        nvme_put_ctrl(&ctrl->ctrl);
>> +        return ERR_PTR(-ECONNREFUSED);
>> +    }
>
> Why not instead of this, simply do in nvme_rdma_setup_ctrl:

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index b606565c2d85..58dedb57214c 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1108,6 +1108,11 @@ static int nvme_rdma_setup_ctrl(struct 
nvme_rdma_ctrl *ctrl, bool new)
                 goto destroy_io;
         }

+       if (nvme_rdma_device_dying(ctrl->device->dev)) {
+               nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
+               goto destroy_io;
+       }
+
         nvme_start_ctrl(&ctrl->ctrl);
         return 0;

>
>>       list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
>>       mutex_unlock(&nvme_rdma_ctrl_mutex);
>>   @@ -2407,6 +2441,7 @@ static struct nvmf_transport_ops 
>> nvme_rdma_transport = {
>>     static void nvme_rdma_remove_one(struct ib_device *ib_device, 
>> void *client_data)
>>   {
>> +    struct nvme_rdma_removing_device removing = { .dev = ib_device };
>>       struct nvme_rdma_ctrl *ctrl;
>>       struct nvme_rdma_device *ndev;
>>       bool found = false;
>> @@ -2414,6 +2449,7 @@ static void nvme_rdma_remove_one(struct 
>> ib_device *ib_device, void *client_data)
>>       mutex_lock(&device_list_mutex);
>>       list_for_each_entry(ndev, &device_list, entry) {
>>           if (ndev->dev == ib_device) {
>> +            ndev->dying = true;
>>               found = true;
>>               break;
>>           }

I'm wandering why not remove it from the list here?

I cannot see why this additional list with a dedicated struct is needed?

>> @@ -2425,6 +2461,7 @@ static void nvme_rdma_remove_one(struct 
>> ib_device *ib_device, void *client_data)
>>         /* Delete all controllers using this device */
>>       mutex_lock(&nvme_rdma_ctrl_mutex);
>> +    list_add(&removing.entry, &nvme_rdma_removing_list);
>>       list_for_each_entry(ctrl, &nvme_rdma_ctrl_list, list) {
>>           if (ctrl->device->dev != ib_device)
>>               continue;
>> @@ -2433,6 +2470,10 @@ static void nvme_rdma_remove_one(struct 
>> ib_device *ib_device, void *client_data)
>>       mutex_unlock(&nvme_rdma_ctrl_mutex);
>>         flush_workqueue(nvme_delete_wq);
>> +
>> +    mutex_lock(&nvme_rdma_ctrl_mutex);
>> +    list_del(&removing.entry);
>> +    mutex_unlock(&nvme_rdma_ctrl_mutex);
>>   }
>>     static struct ib_client nvme_rdma_ib_client = {
>>
>> base-commit: 9eabc91952f9821824ca0288a76b3aba57961c6b
>


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

* Re: [PATCH v2] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-30 21:15     ` Sagi Grimberg
@ 2026-08-31 22:07       ` Casey Chen
  0 siblings, 0 replies; 9+ messages in thread
From: Casey Chen @ 2026-08-31 22:07 UTC (permalink / raw)
  To: sagi; +Cc: leon, linux-nvme, kbusch, hch, axboe, linux-rdma, linux-kernel

On 31/08/2026 0:15, Sagi Grimberg wrote:
> Why not instead of this, simply do in nvme_rdma_setup_ctrl:
>
> +       if (nvme_rdma_device_dying(ctrl->device->dev)) {
> +               nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
> +               goto destroy_io;
> +       }
> +
>          nvme_start_ctrl(&ctrl->ctrl);
>          return 0;

I like that this reuses the existing error path, but I do not think it
closes the race. nvme_rdma_setup_ctrl() returns before
nvme_rdma_create_ctrl() takes nvme_rdma_ctrl_mutex and publishes, so the
test and the publish are not atomic with respect to the walk:

  Thread A (connect)                  Thread B (nvme_rdma_remove_one)
  ----------------------------------  ----------------------------------
  nvme_rdma_setup_ctrl()
    admin + IO queues up, LIVE
    device_dying() -> false
                                      mark device
                                      lock nvme_rdma_ctrl_mutex
                                      walk nvme_rdma_ctrl_list  (A absent)
                                      unlock
                                      flush_workqueue()
    nvme_start_ctrl()
    return 0
  nvme_rdma_create_ctrl()
    lock nvme_rdma_ctrl_mutex
    list_add_tail(&ctrl->list, ...)   <-- published after the walk
    unlock

A's controller is now on nvme_rdma_ctrl_list, nvme_rdma_remove_one() has
already finished and will not run again for this device, so nothing ever
deletes it. Its rdma_cm_ids keep the cma_device reference and
cma_remove_one() waits on it forever - the hang this patch is fixing.

> I cannot see why this additional list with a dedicated struct is needed?

Only to give that test state it may legally read. It has to run under
nvme_rdma_ctrl_mutex, and ->dying is guarded by device_list_mutex -
reading it there is what Leon objected to in v1:

  "The write to ->dying is protected by &device_list_mutex, whereas this
   path relies on &nvme_rdma_ctrl_mutex."

So the list is simply "which ib_devices are being removed", keyed on the
ib_device and guarded by nvme_rdma_ctrl_mutex. To be clear, it is not
what Leon suggested - he proposed moving the ndev off device_list, which
removes ->dying from nvme_rdma_find_get_device() but not from the
publish path, and he said as much ("at least in this path").

The list is not the only way to get that. Alternatives, in increasing
order of how much I like them:

1. A second bool on nvme_rdma_device, guarded by nvme_rdma_ctrl_mutex
   and set in the same section as the walk. Drops the list and the
   struct. Needs nvme_rdma_remove_one() to hold nvme_rdma_dev_get()
   across the callback so every racing connect tests the same ndev.

2. Read ->dying under device_list_mutex nested inside
   nvme_rdma_ctrl_mutex. One flag, no list. I would rather not:
   nvme_rdma_remove_one() already takes those two locks in the opposite
   order (sequentially, not nested), so this plants an ABBA trap for
   whoever tightens that function next.

3. Use client_data as a per-ib_device liveness token: an ->add that does
   ib_set_client_data(ib_device, &nvme_rdma_ib_client, ib_device),
   nvme_rdma_remove_one() clearing it first thing, and both
   nvme_rdma_find_get_device() and nvme_rdma_create_ctrl() testing
   ib_get_client_data() == NULL. No flag, no list, no struct, no pin,
   nothing to reset on re-probe, and ->add cannot fail because the token
   needs no allocation.

(3) also closes a window none of the others do, and which v2 leaves open
by its own admission: once nvme_rdma_remove_one() has returned, ->dying
is gone with the freed ndev, and a connect arriving before
cma_remove_one() unlinks the cma_device can still strand a controller.
remove_client_context() erases client_data after ->remove returns, so
the token stays NULL and that path is refused too.

The catch is that ib_get_client_data()'s kernel-doc says it "can only be
called while the client is registered to the device, once the ib_client
remove() callback returns this cannot be called", and (3) calls it
precisely to detect that state. It works - it is a bare xa_load() and
xa_erase() runs after ->remove - but it is outside what the API
documents. Leon, do you have an opinion on whether that is acceptable,
or whether ib_core should grow something explicit for it?

v3 follows doing (3). It is smaller than v2 (+29, no deletions) and
drops ->dying, the list and the struct, so it should address both of
your comments and Leon's. If the client_data use is not acceptable I
will respin as (1), which keeps everything inside nvme_rdma at the cost
of one bool and a kref held across the callback.

Two side effects of adding ->add that are worth naming:

 - A device with !kverbs_provider never gets ->add at all
   (add_client_context() returns early), so nvme_rdma now refuses it in
   nvme_rdma_find_get_device() rather than failing later when the QP is
   created. nvme_rdma cannot use such a device either way.

 - Clients are added FIFO, and rdma_cm registers before nvme_rdma, so
   during ib_register_device() cma_add_one() runs before
   nvme_rdma_add_one(). A connect resolving in that gap is refused with
   -ECONNREFUSED until ->add has run. It is self correcting on retry,
   but it is a real transient at probe time.

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

* [PATCH v3] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-28 23:24 ` [PATCH v2] " Casey Chen
  2026-08-30 21:10   ` Sagi Grimberg
@ 2026-08-31 22:13   ` Casey Chen
  2026-09-01 22:38     ` Casey Chen
  1 sibling, 1 reply; 9+ messages in thread
From: Casey Chen @ 2026-08-31 22:13 UTC (permalink / raw)
  To: linux-nvme; +Cc: leon, sagi, kbusch, hch, axboe, linux-rdma, linux-kernel

nvme_rdma_remove_one() samples nvme_rdma_ctrl_list once, then blocks in
flush_workqueue(nvme_delete_wq). A connect can publish a controller on
the same ib_device during that window: nvme_rdma_find_get_device()
matches on node GUID in nvme_rdma's private device_list and never
consults ib_core unregistration state. Such a controller is never
deleted, so its rdma_cm_ids keep a reference on the cma_device.

ib_clients are removed LIFO, so nvme_rdma_remove_one() runs before
cma_remove_one(), which then waits for that reference forever. Removing
the RDMA interface underneath live NVMe-oF connections:

  echo 1 | sudo tee /sys/bus/pci/devices/0000:2a:00.1/remove

wedges the unbind permanently:

  INFO: task tee:164872 blocked for more than 200 seconds.
  task:tee             state:D stack:0     pid:164872 ppid:164870 flags:0x00004002
  Call Trace:
   <TASK>
   __schedule+0x4b4/0xf90
   schedule+0x5a/0xc0
   schedule_timeout+0x105/0x110
   ? cma_process_remove+0x1f9/0x240 [rdma_cm]
   __wait_for_common+0xc7/0x1f0
   ? usleep_range_state+0xb0/0xb0
   cma_remove_one+0x50/0xb0 [rdma_cm]
   remove_client_context+0x88/0xc0 [ib_core]
   disable_device+0x8a/0x160 [ib_core]
   __ib_unregister_device+0x42/0xa0 [ib_core]
   ib_unregister_device+0x22/0x30 [ib_core]
   mlx5r_remove+0x39/0x60 [mlx5_ib]
   auxiliary_bus_remove+0x18/0x30
   device_release_driver_internal+0x18f/0x1f0
   bus_remove_device+0xbc/0x120
   device_del+0x154/0x3d0
   ? devl_param_driverinit_value_get+0x29/0x90
   mlx5_rescan_drivers_locked.part.0+0x78/0x1c0 [mlx5_core]
   mlx5_unregister_device+0x34/0x50 [mlx5_core]
   mlx5_uninit_one+0x45/0x110 [mlx5_core]
   remove_one+0x4e/0xc0 [mlx5_core]
   pci_device_remove+0x39/0xa0
   device_release_driver_internal+0x18f/0x1f0
   pci_stop_bus_device+0x68/0x90
   pci_stop_and_remove_bus_device_locked+0x28/0x40
   remove_store+0x75/0x90
   kernfs_fop_write_iter+0x147/0x1d0
   vfs_write+0x2af/0x410
   ksys_write+0x5f/0xe0
   do_syscall_64+0x35/0x80
   entry_SYSCALL_64_after_hwframe+0x4b/0xb5
   </TASK>

Because the unbind stalls mid-teardown the netdev is never unregistered,
so userspace keeps reconnecting over the interface and loses the race
again.

nvme_rdma has no ->add callback, so its client_data is unused. Use it as
a per-ib_device liveness token: ->add stores the ib_device itself,
nvme_rdma_remove_one() clears it before doing anything else, and ib_core
erases it once ->remove has returned. ib_get_client_data() returning
NULL therefore means "this device is being removed, or already has
been", which is exactly the span over which nvme_rdma must refuse it.
The token needs no allocation, so ->add cannot fail and leave a device
attached with no ->remove to follow.

Two sites test it:

 - nvme_rdma_find_get_device() refuses a device that is going away, so a
   connect starting after removal begins never builds a PD, CQ or QP on
   it, and no stale nvme_rdma_device is handed out by the node GUID
   match.

 - nvme_rdma_create_ctrl() tests it under nvme_rdma_ctrl_mutex
   immediately before publishing, which covers a connect that was
   already in flight. The token is cleared before
   nvme_rdma_remove_one() acquires that mutex, so the two orderings are
   exhaustive: a publisher that takes the mutex first is on the list and
   is deleted by the walk, and one that takes it afterwards observes
   NULL and deletes its own controller. No controller can be added
   behind the walk, so a single sweep suffices.

Keying on the ib_device rather than on nvme_rdma_device matters: the
lookup matches on node GUID while the removal matches on the ib_device
pointer, so the two are not one to one, and the token also outlives the
nvme_rdma_device, which is freed as soon as its last queue is torn down.

The controller is fully live at the point the connect is refused, so it
is torn down with nvme_delete_ctrl_sync(). ->list is still empty there,
leaving opts to nvmf_create_ctrl(), and nvme_init_ctrl() left two
references while nvme_delete_ctrl_sync() consumes only the one that
nvme_uninit_ctrl() drops, so the other is put explicitly.

Reproduced on a 6.6 based kernel by removing and rescanning the mlx5
interface carrying the NVMe-oF RDMA connections in a loop, with IO
running and a userspace daemon reconnecting the controllers throughout.
The hang is racy: most removals complete normally, and only one that
lands while a connect is in flight leaves the sysfs write stuck in D
state with the trace above. With this patch the loop ran clean: removals
complete and the controllers reconnect after the following PCI rescan.

Fixes: e87a911fed07 ("nvme-rdma: use ib_client API to detect device removal")
Signed-off-by: Casey Chen <cachen@purestorage.com>
---

Changes since v2:
https://lore.kernel.org/all/20260828232436.270184-1-cachen@purestorage.com/

 - Drop ->dying, nvme_rdma_removing_list and struct
   nvme_rdma_removing_device entirely (Sagi, Leon). The state is now a
   per-ib_device liveness token in client_data, which nvme_rdma was not
   using, so there is no new field, list or struct and nothing to reset
   on re-probe.
 - The test is keyed on the ib_device rather than on nvme_rdma_device.
   That also fixes something v2 got wrong: the lookup matches on node
   GUID while the removal matches on the ib_device pointer, so the two
   are not one to one, and a flag on nvme_rdma_device can be set on an
   ndev shared with a device that is not being removed.
 - This closes the window v2 documented as knowingly open. Once
   nvme_rdma_remove_one() returned, v2 had no state left, so a connect
   arriving before cma_remove_one() unlinks the cma_device could still
   strand a controller. remove_client_context() erases client_data after
   ->remove returns, so the token stays NULL and that path is refused.

Why the test cannot move to nvme_rdma_setup_ctrl(), and the alternatives
to this approach, are discussed in the reply to Sagi on v2:
https://lore.kernel.org/all/20260831220754.1714514-1-cachen@purestorage.com/

Two side effects of adding ->add, neither of which I think is a problem
but both worth naming:

 - A device with !kverbs_provider never gets ->add
   (add_client_context() returns early), so nvme_rdma now refuses it in
   nvme_rdma_find_get_device() rather than failing later at QP creation.
   nvme_rdma cannot use such a device either way.

 - Clients are added FIFO and rdma_cm registers before nvme_rdma, so
   during ib_register_device() cma_add_one() runs before
   nvme_rdma_add_one(). A connect resolving in that gap is refused with
   -ECONNREFUSED until ->add has run. Self correcting on retry, but a
   real transient at probe time.

Note ib_get_client_data()'s kernel-doc says it "can only be called while
the client is registered to the device, once the ib_client remove()
callback returns this cannot be called", and this uses it precisely to
detect that state. It works, since it is a bare xa_load() and xa_erase()
runs after ->remove, but it is outside what the API documents. Leon, if
you would rather ib_core grew something explicit for this, or if you
prefer the flag based variant, say so and I will respin.

The ->add path is new in this version and has not run on the setup that
reproduced the hang, so this wants a fresh soak before it is applied.
 drivers/nvme/host/core.c |  1 +
 drivers/nvme/host/rdma.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 758245c799a1..bede16fe1ff5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -282,6 +282,7 @@ void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 		nvme_do_delete_ctrl(ctrl);
 	nvme_put_ctrl(ctrl);
 }
+EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
 
 static blk_status_t nvme_error_status(u16 status)
 {
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 29ecbe71bb2e..04687fe22804 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -46,6 +46,19 @@ static LIST_HEAD_GUARDED(device_list, device_list_mutex);
 static DEFINE_MUTEX(nvme_rdma_ctrl_mutex);
 static LIST_HEAD_GUARDED(nvme_rdma_ctrl_list, nvme_rdma_ctrl_mutex);
 
+static struct ib_client nvme_rdma_ib_client;
+
+static int nvme_rdma_add_one(struct ib_device *ib_device)
+{
+	ib_set_client_data(ib_device, &nvme_rdma_ib_client, ib_device);
+	return 0;
+}
+
+static bool nvme_rdma_device_removing(struct ib_device *ib_device)
+{
+	return !ib_get_client_data(ib_device, &nvme_rdma_ib_client);
+}
+
 struct nvme_rdma_device {
 	struct ib_device	*dev;
 	struct ib_pd		*pd;
@@ -377,6 +390,9 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
 	struct nvme_rdma_device *ndev;
 
 	mutex_lock(&device_list_mutex);
+	if (nvme_rdma_device_removing(cm_id->device))
+		goto out_err;
+
 	list_for_each_entry(ndev, &device_list, entry) {
 		if (ndev->dev->node_guid == cm_id->device->node_guid &&
 		    nvme_rdma_dev_get(ndev))
@@ -2380,6 +2396,15 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		nvmf_ctrl_subsysnqn(&ctrl->ctrl), &ctrl->addr, opts->host->nqn);
 
 	mutex_lock(&nvme_rdma_ctrl_mutex);
+	if (nvme_rdma_device_removing(ctrl->device->dev)) {
+		mutex_unlock(&nvme_rdma_ctrl_mutex);
+		dev_info(ctrl->ctrl.device,
+			 "hca %s is being removed, aborting connect\n",
+			 dev_name(ctrl->device->dev->dma_device));
+		nvme_delete_ctrl_sync(&ctrl->ctrl);
+		nvme_put_ctrl(&ctrl->ctrl);
+		return ERR_PTR(-ECONNREFUSED);
+	}
 	list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
@@ -2411,6 +2436,8 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 	struct nvme_rdma_device *ndev;
 	bool found = false;
 
+	ib_set_client_data(ib_device, &nvme_rdma_ib_client, NULL);
+
 	mutex_lock(&device_list_mutex);
 	list_for_each_entry(ndev, &device_list, entry) {
 		if (ndev->dev == ib_device) {
@@ -2437,6 +2464,7 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 
 static struct ib_client nvme_rdma_ib_client = {
 	.name   = "nvme_rdma",
+	.add    = nvme_rdma_add_one,
 	.remove = nvme_rdma_remove_one
 };
 

base-commit: 9eabc91952f9821824ca0288a76b3aba57961c6b
-- 
2.34.1


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

* Re: [PATCH v3] nvme-rdma: fix ib_device removal race that hangs PCI unbind
  2026-08-31 22:13   ` [PATCH v3] " Casey Chen
@ 2026-09-01 22:38     ` Casey Chen
  0 siblings, 0 replies; 9+ messages in thread
From: Casey Chen @ 2026-09-01 22:38 UTC (permalink / raw)
  To: linux-nvme; +Cc: leon, sagi, kbusch, hch, axboe, linux-rdma, linux-kernel

On 31/08/2026 16:13, Casey Chen wrote:
> The ->add path is new in this version and has not run on the setup that
> reproduced the hang, so this wants a fresh soak before it is applied.

That soak is done now, so the caveat above no longer applies.

The test bounces both mlx5 interfaces carrying the NVMe-oF RDMA
connections, one at a time, with IO running throughout and a userspace
daemon reconnecting the controllers:

  ethtool -i <iface> | grep bus-info
  echo 1 | sudo tee /sys/bus/pci/devices/<bdf>/remove
  sleep 30
  echo 1 | sudo tee /sys/bus/pci/rescan
  sleep 30

30 iterations over the two interfaces, so 60 remove/rescan cycles, with
60s of settle time between iterations - roughly 90 minutes.

No hang with v3 applied. Every write to remove returned, the interface
came back on the following rescan and the controllers reconnected. The
->add path is covered by this, since each rescan re-probes the HCA as a
new ib_device and so goes through nvme_rdma_add_one().

For contrast, on the same setup without the fix the write to remove
eventually never returns and the task is left in D state with the trace
in the commit message. It is intermittent - most removals complete
normally, and only one that lands while a connect is in flight strands a
controller.

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

end of thread, other threads:[~2026-09-01 22:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-06 21:18 [PATCH] nvme-rdma: fix ib_device removal race that hangs PCI unbind Casey Chen
2026-08-13 10:07 ` Leon Romanovsky
2026-08-23  0:37 ` Sagi Grimberg
2026-08-28 23:24 ` [PATCH v2] " Casey Chen
2026-08-30 21:10   ` Sagi Grimberg
2026-08-30 21:15     ` Sagi Grimberg
2026-08-31 22:07       ` Casey Chen
2026-08-31 22:13   ` [PATCH v3] " Casey Chen
2026-09-01 22:38     ` Casey Chen

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