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

[Casey Chen]

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.