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

[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@isslab.korea.ac.kr

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

[PATCH v3 1/2] IB/isert: do not publish connections being released on a second list

[Yehyeong Lee]

isert_free_np() moves the connections left on isert_np->pending and
isert_np->accepted onto a stack-local list, drops isert_np->mutex and
releases them from there.  isert_conn->node is then linked into that
local list instead of being empty, which is the test
isert_handle_unbound_conn() uses to decide whether a connection still
needs cleaning up.  It runs from the RDMA CM handler, which is not
serialised against isert_free_np(), so it unlinks the connection from a
list it does not own and queues a second release for it.

Take the connections off isert_np one at a time, so that
isert_conn->node is empty once isert_np->mutex is dropped.  The release
keeps running with no mutex held, which commit 691b0480933f ("IB/isert:
Fix dead lock in ib_isert") needs: releasing a connection destroys its
cm_id, and rdma_destroy_id() waits for the handler_mutex that the CM
handler holds while it waits for isert_np->mutex.

Reproduced with a test-only delay added inside isert_free_np()'s release
loop: 36 reports in each of three unpatched runs, none in three patched
runs.

  BUG: KASAN: slab-use-after-free in isert_free_np+0x6a4/0xab0
  Read of size 8 at addr ffff888109249040 by task rmdir/193

  CPU: 1 UID: 0 PID: 193 Comm: rmdir Not tainted 7.3.0-BASET-13248-g103b309e2033 #15 PREEMPT(full)
  Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  Call Trace:
   <TASK>
   dump_stack_lvl+0x53/0x70
   print_report+0xd0/0x630
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? isert_free_np+0x6a4/0xab0
   kasan_report+0xce/0x100
   ? isert_free_np+0x6a4/0xab0
   isert_free_np+0x6a4/0xab0
   ? __pfx_isert_free_np+0x10/0x10
   iscsit_del_np+0x163/0x340
   iscsit_tpg_del_network_portal+0x3c3/0x530
   ? mutex_unlock+0x7b/0xd0
   lio_target_call_delnpfromtpg+0x40/0x60
   config_item_cleanup+0x115/0x190
   configfs_rmdir+0x7c7/0x9f0
   ? __pfx_configfs_rmdir+0x10/0x10
   vfs_rmdir+0x291/0x770
   ? lookup_one_qstr_excl+0x1a/0x1d0
   filename_rmdir+0x35d/0x4f0
   ? __pfx_filename_rmdir+0x10/0x10
   ? kasan_save_track+0x14/0x30
   ? __kasan_slab_alloc+0x6e/0x70
   ? strncpy_from_user+0x2b/0x1c0
   __x64_sys_rmdir+0x41/0x60
   do_syscall_64+0xdd/0x4a0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
   </TASK>

  Allocated by task 10:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_cache_noprof+0x148/0x360
   isert_cma_handler+0x1e3/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_ib_req_handler+0x17d9/0x4480
   cm_process_work+0x41/0x330
   cm_work_handler+0x5727/0xc160
   process_one_work+0x697/0x1060
   worker_thread+0x45b/0xd10
   kthread+0x2c8/0x3b0
   ret_from_fork+0x36e/0x5a0
   ret_from_fork_asm+0x1a/0x30

  Freed by task 43:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x121/0x390
   process_one_work+0x697/0x1060
   worker_thread+0x45b/0xd10
   kthread+0x2c8/0x3b0
   ret_from_fork+0x36e/0x5a0
   ret_from_fork_asm+0x1a/0x30

  Last potentially related work creation:
   kasan_save_stack+0x33/0x60
   kasan_record_aux_stack+0x8c/0xa0
   __queue_work+0x7a1/0x15e0
   queue_work_on+0x5d/0x70
   isert_disconnected_handler.isra.0+0x20b/0x380
   isert_cma_handler+0xd71/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_ib_handler+0x190/0x750
   cm_process_work+0x41/0x330
   cm_work_handler+0x582b/0xc160
   process_one_work+0x697/0x1060
   worker_thread+0x45b/0xd10
   kthread+0x2c8/0x3b0
   ret_from_fork+0x36e/0x5a0
   ret_from_fork_asm+0x1a/0x30

  The buggy address belongs to the object at ffff888109248000
   which belongs to the cache kmalloc-8k of size 8192
  The buggy address is located 4160 bytes inside of
   freed 8192-byte region [ffff888109248000, ffff88810924a000)

Fixes: 691b0480933f ("IB/isert: Fix dead lock in ib_isert")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 54 ++++++++++++++-----------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index efa8d7b7844c97..5371bba71ba3cb 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2479,12 +2479,29 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
 	return 0;
 }
 
+static struct isert_conn *
+isert_np_take_conn(struct isert_np *isert_np)
+{
+	struct isert_conn *isert_conn;
+
+	lockdep_assert_held(&isert_np->mutex);
+
+	isert_conn = list_first_entry_or_null(&isert_np->pending,
+					      struct isert_conn, node);
+	if (!isert_conn)
+		isert_conn = list_first_entry_or_null(&isert_np->accepted,
+						      struct isert_conn, node);
+	if (isert_conn)
+		list_del_init(&isert_conn->node);
+
+	return isert_conn;
+}
+
 static void
 isert_free_np(struct iscsi_np *np)
 {
 	struct isert_np *isert_np = np->np_context;
-	struct isert_conn *isert_conn, *n;
-	LIST_HEAD(drop_conn_list);
+	struct isert_conn *isert_conn;
 
 	if (isert_np->cm_id)
 		rdma_destroy_id(isert_np->cm_id);
@@ -2497,31 +2514,22 @@ isert_free_np(struct iscsi_np *np)
 	 * up in accepted and pending lists.
 	 */
 	mutex_lock(&isert_np->mutex);
-	if (!list_empty(&isert_np->pending)) {
+	if (!list_empty(&isert_np->pending))
 		isert_info("Still have isert pending connections\n");
-		list_for_each_entry_safe(isert_conn, n,
-					 &isert_np->pending,
-					 node) {
-			isert_info("cleaning isert_conn %p state (%d)\n",
-				   isert_conn, isert_conn->state);
-			list_move_tail(&isert_conn->node, &drop_conn_list);
-		}
-	}
-
-	if (!list_empty(&isert_np->accepted)) {
+	if (!list_empty(&isert_np->accepted))
 		isert_info("Still have isert accepted connections\n");
-		list_for_each_entry_safe(isert_conn, n,
-					 &isert_np->accepted,
-					 node) {
-			isert_info("cleaning isert_conn %p state (%d)\n",
-				   isert_conn, isert_conn->state);
-			list_move_tail(&isert_conn->node, &drop_conn_list);
-		}
-	}
 	mutex_unlock(&isert_np->mutex);
 
-	list_for_each_entry_safe(isert_conn, n, &drop_conn_list, node) {
-		list_del_init(&isert_conn->node);
+	for (;;) {
+		mutex_lock(&isert_np->mutex);
+		isert_conn = isert_np_take_conn(isert_np);
+		mutex_unlock(&isert_np->mutex);
+
+		if (!isert_conn)
+			break;
+
+		isert_info("cleaning isert_conn %p state (%d)\n",
+			   isert_conn, isert_conn->state);
 		isert_connect_release(isert_conn);
 	}
 
-- 
2.43.0

[PATCH v3 2/2] IB/isert: tear connections down from an ib_client on device removal

[Yehyeong Lee]

The RDMA_CM_EVENT_DEVICE_REMOVAL callback waits on the connection state:

	wait_event_interruptible(isert_conn->rem_wait,
				 isert_conn->state == ISER_CONN_DOWN);
	kfree(isert_conn);

That is a use-after-free.  isert_release_work() publishes ISER_CONN_DOWN
before dropping the last reference, so the wait returns while that work is
still on its way into kref_put(), as in the splat below.  The state is read
with no lock held, so reordering the store does not help.

It also deadlocks.  cma_send_device_removal_put() runs the callback under
the cm_id's handler_mutex, which rdma_destroy_id() needs, so a teardown
past the dev_removed test blocks there while the callback waits for it.

Tear the connections down from an ib_client removal callback instead,
where no cm_id lock is held, and wait there for the last isert_device
reference, which isert_connect_release() drops.  nvmet-rdma did the same
in commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock
in device removal").  The cm callback keeps the cm_id and returns zero,
so dev_removed and rem_wait are gone.

Three things make that wait terminate.  A connection in ISER_CONN_INIT is
only marked, and isert_connected_handler() releases it.
isert_connect_request() fills isert_conn_list and isert_np->accepted under
isert_conn_mutex and rejects a request once the device is being removed.
isert_remove_one() holds isert_conn_mutex across the whole walk, because
it guards the lifetime of those connections and not just the list.

4 of 12 unpatched runs hit this use-after-free; none of 12 patched runs
did.

  BUG: KASAN: slab-use-after-free in isert_release_work+0x78/0x160
  Write of size 4 at addr ffff888105829180 by task kworker/u16:5/70

  CPU: 0 UID: 0 PID: 70 Comm: kworker/u16:5 Not tainted 7.3.0-CTL-13247-g330517c63aa9 #12 PREEMPT(full)
  Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  Workqueue: isert_release_wq isert_release_work
  Call Trace:
   <TASK>
   dump_stack_lvl+0x53/0x70
   print_report+0xd0/0x630
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? queued_spin_lock_slowpath+0xef/0xb10
   ? isert_release_work+0x78/0x160
   kasan_report+0xce/0x100
   ? isert_release_work+0x78/0x160
   kasan_check_range+0x105/0x1b0
   isert_release_work+0x78/0x160
   process_one_work+0x697/0x1060
   ? __pfx___schedule+0x10/0x10
   ? __pfx_process_one_work+0x10/0x10
   ? _raw_spin_lock_irq+0x81/0xe0
   ? __pfx_isert_release_work+0x10/0x10
   ? assign_work+0x11d/0x370
   worker_thread+0x45b/0xd10
   ? __pfx_worker_thread+0x10/0x10
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x2c8/0x3b0
   ? recalc_sigpending+0x15c/0x1e0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x36e/0x5a0
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x572/0xdc0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>

  Allocated by task 29:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_cache_noprof+0x148/0x360
   isert_cma_handler+0x1e3/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_ib_req_handler+0x17d9/0x4480
   cm_process_work+0x41/0x330
   cm_work_handler+0x5727/0xc160
   process_one_work+0x697/0x1060
   worker_thread+0x45b/0xd10
   kthread+0x2c8/0x3b0
   ret_from_fork+0x36e/0x5a0
   ret_from_fork_asm+0x1a/0x30

  Freed by task 218:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x121/0x390
   isert_cma_handler+0x10ff/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_process_remove+0xff/0x5a0
   cma_remove_one+0xcc/0x250
   remove_client_context+0xa4/0xf0
   disable_device+0x128/0x240
   __ib_unregister_device+0x226/0x3f0
   ib_unregister_device_and_put+0x50/0x70
   nldev_dellink+0x28e/0x3d0
   rdma_nl_rcv_msg+0x29c/0x4e0
   rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
   netlink_unicast+0x68f/0x940
   netlink_sendmsg+0x810/0xf20
   __sys_sendto+0x337/0x390
   __x64_sys_sendto+0xdb/0x1b0
   do_syscall_64+0xdd/0x4a0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

  Last potentially related work creation:
   kasan_save_stack+0x33/0x60
   kasan_record_aux_stack+0x8c/0xa0
   __queue_work+0x7a1/0x15e0
   queue_work_on+0x5d/0x70
   isert_disconnected_handler.isra.0+0x20b/0x380
   isert_cma_handler+0x101b/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_process_remove+0xff/0x5a0
   cma_remove_one+0xcc/0x250
   remove_client_context+0xa4/0xf0
   disable_device+0x128/0x240
   __ib_unregister_device+0x226/0x3f0
   ib_unregister_device_and_put+0x50/0x70
   nldev_dellink+0x28e/0x3d0
   rdma_nl_rcv_msg+0x29c/0x4e0
   rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
   netlink_unicast+0x68f/0x940
   netlink_sendmsg+0x810/0xf20
   __sys_sendto+0x337/0x390
   __x64_sys_sendto+0xdb/0x1b0
   do_syscall_64+0xdd/0x4a0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

  The buggy address belongs to the object at ffff888105828000
   which belongs to the cache kmalloc-8k of size 8192
  The buggy address is located 4480 bytes inside of
   freed 8192-byte region [ffff888105828000, ffff88810582a000)

Fixes: 63b268d232b8 ("IB/isert: Properly release resources on DEVICE_REMOVAL")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 149 +++++++++++++++++++-----
 drivers/infiniband/ulp/isert/ib_isert.h |   5 +-
 2 files changed, 120 insertions(+), 34 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 5371bba71ba3cb..223a727c1d0dec 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -43,6 +43,8 @@ MODULE_PARM_DESC(sg_tablesize,
 
 static DEFINE_MUTEX(device_list_mutex);
 static LIST_HEAD(device_list);
+static DEFINE_MUTEX(isert_conn_mutex);
+static LIST_HEAD(isert_conn_list);
 static struct workqueue_struct *isert_login_wq;
 static struct workqueue_struct *isert_comp_wq;
 static struct workqueue_struct *isert_release_wq;
@@ -56,6 +58,8 @@ isert_rdma_accept(struct isert_conn *isert_conn);
 struct rdma_cm_id *isert_setup_id(struct isert_np *isert_np);
 
 static void isert_release_work(struct work_struct *work);
+static void
+isert_handle_unbound_conn(struct isert_conn *isert_conn, bool iscsi_ref);
 static void isert_recv_done(struct ib_cq *cq, struct ib_wc *wc);
 static void isert_send_done(struct ib_cq *cq, struct ib_wc *wc);
 static void isert_login_recv_done(struct ib_cq *cq, struct ib_wc *wc);
@@ -255,6 +259,8 @@ isert_device_put(struct isert_device *device)
 	if (!device->refcount) {
 		isert_free_device_ib_res(device);
 		list_del(&device->dev_node);
+		if (device->removal_comp)
+			complete(device->removal_comp);
 		kfree(device);
 	}
 	mutex_unlock(&device_list_mutex);
@@ -309,7 +315,7 @@ isert_init_conn(struct isert_conn *isert_conn)
 	INIT_LIST_HEAD(&isert_conn->node);
 	init_completion(&isert_conn->login_comp);
 	init_completion(&isert_conn->login_req_comp);
-	init_waitqueue_head(&isert_conn->rem_wait);
+	INIT_LIST_HEAD(&isert_conn->conn_list);
 	kref_init(&isert_conn->kref);
 	atomic_set(&isert_conn->ctrl_comp_cnt, 0);
 	mutex_init(&isert_conn->mutex);
@@ -464,9 +470,19 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 	if (ret)
 		goto out_destroy_qp;
 
+	mutex_lock(&isert_conn_mutex);
+	if (device->removing) {
+		mutex_unlock(&isert_conn_mutex);
+		isert_info("device %p is being removed, rejecting conn %p\n",
+			   device, isert_conn);
+		ret = -ENODEV;
+		goto out_destroy_qp;
+	}
+	list_add_tail(&isert_conn->conn_list, &isert_conn_list);
 	mutex_lock(&isert_np->mutex);
 	list_add_tail(&isert_conn->node, &isert_np->accepted);
 	mutex_unlock(&isert_np->mutex);
+	mutex_unlock(&isert_conn_mutex);
 
 	return 0;
 
@@ -491,9 +507,12 @@ isert_connect_release(struct isert_conn *isert_conn)
 
 	BUG_ON(!device);
 
+	mutex_lock(&isert_conn_mutex);
+	list_del_init(&isert_conn->conn_list);
+	mutex_unlock(&isert_conn_mutex);
+
 	isert_free_rx_descriptors(isert_conn);
-	if (isert_conn->cm_id &&
-	    !isert_conn->dev_removed)
+	if (isert_conn->cm_id)
 		rdma_destroy_id(isert_conn->cm_id);
 
 	if (isert_conn->qp)
@@ -504,12 +523,9 @@ isert_connect_release(struct isert_conn *isert_conn)
 	if (isert_conn->login_desc)
 		isert_free_login_buf(isert_conn);
 
-	isert_device_put(device);
+	kfree(isert_conn);
 
-	if (isert_conn->dev_removed)
-		wake_up_interruptible(&isert_conn->rem_wait);
-	else
-		kfree(isert_conn);
+	isert_device_put(device);
 }
 
 static void
@@ -521,13 +537,25 @@ isert_connected_handler(struct rdma_cm_id *cma_id)
 	isert_info("conn %p\n", isert_conn);
 
 	mutex_lock(&isert_conn->mutex);
+	if (isert_conn->state >= ISER_CONN_TERMINATING) {
+		/* The login receive is still posted, drain before releasing. */
+		isert_info("conn %p is terminating, state %d\n",
+			   isert_conn, isert_conn->state);
+		if (rdma_disconnect(isert_conn->cm_id))
+			isert_warn("Failed rdma_disconnect isert_conn %p\n",
+				   isert_conn);
+		ib_drain_qp(isert_conn->qp);
+		isert_handle_unbound_conn(isert_conn, false);
+		mutex_unlock(&isert_conn->mutex);
+		return;
+	}
 	isert_conn->state = ISER_CONN_UP;
 	kref_get(&isert_conn->kref);
-	mutex_unlock(&isert_conn->mutex);
 
 	mutex_lock(&isert_np->mutex);
 	list_move_tail(&isert_conn->node, &isert_np->pending);
 	mutex_unlock(&isert_np->mutex);
+	mutex_unlock(&isert_conn->mutex);
 
 	isert_info("np %p: Allow accept_np to continue\n", isert_np);
 	up(&isert_np->sem);
@@ -552,7 +580,7 @@ isert_put_conn(struct isert_conn *isert_conn)
 }
 
 static void
-isert_handle_unbound_conn(struct isert_conn *isert_conn)
+isert_handle_unbound_conn(struct isert_conn *isert_conn, bool iscsi_ref)
 {
 	struct isert_np *isert_np = isert_conn->cm_id->context;
 
@@ -563,7 +591,8 @@ isert_handle_unbound_conn(struct isert_conn *isert_conn)
 		 * so schedule a cleanup ourselves
 		 */
 		list_del_init(&isert_conn->node);
-		isert_put_conn(isert_conn);
+		if (iscsi_ref)
+			isert_put_conn(isert_conn);
 		queue_work(isert_release_wq, &isert_conn->release_work);
 	}
 	mutex_unlock(&isert_np->mutex);
@@ -625,20 +654,23 @@ isert_np_cma_handler(struct isert_np *isert_np,
 	return -1;
 }
 
-static int
-isert_disconnected_handler(struct rdma_cm_id *cma_id,
-			   enum rdma_cm_event_type event)
+static void
+isert_teardown_conn(struct isert_conn *isert_conn)
 {
-	struct isert_conn *isert_conn = cma_id->qp->qp_context;
+	lockdep_assert_held(&isert_conn->mutex);
 
-	mutex_lock(&isert_conn->mutex);
 	switch (isert_conn->state) {
 	case ISER_CONN_TERMINATING:
+	case ISER_CONN_DOWN:
+		break;
+	case ISER_CONN_INIT:
+		/* Mark only; isert_connected_handler() releases it. */
+		isert_conn->state = ISER_CONN_TERMINATING;
 		break;
 	case ISER_CONN_UP:
 		isert_conn_terminate(isert_conn);
 		ib_drain_qp(isert_conn->qp);
-		isert_handle_unbound_conn(isert_conn);
+		isert_handle_unbound_conn(isert_conn, true);
 		break;
 	case ISER_CONN_BOUND:
 	case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
@@ -648,6 +680,16 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
 		isert_warn("conn %p terminating in state %d\n",
 			   isert_conn, isert_conn->state);
 	}
+}
+
+static int
+isert_disconnected_handler(struct rdma_cm_id *cma_id,
+			   enum rdma_cm_event_type event)
+{
+	struct isert_conn *isert_conn = cma_id->qp->qp_context;
+
+	mutex_lock(&isert_conn->mutex);
+	isert_teardown_conn(isert_conn);
 	mutex_unlock(&isert_conn->mutex);
 
 	return 0;
@@ -674,7 +716,6 @@ static int
 isert_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 {
 	struct isert_np *isert_np = cma_id->context;
-	struct isert_conn *isert_conn;
 	int ret = 0;
 
 	isert_info("%s (%d): status %d id %p np %p\n",
@@ -695,21 +736,10 @@ isert_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 		break;
 	case RDMA_CM_EVENT_ADDR_CHANGE:
 	case RDMA_CM_EVENT_DISCONNECTED:
-	case RDMA_CM_EVENT_TIMEWAIT_EXIT:  /* FALLTHRU */
+	case RDMA_CM_EVENT_TIMEWAIT_EXIT:
+	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 		ret = isert_disconnected_handler(cma_id, event->event);
 		break;
-	case RDMA_CM_EVENT_DEVICE_REMOVAL:
-		isert_conn = cma_id->qp->qp_context;
-		isert_conn->dev_removed = true;
-		isert_disconnected_handler(cma_id, event->event);
-		wait_event_interruptible(isert_conn->rem_wait,
-					 isert_conn->state == ISER_CONN_DOWN);
-		kfree(isert_conn);
-		/*
-		 * return non-zero from the callback to destroy
-		 * the rdma cm id
-		 */
-		return 1;
 	case RDMA_CM_EVENT_REJECTED:
 		isert_info("Connection rejected: %s\n",
 			   rdma_reject_msg(cma_id, event->status));
@@ -2697,8 +2727,54 @@ static struct iscsit_transport iser_target_transport = {
 	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
 };
 
+static void
+isert_remove_one(struct ib_device *ib_device, void *client_data)
+{
+	DECLARE_COMPLETION_ONSTACK(removal_comp);
+	struct isert_device *device, *found = NULL;
+	struct isert_conn *isert_conn;
+
+	mutex_lock(&device_list_mutex);
+	list_for_each_entry(device, &device_list, dev_node) {
+		if (device->ib_device != ib_device)
+			continue;
+		device->refcount++;
+		device->removal_comp = &removal_comp;
+		found = device;
+		break;
+	}
+	mutex_unlock(&device_list_mutex);
+
+	if (!found)
+		return;
+
+	mutex_lock(&isert_conn_mutex);
+	found->removing = true;
+	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
+		if (isert_conn->device != found)
+			continue;
+
+		isert_info("removing conn %p state %d\n",
+			   isert_conn, isert_conn->state);
+		mutex_lock(&isert_conn->mutex);
+		isert_teardown_conn(isert_conn);
+		mutex_unlock(&isert_conn->mutex);
+	}
+	mutex_unlock(&isert_conn_mutex);
+
+	isert_device_put(found);
+	wait_for_completion(&removal_comp);
+}
+
+static struct ib_client isert_ib_client = {
+	.name   = "isert",
+	.remove = isert_remove_one,
+};
+
 static int __init isert_init(void)
 {
+	int ret = -ENOMEM;
+
 	isert_login_wq = alloc_workqueue("isert_login_wq", WQ_PERCPU, 0);
 	if (!isert_login_wq) {
 		isert_err("Unable to allocate isert_login_wq\n");
@@ -2719,21 +2795,30 @@ static int __init isert_init(void)
 		goto destroy_comp_wq;
 	}
 
+	ret = ib_register_client(&isert_ib_client);
+	if (ret) {
+		isert_err("Unable to register isert ib client\n");
+		goto destroy_release_wq;
+	}
+
 	iscsit_register_transport(&iser_target_transport);
 	isert_info("iSER_TARGET[0] - Loaded iser_target_transport\n");
 
 	return 0;
 
+destroy_release_wq:
+	destroy_workqueue(isert_release_wq);
 destroy_comp_wq:
 	destroy_workqueue(isert_comp_wq);
 destroy_login_wq:
 	destroy_workqueue(isert_login_wq);
 
-	return -ENOMEM;
+	return ret;
 }
 
 static void __exit isert_exit(void)
 {
+	ib_unregister_client(&isert_ib_client);
 	flush_workqueue(isert_login_wq);
 	destroy_workqueue(isert_release_wq);
 	destroy_workqueue(isert_comp_wq);
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 519b17e54bd342..802a12c5aae086 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -191,8 +191,7 @@ struct isert_conn {
 	atomic_t		ctrl_comp_cnt;
 	bool                    logout_posted;
 	bool                    snd_w_inv;
-	wait_queue_head_t	rem_wait;
-	bool			dev_removed;
+	struct list_head	conn_list;
 };
 
 struct isert_device {
@@ -203,6 +202,8 @@ struct isert_device {
 	struct isert_comp	*comps;
 	int                     comps_used;
 	struct list_head	dev_node;
+	struct completion	*removal_comp;
+	bool			removing;
 };
 
 struct isert_np {
-- 
2.43.0

Re: [PATCH v3 1/2] IB/isert: do not publish connections being released on a second list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] IB/isert: race between isert_free_np() and isert_connected_handler()
- [Critical] IB/isert: race between isert_free_np() and isert_connect_error()
--

commit a36a7ac6c22cec858896a9509524018f8c67a5b1
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

IB/isert: do not publish connections being released on a second list

This commit changes isert_free_np() to take connections off
isert_np->pending and isert_np->accepted one at a time under
isert_np->mutex rather than moving them onto a local list, so that
isert_conn->node is empty when isert_np->mutex is dropped and
isert_handle_unbound_conn() does not unlink and release them a second time.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..5371bba71ba3c 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -2479,12 +2479,29 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
>  	return 0;
>  }
>  
> +static struct isert_conn *
> +isert_np_take_conn(struct isert_np *isert_np)
> +{
> +	struct isert_conn *isert_conn;
> +
> +	lockdep_assert_held(&isert_np->mutex);
> +
> +	isert_conn = list_first_entry_or_null(&isert_np->pending,
> +					      struct isert_conn, node);
> +	if (!isert_conn)
> +		isert_conn = list_first_entry_or_null(&isert_np->accepted,
> +						      struct isert_conn, node);
> +	if (isert_conn)
> +		list_del_init(&isert_conn->node);
> +
> +	return isert_conn;
> +}

[Severity: Critical]
Can isert_np_take_conn() race with isert_connect_error() when a connection on
isert_np->accepted fails concurrently with isert_free_np()?

In isert_free_np(), rdma_destroy_id(isert_np->cm_id) only destroys the
listening cm_id, leaving child connection cm_ids created by
isert_connect_request() active on isert_np->accepted with kref == 1.

If RDMA_CM_EVENT_REJECTED, RDMA_CM_EVENT_UNREACHABLE, or
RDMA_CM_EVENT_CONNECT_ERROR fires on a child cm_id,
cma_ib_handler()->isert_cma_handler()->isert_connect_error() runs under
id_priv->handler_mutex and calls ib_drain_qp() before taking isert_np->mutex:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_error() {
	struct isert_conn *isert_conn = cma_id->qp->qp_context;
	struct isert_np *isert_np = cma_id->context;

	ib_drain_qp(isert_conn->qp);

	mutex_lock(&isert_np->mutex);
	list_del_init(&isert_conn->node);
	mutex_unlock(&isert_np->mutex);
	isert_conn->cm_id = NULL;
	isert_put_conn(isert_conn);

	return -1;
}

If isert_free_np() takes that connection via isert_np_take_conn() while
isert_connect_error() is in ib_drain_qp(), isert_free_np() drops
isert_np->mutex and calls isert_connect_release(isert_conn), which blocks in
rdma_destroy_id(isert_conn->cm_id) waiting for id_priv->handler_mutex.

When ib_drain_qp() finishes, isert_connect_error() does not check
!list_empty(&isert_conn->node) under isert_np->mutex. Would it then call
isert_put_conn(isert_conn) (dropping kref from 1 to 0 and running
isert_connect_release(isert_conn) -> kfree(isert_conn)) and return -1 to
destroy the cm_id while isert_free_np() is concurrently releasing the same
isert_conn?

> +
>  static void
>  isert_free_np(struct iscsi_np *np)
>  {
>  	struct isert_np *isert_np = np->np_context;
> -	struct isert_conn *isert_conn, *n;
> -	LIST_HEAD(drop_conn_list);
> +	struct isert_conn *isert_conn;
>  
>  	if (isert_np->cm_id)
>  		rdma_destroy_id(isert_np->cm_id);
> @@ -2497,31 +2514,22 @@ isert_free_np(struct iscsi_np *np)
>  	 * up in accepted and pending lists.
>  	 */
>  	mutex_lock(&isert_np->mutex);
> -	if (!list_empty(&isert_np->pending)) {
> +	if (!list_empty(&isert_np->pending))
>  		isert_info("Still have isert pending connections\n");
> -		list_for_each_entry_safe(isert_conn, n,
> -					 &isert_np->pending,
> -					 node) {
> -			isert_info("cleaning isert_conn %p state (%d)\n",
> -				   isert_conn, isert_conn->state);
> -			list_move_tail(&isert_conn->node, &drop_conn_list);
> -		}
> -	}
> -
> -	if (!list_empty(&isert_np->accepted)) {
> +	if (!list_empty(&isert_np->accepted))
>  		isert_info("Still have isert accepted connections\n");
> -		list_for_each_entry_safe(isert_conn, n,
> -					 &isert_np->accepted,
> -					 node) {
> -			isert_info("cleaning isert_conn %p state (%d)\n",
> -				   isert_conn, isert_conn->state);
> -			list_move_tail(&isert_conn->node, &drop_conn_list);
> -		}
> -	}
>  	mutex_unlock(&isert_np->mutex);
>  
> -	list_for_each_entry_safe(isert_conn, n, &drop_conn_list, node) {
> -		list_del_init(&isert_conn->node);
> +	for (;;) {
> +		mutex_lock(&isert_np->mutex);
> +		isert_conn = isert_np_take_conn(isert_np);
> +		mutex_unlock(&isert_np->mutex);
> +
> +		if (!isert_conn)
> +			break;
> +
> +		isert_info("cleaning isert_conn %p state (%d)\n",
> +			   isert_conn, isert_conn->state);
>  		isert_connect_release(isert_conn);
>  	}

[Severity: Critical]
What happens if RDMA_CM_EVENT_ESTABLISHED fires on a connection just after
isert_np_take_conn() pops it from isert_np->accepted and drops
isert_np->mutex?

While isert_free_np()->isert_connect_release()->rdma_destroy_id() waits for
id_priv->handler_mutex, cma_ib_handler()->isert_cma_handler()->
isert_connected_handler() runs and moves isert_conn->node onto
isert_np->pending without checking !list_empty(&isert_conn->node):

drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
	...
	mutex_lock(&isert_np->mutex);
	list_move_tail(&isert_conn->node, &isert_np->pending);
	mutex_unlock(&isert_np->mutex);

	isert_info("np %p: Allow accept_np to continue\n", isert_np);
	up(&isert_np->sem);
}

Once isert_connected_handler() returns and drops id_priv->handler_mutex,
isert_connect_release() finishes and frees isert_conn via kfree(isert_conn)
while isert_conn->node is linked on isert_np->pending.

On the next iteration of the for (;;) loop in isert_free_np(), wouldn't
isert_np_take_conn() pop that already-freed isert_conn from isert_np->pending
and call isert_connect_release(isert_conn) a second time?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102061608.617186-1-yhlee@isslab.korea.ac.kr?part=1

Re: [PATCH v3 2/2] IB/isert: tear connections down from an ib_client on device removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] IB/isert: UAF and invalid reject on removing check in isert_connect_request()
- [High] IB/isert: hang and connection leak in isert_teardown_conn() on ISER_CONN_INIT
- [High] IB/isert: new isert_device and conn created after isert_remove_one() runs
- [High] IB/isert: ABBA deadlock calling ib_drain_qp() under isert_conn->mutex
--

commit 0865225bbb6b53b508af1b272b6ebb49c8aac4d5
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

IB/isert: tear connections down from an ib_client on device removal

This commit replaces the RDMA_CM_EVENT_DEVICE_REMOVAL wait and kfree with an
ib_client removal callback (isert_remove_one) that tears down active
connections and waits for the last isert_device reference to be released.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 5371bba71ba3c..223a727c1d0de 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
[ ... ]
> @@ -464,9 +470,19 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>  	if (ret)
>  		goto out_destroy_qp;
>  
> +	mutex_lock(&isert_conn_mutex);
> +	if (device->removing) {
> +		mutex_unlock(&isert_conn_mutex);
> +		isert_info("device %p is being removed, rejecting conn %p\n",
> +			   device, isert_conn);
> +		ret = -ENODEV;
> +		goto out_destroy_qp;
> +	}

[Severity: Critical]
By the time isert_connect_request() acquires isert_conn_mutex and checks
device->removing, it has already called isert_login_post_recv() and
isert_rdma_accept():

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_request() {
	...
	ret = isert_login_post_recv(isert_conn);
	if (ret)
		goto out_destroy_qp;

	ret = isert_rdma_accept(isert_conn);
	if (ret)
		goto out_destroy_qp;

	mutex_lock(&isert_conn_mutex);
	if (device->removing) {
		...
		ret = -ENODEV;
		goto out_destroy_qp;
	}
	...
out_destroy_qp:
	isert_destroy_qp(isert_conn);
out_rsp_dma_map:
	isert_free_login_buf(isert_conn);
out_conn_dev:
	isert_device_put(device);
out:
	kfree(isert_conn);
	rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
	return ret;
}

Because rdma_accept() has already transitioned isert_conn->qp to RTR/RTS,
jumping to out_destroy_qp destroys the QP and returns the shared
IB_POLL_WORKQUEUE CQ to the CQ pool via ib_cq_pool_put() without calling
ib_drain_qp(), then immediately frees login_desc, device, and isert_conn. If a
receive or flush CQE for the posted login_desc->rx_cqe was already queued on
the shared CQ, won't ib_cq_poll_work() invoke isert_login_recv_done() on the
freed login_desc, isert_conn, and device?

In addition, jumping to out after rdma_accept() succeeded calls rdma_reject()
and returns -ENODEV to the CMA REQ handler on an already-accepted cm_id. On
iWARP, iw_cm_accept() has already set IWCM_F_CONNECT_WAIT and taken a
reference on isert_conn->qp, which causes iw_destroy_cm_id() to hang waiting
for IWCM_F_CONNECT_WAIT to clear. Should the device->removing check happen
before isert_login_post_recv() and isert_rdma_accept() while holding a lock
that serializes against isert_remove_one()?

[ ... ]
> @@ -521,13 +537,25 @@ isert_connected_handler(struct rdma_cm_id *cma_id)
>  	isert_info("conn %p\n", isert_conn);
>  
>  	mutex_lock(&isert_conn->mutex);
> +	if (isert_conn->state >= ISER_CONN_TERMINATING) {
> +		/* The login receive is still posted, drain before releasing. */
> +		isert_info("conn %p is terminating, state %d\n",
> +			   isert_conn, isert_conn->state);
> +		if (rdma_disconnect(isert_conn->cm_id))
> +			isert_warn("Failed rdma_disconnect isert_conn %p\n",
> +				   isert_conn);
> +		ib_drain_qp(isert_conn->qp);
> +		isert_handle_unbound_conn(isert_conn, false);
> +		mutex_unlock(&isert_conn->mutex);
> +		return;
> +	}

[Severity: High]
Can calling ib_drain_qp(isert_conn->qp) while holding isert_conn->mutex here
(and in isert_teardown_conn() under the ISER_CONN_UP case) cause an ABBA
deadlock with the CQ completion workqueue?

In isert_create_qp(), isert_conn->cq is allocated from the shared pool with
IB_POLL_WORKQUEUE. If the initiator's login request PDU arrives before
rdma_disconnect() moves the QP to error, login_desc->rx_cqe completes with
IB_WC_SUCCESS ahead of the drain CQE on isert_conn->cq, and ib_cq_poll_work()
runs isert_login_recv_done():

drivers/infiniband/ulp/isert/ib_isert.c:isert_login_recv_done() {
	...
	mutex_lock(&isert_conn->mutex);
	complete(&isert_conn->login_req_comp);
	mutex_unlock(&isert_conn->mutex);
	...
}

Because isert_login_recv_done() blocks on isert_conn->mutex, ib_cq_poll_work()
cannot advance to the drain CQE that ib_drain_qp() is waiting on while holding
isert_conn->mutex (and isert_conn_mutex when called from isert_remove_one()).

[ ... ]
> @@ -625,20 +654,23 @@ isert_np_cma_handler(struct isert_np *isert_np,
>  	return -1;
>  }
>  
> -static int
> -isert_disconnected_handler(struct rdma_cm_id *cma_id,
> -			   enum rdma_cm_event_type event)
> +static void
> +isert_teardown_conn(struct isert_conn *isert_conn)
>  {
> -	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> +	lockdep_assert_held(&isert_conn->mutex);
>  
> -	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
> +	case ISER_CONN_DOWN:
> +		break;
> +	case ISER_CONN_INIT:
> +		/* Mark only; isert_connected_handler() releases it. */
> +		isert_conn->state = ISER_CONN_TERMINATING;
>  		break;

[Severity: High]
As reported by Sashiko on v2 [1], when a connection in ISER_CONN_INIT is torn
down in isert_teardown_conn(), it is only marked ISER_CONN_TERMINATING and
relies on isert_connected_handler() (RDMA_CM_EVENT_ESTABLISHED) to disconnect,
drain the QP, and schedule release_work. What happens if
RDMA_CM_EVENT_ESTABLISHED never arrives?

For example, if the peer disconnects after CM REQ/REP without sending RTU (or
closes the connection before establishment on iWARP), cma_ib_handler() /
cma_iw_handler() delivers RDMA_CM_EVENT_DISCONNECTED, which calls
isert_disconnected_handler() -> isert_teardown_conn(). That transitions
isert_conn->state to ISER_CONN_TERMINATING without scheduling cleanup, and
since the CM state is now disconnecting, neither RDMA_CM_EVENT_ESTABLISHED nor
RDMA_CM_EVENT_UNREACHABLE will ever fire. Similarly, if isert_remove_one()
runs while a connection is in ISER_CONN_INIT and the peer never sends RTU,
isert_remove_one() runs before cma_remove_one() and cm_remove_one() in the
ib_client removal order. Won't that leak isert_conn and its isert_device
reference and cause isert_remove_one() to hang indefinitely in
wait_for_completion(&removal_comp)?

[ ... ]
> @@ -2697,8 +2727,54 @@ static struct iscsit_transport iser_target_transport = {
>  	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
>  };
>  
> +static void
> +isert_remove_one(struct ib_device *ib_device, void *client_data)
> +{
> +	DECLARE_COMPLETION_ONSTACK(removal_comp);
> +	struct isert_device *device, *found = NULL;
> +	struct isert_conn *isert_conn;
> +
> +	mutex_lock(&device_list_mutex);
> +	list_for_each_entry(device, &device_list, dev_node) {
> +		if (device->ib_device != ib_device)
> +			continue;
> +		device->refcount++;
> +		device->removal_comp = &removal_comp;
> +		found = device;
> +		break;
> +	}
> +	mutex_unlock(&device_list_mutex);
> +
> +	if (!found)
> +		return;
> +
> +	mutex_lock(&isert_conn_mutex);
> +	found->removing = true;
> +	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> +		if (isert_conn->device != found)
> +			continue;
> +
> +		isert_info("removing conn %p state %d\n",
> +			   isert_conn, isert_conn->state);
> +		mutex_lock(&isert_conn->mutex);
> +		isert_teardown_conn(isert_conn);
> +		mutex_unlock(&isert_conn->mutex);
> +	}
> +	mutex_unlock(&isert_conn_mutex);
> +
> +	isert_device_put(found);
> +	wait_for_completion(&removal_comp);
> +}

[Severity: High]
As reported by Sashiko on v2 [2], during IB device unregistration,
disable_device() invokes ib_client remove callbacks in reverse registration
order, so isert_remove_one() runs and returns before cma_remove_one() destroys
listening rdma_cm_ids.

Because struct isert_device entries only exist on device_list while
device->refcount > 0, if no connections exist when isert_remove_one() runs
(found == NULL), or after wait_for_completion(&removal_comp) returns and found
has been removed from device_list and freed by isert_device_put(), there is no
record that ib_device is being removed.

If an RDMA_CM_EVENT_CONNECT_REQUEST arrives on a still-active listener before
cma_remove_one() runs, isert_connect_request() -> isert_device_get() will
allocate a fresh struct isert_device with removing == false and accept the new
connection in ISER_CONN_INIT after isert_remove_one() has already finished.
When cma_remove_one() -> cma_process_remove() subsequently delivers
RDMA_CM_EVENT_DEVICE_REMOVAL to that connection, isert_cma_handler() returns 0
and isert_teardown_conn() only sets state = ISER_CONN_TERMINATING while
id_priv->state is set to RDMA_CM_DEVICE_REMOVAL (suppressing any further CM
events). Won't cma_process_remove() then block forever waiting for that cm_id
to be destroyed?

[1] https://lore.kernel.org/r/20261026060505.409476-1-yhlee@isslab.korea.ac.kr
[2] https://lore.kernel.org/r/20261026060505.409476-1-yhlee@isslab.korea.ac.kr

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102061608.617186-1-yhlee@isslab.korea.ac.kr?part=2