Re: [PATCH v2] nvme-rdma: fix ib_device removal race that hangs PCI unbind

[Sagi Grimberg <sagi@grimberg.me>]

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