[PATCH v3 0/2] IB/isert: fix the device removal use-after-free

[Yehyeong Lee]

The RDMA_CM_EVENT_DEVICE_REMOVAL callback in ib_isert waits for the
connection it was handed to reach ISER_CONN_DOWN and then frees it, which
is a use-after-free against isert_connect_release(), and the wait itself
deadlocks against the handler_mutex that rdma_destroy_id() needs.  2/2
moves the teardown into an ib_client removal callback, the way nvmet-rdma
did in commit f1d4ef7d8883.

1/2 is a separate problem that the review of v2 found.  It predates the
device removal work: isert_free_np() publishes the connections it is about
to release on a stack-local list without emptying isert_conn->node, so
isert_handle_unbound_conn() treats them as unclaimed, unlinks them from a
list it does not own and queues a second release.  It needs to go first
because 2/2 adds another caller of that path.

v2 -> v3

v2: https://lore.kernel.org/r/20261026060505.409476-1-yhlee@xxxxxxxxxxxxxxxxxx

The review of v2 raised four problems.  All four were real.

The race between isert_remove_one() and isert_free_np() on unbound
connections is 1/2.  Holding isert_np->mutex over the release loop would
close it and would also bring back the deadlock that commit 691b0480933f
("IB/isert: Fix dead lock in ib_isert") fixed, so 1/2 takes the
connections off isert_np one at a time, which empties isert_conn->node
before the mutex is dropped.

The race between isert_connected_handler() and isert_remove_one() was
introduced by v2.  Both handlers used to be reached only from
isert_cma_handler(), which the CM serialises per cm_id, so splitting the
ISER_CONN_UP store and the move onto isert_np->pending was safe until
isert_remove_one() started calling the teardown without that mutex.  2/2
keeps isert_conn->mutex across both.

The hang on an ISER_CONN_INIT connection is closed by marking it in
isert_teardown_conn() and having isert_connected_handler() release it
rather than promote it.  A marked connection whose peer never answers
holds the removal until the CM gives up on max_cm_retries.

The hang on racing connects is closed for the half where a connection the
walk has passed is promoted afterwards, which the previous item covers.
For a connect request arriving during the removal I have no measurement,
because no further RDMA_CM_EVENT_CONNECT_REQUEST arrived on soft-RoCE once
rdma link delete had started.  While isert_remove_one() is still running,
2/2 refuses such a request: isert_connect_request() fills isert_conn_list
and isert_np->accepted in one isert_conn_mutex section and returns -ENODEV
if the device is already being removed.

One window is left open and 2/2 does not close it.  Once
isert_remove_one() has returned, the isert_device for that ib_device is
gone, so a CONNECT_REQUEST that arrives before cma_remove_one() tears the
listener down makes isert_device_get() create a fresh isert_device, with
removing clear, and the connection stays in ISER_CONN_INIT.
cma_process_remove() then raises DEVICE_REMOVAL on that cm_id,
isert_teardown_conn() only marks the connection, and no further rdma_cm
event can reach it: cma_send_device_removal_put() has already set the id
to RDMA_CM_DEVICE_REMOVAL, and cma_ib_handler() drops every non-timewait
event for an id that is not in RDMA_CM_CONNECT.
Nothing destroys the cm_id, so the wait_for_completion(&cma_dev->comp) at
the end of cma_process_remove() does not return.  The unpatched code
blocks in the same case, in wait_event_interruptible(), so this is not a
regression, and it is not a memory-safety problem.  I have not reproduced
it.  A follow-up patch can close it.

One thing changed beyond answering the review.  The branch in
isert_connected_handler() that releases a connection it refuses to promote
disconnects and drains the QP first, because the login receive buffer is
still posted and isert_login_recv_done() dereferences isert_conn.

Measured with soft-RoCE under KASAN, removing the device under 32
connections: 4 of 12 unpatched runs hit the use-after-free, 76 reports in
all, and no removal finished within the cutoff; with this patch none did
and all 12 finished.  A connection held in ISER_CONN_INIT across the walk
took the removal to 73 seconds, 59 of them waiting for
RDMA_CM_EVENT_UNREACHABLE with status -110, against 14 seconds without
that delay.  Login and I/O showed no regression over 12 runs, and a
PROVE_LOCKING build reported nothing over 6 runs on each side.

Yehyeong Lee (2):
  IB/isert: do not publish connections being released on a second list
  IB/isert: tear connections down from an ib_client on device removal

 drivers/infiniband/ulp/isert/ib_isert.c | 203 +++++++++++++++++++++++---------
 drivers/infiniband/ulp/isert/ib_isert.h |   5 +-
 2 files changed, 151 insertions(+), 57 deletions(-)

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4