[PATCH v6 0/6] 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, a
use-after-free against isert_connect_release(), and the wait deadlocks
against the handler_mutex rdma_destroy_id() needs.  3/6 moves the teardown
into an ib_client callback, as nvmet-rdma did in f1d4ef7d8883, and gives
every connection one owner.  1/6 and 2/6 are older problems; 4/6, 5/6 and
6/6 close three holes the walk makes visible.

v5 -> v6

v5: https://lore.kernel.org/r/20261103125955.1530355-1-yhlee@xxxxxxxxxxxxxxxxxx

v4: https://lore.kernel.org/r/20261103032924.1073138-1-yhlee@xxxxxxxxxxxxxxxxxx

The high point on 3/5 was the BOUND path asking iscsi to reinstate the
connection, which during the final login response signals an rx thread that
is still waiting for its login completion, so it leaves without running the
connection exit path and the removal waits for ever.  6/6 asks for
reinstatement only from ISER_CONN_FULL_FEATURE.  The call and the path are
older than this series, so 6/6 carries its own Fixes tag.  A test-only
delay in that gap reproduced the hang on all three runs of 1 to 5 and on
none of three with 6/6.

Both medium points on 5/5 were right, and v6 removes their cause instead of
patching it.  5/6 no longer allocates anything: isert_add_one() stores the
ib_client data, isert_remove_one() clears it under device_list_mutex before
anything else, and isert_device_get() refuses a device whose data is NULL.
Nothing can pile up over repeated device removals, and the isert_init()
failure path, where ib_register_client() calls ib_unregister_client()
itself and every remove callback used to allocate a marker, has nothing
left to leak.  Twenty link removals and re-adds left seventeen and thirteen
unused structs on device_list before this change and none after.

The high point on 5/5 was right as well, though the refusal was never
permanent.  It is now exactly the gap between the core clearing our client
data and setting it again, which for a namespace move is disable_device()
to enable_device_and_get().  I could not measure that gap: moving a device
between namespaces needs exclusive netns mode, which this setup refuses
while the initiator keeps an rxe device in another namespace.

Measured over soft-RoCE with KASAN.  The cells v5 passed are unchanged from
1 to 5 to 1 to 6, 84 runs with no KASAN, no lockdep splat and no refcount
warning on either side, login and I/O included.  The late-device window
opened in three of fifteen runs of 1 to 4, each building a second
isert_device and taking ten or eleven core warnings, and in three of
fifteen runs with 5/6, which built none.  PROVE_LOCKING gave no splat in
twenty single runs.

Yehyeong Lee (6):
  IB/isert: do not publish connections being released on a second list
  IB/isert: drain the QP without holding isert_conn->mutex
  IB/isert: tear connections down from an ib_client on device removal
  IB/isert: drop the promotion reference when a connection errors out
  IB/isert: do not build a new isert_device for a device being removed
  IB/isert: only reinstate a connection that reached full feature phase

 drivers/infiniband/ulp/isert/ib_isert.c | 399 +++++++++++++++++-------
 drivers/infiniband/ulp/isert/ib_isert.h |   9 +-
 2 files changed, 297 insertions(+), 111 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0