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

[PATCH v4 0/3] 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.  3/3
moves the teardown into an ib_client callback, as nvmet-rdma did in commit
f1d4ef7d8883.  1/3 is a separate problem that 3/3 adds a caller to, and
2/3 is an older deadlock that 3/3 would otherwise inherit.

The design is one owner per connection: publishing happens in the same
isert_conn_mutex section that tests isert_device->removing, and whoever
empties isert_conn->node under isert_np->mutex owns the release.

v3 -> v4

v3: https://lore.kernel.org/r/20261102061608.617186-1-yhlee@isslab.korea.ac.kr

Review of v3 raised six problems, all real; the first was a regression.

The refusal path in isert_connect_request() was that regression: v3 tested
removing after isert_rdma_accept() had run, so it destroyed an accepted QP
without draining it and rejected an id it had accepted.  v4 tests removing
before the accept and rejects there; if the test at the publish point
catches it, v4 drains the QP and returns an error so cma destroys the id.

Draining under isert_conn->mutex was the second, and not v3's doing:
mainline has done it since commit 6d1fba0c2cc7 ("iser-target: Rework
connection termination").  2/3 fixes it, and I split the drain fix out
with its own Fixes tag so it can reach stable without the rest.

The connection left in ISER_CONN_INIT was the third.  v3 marked it and
waited for a CM event that never comes; in v4 the removal walk owns it.

The trigger review named there, a DREQ before the RTU, needs the RTU lost
after the initiator sent it; a test-only drop left eight connections
unreleased on v3 with the removal never finishing, and none on v4.

The fourth and fifth were races on the np lists, both in 1/3.  The CM
handlers test isert_conn->node under isert_np->mutex and stand back when
it is empty, and connect_error returns zero there.

The sixth, a CONNECT_REQUEST after isert_remove_one() returns, is still
open: a test-only delay showed a new isert_device torn down cleanly by
cma's DEVICE_REMOVAL, but nothing orders that against device release.

One change is mine: the teardown now runs outside isert_conn_mutex, which
let a connection outlive its portal, so isert_np is reference counted.

With soft-RoCE, three v3 against three v4 runs per case: accept and INIT
went from a removal that never finished to one finished in seconds, drain
from six stuck drains to none, np list from 32 use after frees to none.
2/3 alone turned ten hung task warnings a run into none, three runs each.
Under KASAN one of twelve unpatched runs finished the removal and two
reported the use-after-free, 17 and 11 times; all twelve patched runs
finished and reported nothing.  Making rdma_accept() fail on every fourth
connection refused 48 connections over three runs with no KASAN report,
and every removal finished.  Login and I/O ran 18 times a side with no
difference and PROVE_LOCKING gave no splat in eight runs.

Yehyeong Lee (3):
  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

 drivers/infiniband/ulp/isert/ib_isert.c | 310 ++++++++++++++++++------
 drivers/infiniband/ulp/isert/ib_isert.h |   9 +-
 2 files changed, 240 insertions(+), 79 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

[PATCH v4 1/3] 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 and releases them from there,
so isert_conn->node is not empty once isert_np->mutex is dropped.  That
emptiness is how the CM handlers decide whether a connection is still
theirs, so isert_handle_unbound_conn() and isert_connect_error() unlink
it from a list they do not own and release it a second time, and
isert_connected_handler() moves it back onto isert_np->pending.

Take the connections off isert_np one at a time, so that the node is
empty once the mutex is dropped, and release them by dropping references
instead of calling isert_connect_release() directly.  The two CM handlers
test the node under isert_np->mutex and stand back when it is empty;
connect_error also returns zero there, so free_np's rdma_destroy_id()
destroys the cm_id rather than cma destroying it as well.  Whoever
empties the node owns the release, which still runs with no mutex held,
as commit 691b0480933f ("IB/isert: Fix dead lock in ib_isert") needs.

Reproduced with a test-only delay in 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 | 84 ++++++++++++++++---------
 1 file changed, 55 insertions(+), 29 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index efa8d7b7844c97..8255208e30c7d6 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -521,13 +521,18 @@ isert_connected_handler(struct rdma_cm_id *cma_id)
 	isert_info("conn %p\n", isert_conn);
 
 	mutex_lock(&isert_conn->mutex);
+	mutex_lock(&isert_np->mutex);
+	if (list_empty(&isert_conn->node)) {
+		mutex_unlock(&isert_np->mutex);
+		mutex_unlock(&isert_conn->mutex);
+		isert_info("conn %p is being released already\n", isert_conn);
+		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);
@@ -658,12 +663,19 @@ isert_connect_error(struct rdma_cm_id *cma_id)
 {
 	struct isert_conn *isert_conn = cma_id->qp->qp_context;
 	struct isert_np *isert_np = cma_id->context;
-
-	ib_drain_qp(isert_conn->qp);
+	bool owned;
 
 	mutex_lock(&isert_np->mutex);
+	owned = !list_empty(&isert_conn->node);
 	list_del_init(&isert_conn->node);
 	mutex_unlock(&isert_np->mutex);
+
+	if (!owned) {
+		isert_info("conn %p is being released already\n", isert_conn);
+		return 0;
+	}
+
+	ib_drain_qp(isert_conn->qp);
 	isert_conn->cm_id = NULL;
 	isert_put_conn(isert_conn);
 
@@ -2479,12 +2491,33 @@ 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, bool *promoted)
+{
+	struct isert_conn *isert_conn;
+
+	lockdep_assert_held(&isert_np->mutex);
+
+	*promoted = true;
+	isert_conn = list_first_entry_or_null(&isert_np->pending,
+					      struct isert_conn, node);
+	if (!isert_conn) {
+		*promoted = false;
+		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;
+	bool promoted;
 
 	if (isert_np->cm_id)
 		rdma_destroy_id(isert_np->cm_id);
@@ -2497,32 +2530,25 @@ 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);
-		isert_connect_release(isert_conn);
+	for (;;) {
+		mutex_lock(&isert_np->mutex);
+		isert_conn = isert_np_take_conn(isert_np, &promoted);
+		mutex_unlock(&isert_np->mutex);
+
+		if (!isert_conn)
+			break;
+
+		isert_info("cleaning isert_conn %p state (%d)\n",
+			   isert_conn, isert_conn->state);
+		if (promoted)
+			isert_put_conn(isert_conn);
+		isert_put_conn(isert_conn);
 	}
 
 	np->np_context = NULL;
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] IB/isert: missing ib_drain_qp() in isert_free_np() and isert_connect_error()
--

commit 56a817f50afa9a4dae171633af8e54e1c447db9f
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 unlink connections from
isert_np->pending and isert_np->accepted one at a time rather than moving
them onto a local list, and updates the CM handlers to check whether
isert_conn->node is already empty under isert_np->mutex before acting on
the connection.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..8255208e30c7d 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -658,12 +663,19 @@ isert_connect_error(struct rdma_cm_id *cma_id)
>  {
>  	struct isert_conn *isert_conn = cma_id->qp->qp_context;
>  	struct isert_np *isert_np = cma_id->context;
> -
> -	ib_drain_qp(isert_conn->qp);
> +	bool owned;
>  
>  	mutex_lock(&isert_np->mutex);
> +	owned = !list_empty(&isert_conn->node);
>  	list_del_init(&isert_conn->node);
>  	mutex_unlock(&isert_np->mutex);
> +
> +	if (!owned) {
> +		isert_info("conn %p is being released already\n", isert_conn);
> +		return 0;
> +	}
> +
> +	ib_drain_qp(isert_conn->qp);
>  	isert_conn->cm_id = NULL;
>  	isert_put_conn(isert_conn);
>  
> @@ -2479,12 +2491,33 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)

[ ... ]

> @@ -2497,32 +2530,25 @@ isert_free_np(struct iscsi_np *np)

[ ... ]

> +	for (;;) {
> +		mutex_lock(&isert_np->mutex);
> +		isert_conn = isert_np_take_conn(isert_np, &promoted);
> +		mutex_unlock(&isert_np->mutex);
> +
> +		if (!isert_conn)
> +			break;
> +
> +		isert_info("cleaning isert_conn %p state (%d)\n",
> +			   isert_conn, isert_conn->state);
> +		if (promoted)
> +			isert_put_conn(isert_conn);
> +		isert_put_conn(isert_conn);
>  	}

[Severity: Critical]
Can skipping ib_drain_qp(isert_conn->qp) when isert_free_np() takes ownership
of the connection cause a use-after-free in isert_login_recv_done()?

During RDMA_CM_EVENT_CONNECT_REQUEST, isert_connect_request() allocates a QP
using a shared workqueue-polled CQ from ib_cq_pool_get(..., IB_POLL_WORKQUEUE),
posts the login receive WR with wr_cqe = &isert_conn->login_desc->rx_cqe
(callback isert_login_recv_done) via isert_login_post_recv(), and transitions
the QP to RTS in isert_rdma_accept() before adding isert_conn->node to
isert_np->accepted.

When an IB_CM_REJ_RECEIVED event (or a cma_rep_recv() failure) occurs,
cma_ib_handler() calls cma_modify_qp_err() before invoking
cma_cm_event_handler() -> isert_cma_handler() -> isert_connect_error(). That
QP state transition flushes the pre-posted login receive WR into the shared CQ
and queues cq->work (ib_cq_poll_work). Likewise, for a connection on
isert_np->accepted or isert_np->pending, the remote initiator may already have
sent a login PDU that completed into the shared CQ.

Previously, isert_connect_error() unconditionally called
ib_drain_qp(isert_conn->qp) at the start of the function (added in commit
cd52cb26e7ea ("IB/isert: Fix use-after-free in isert_connect_error")) before
taking isert_np->mutex. With this change, if isert_free_np() unlinks
isert_conn->node via isert_np_take_conn() first, isert_connect_error() sees
!owned and returns 0 without draining the QP, and neither isert_free_np() nor
isert_connect_release() calls ib_drain_qp(isert_conn->qp):

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_release() {
	...
	if (isert_conn->cm_id &&
	    !isert_conn->dev_removed)
		rdma_destroy_id(isert_conn->cm_id);

	if (isert_conn->qp)
		isert_destroy_qp(isert_conn);

	isert_unmap_tx_desc(&isert_conn->login_tx_desc, device->ib_device);

	if (isert_conn->login_desc)
		isert_free_login_buf(isert_conn);
	...
	kfree(isert_conn);
}

Because isert_destroy_qp() releases the shared CQ via ib_cq_pool_put(), which
only decrements cq->cqe_used without canceling or flushing cq->work, can
ib_cq_poll_work concurrently invoke isert_login_recv_done() and dereference
wc->qp->qp_context, isert_conn->device, isert_conn->login_desc, and
isert_conn->mutex after isert_connect_release() has already freed them?

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

[PATCH v4 2/3] IB/isert: drain the QP without holding isert_conn->mutex

[Yehyeong Lee]

isert_disconnected_handler() drains the QP with isert_conn->mutex held.
The drain waits for the flush completions of the send and receive
queues, and those are reaped by ib_cq_poll_work() on ib-comp-wq, where
isert_login_recv_done() takes isert_conn->mutex before it looks at the
connection state.  If a login receive completes while the handler holds
the mutex, the completion handler blocks on the mutex and the drain
blocks on the completion that handler would have reaped.

Return from isert_conn_terminate() whether this caller moved the
connection to ISER_CONN_TERMINATING, and do the drain and the unbound
connection release after dropping isert_conn->mutex.  Only one caller
sees true for a given connection, so the drain still runs once.

Two test-only delay knobs open the window, one in
isert_login_recv_done() and one before the drain; it does not open on
its own in this setup.  With them, three runs of 7.4-rc1 plus patch 1 of
this series produced ten hung task warnings each, six, six and seven of
them waiting in ib_drain_qp() and four, four and three in
isert_login_recv_done().  Three runs with this patch produced none.

  INFO: task kworker/0:1:10 blocked for more than 10 seconds.
        Not tainted 7.4.0-rc1-B13-00002-g093ce9c311ad #47
  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  task:kworker/0:1     state:D stack:27392 pid:10    tgid:10    ppid:2      task_flags:0x4208060 flags:0x00080000
  Workqueue: ib_cm cm_work_handler
  Call Trace:
   <TASK>
   __schedule+0x180f/0x4bb0
   ? _raw_spin_lock_irq+0x1/0xe0
   ? __pfx___schedule+0x10/0x10
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   schedule+0x70/0x1f0
   schedule_timeout+0x1cd/0x200
   ? __pfx_schedule_timeout+0x10/0x10
   ? rxe_qp_from_attr+0x1497/0x18c0
   ? __pfx__raw_spin_lock_irq+0x10/0x10
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? _raw_spin_lock_irqsave+0x86/0xe0
   __wait_for_common+0x29e/0x420
   ? __pfx_schedule_timeout+0x10/0x10
   ? __pfx___wait_for_common+0x10/0x10
   __ib_drain_sq+0x251/0x340
   ? __pfx___ib_drain_sq+0x10/0x10
   ? __pfx_ib_drain_qp_done+0x10/0x10
   ? irq_work_queue+0x1c/0xb0
   ? __pfx__printk+0x10/0x10
   ? __pfx_process_timeout+0x10/0x10
   ib_drain_sq+0x146/0x1a0
   ib_drain_qp+0xd/0x50
   isert_disconnected_handler.isra.0+0xc4/0x3f0
   isert_cma_handler+0xd74/0x27f0
   ? ip6_finish_output2+0xc09/0x1bd0
   ? __pfx_isert_cma_handler+0x10/0x10
   ? __pfx_ip6_finish_output2+0x10/0x10
   ? _raw_spin_lock_irqsave+0x86/0xe0
   cma_cm_event_handler+0x3e/0x240
   cma_ib_handler+0x190/0x750
   ? __pfx_cma_ib_handler+0x10/0x10
   cm_process_work+0x41/0x330
   cm_work_handler+0x582b/0xc160
   ? _raw_spin_lock_irqsave+0x86/0xe0
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? hrtimer_start_range_ns+0xa6/0x2c0
   ? __pfx_cm_work_handler+0x10/0x10
   ? finish_task_switch.isra.0+0x1d9/0xa70
   process_one_work+0x697/0x1060
   ? __pfx___schedule+0x10/0x10
   ? __pfx_process_one_work+0x10/0x10
   ? _raw_spin_lock_irq+0x81/0xe0
   ? __pfx_cm_work_handler+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>
  INFO: task kworker/0:0H:11 blocked for more than 10 seconds.
        Not tainted 7.4.0-rc1-B13-00002-g093ce9c311ad #47
  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  task:kworker/0:0H    state:D stack:29592 pid:11    tgid:11    ppid:2      task_flags:0x4208060 flags:0x00080000
  Workqueue: ib-comp-wq ib_cq_poll_work
  Call Trace:
   <TASK>
   __schedule+0x180f/0x4bb0
   ? _raw_spin_lock_irq+0x1/0xe0
   ? __pfx___schedule+0x10/0x10
   ? _raw_spin_lock+0x80/0xe0
   schedule+0x70/0x1f0
   schedule_preempt_disabled+0x11/0x20
   __mutex_lock.constprop.0+0xc2f/0x1910
   ? __pfx___mutex_lock.constprop.0+0x10/0x10
   ? _printk+0xba/0xf0
   mutex_lock+0xc2/0xd0
   ? __pfx_mutex_lock+0x10/0x10
   ? _raw_spin_lock_irqsave+0x86/0xe0
   isert_login_recv_done+0x2b9/0x590
   __ib_process_cq+0xe1/0x390
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ib_cq_poll_work+0x46/0x150
   process_one_work+0x697/0x1060
   ? __pfx_process_one_work+0x10/0x10
   ? _raw_spin_lock_irq+0x81/0xe0
   ? __pfx_ib_cq_poll_work+0x10/0x10
   ? assign_work+0x11d/0x370
   worker_thread+0x45b/0xd10
   ? __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>
  INFO: task kworker/0:0H:11 is blocked on a mutex likely owned by task kworker/3:0:34.

Fixes: 6d1fba0c2cc7 ("iser-target: Rework connection termination")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 26 +++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 8255208e30c7d6..c6f417bb206901 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -578,21 +578,17 @@ isert_handle_unbound_conn(struct isert_conn *isert_conn)
  * isert_conn_terminate() - Initiate connection termination
  * @isert_conn: isert connection struct
  *
- * Notes:
- * In case the connection state is BOUND, move state
- * to TEMINATING and start teardown sequence (rdma_disconnect).
- * In case the connection state is UP, complete flush as well.
- *
- * This routine must be called with mutex held. Thus it is
- * safe to call multiple times.
+ * Returns true if this caller moved the connection to ISER_CONN_TERMINATING.
  */
-static void
+static bool
 isert_conn_terminate(struct isert_conn *isert_conn)
 {
 	int err;
 
+	lockdep_assert_held(&isert_conn->mutex);
+
 	if (isert_conn->state >= ISER_CONN_TERMINATING)
-		return;
+		return false;
 
 	isert_info("Terminating conn %p state %d\n",
 		   isert_conn, isert_conn->state);
@@ -601,6 +597,8 @@ isert_conn_terminate(struct isert_conn *isert_conn)
 	if (err)
 		isert_warn("Failed rdma_disconnect isert_conn %p\n",
 			   isert_conn);
+
+	return true;
 }
 
 static int
@@ -635,15 +633,14 @@ 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;
+	bool terminated = false;
 
 	mutex_lock(&isert_conn->mutex);
 	switch (isert_conn->state) {
 	case 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);
+		terminated = isert_conn_terminate(isert_conn);
 		break;
 	case ISER_CONN_BOUND:
 	case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
@@ -655,6 +652,11 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
 	}
 	mutex_unlock(&isert_conn->mutex);
 
+	if (terminated) {
+		ib_drain_qp(isert_conn->qp);
+		isert_handle_unbound_conn(isert_conn);
+	}
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v4 2/3] IB/isert: drain the QP without holding isert_conn->mutex

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] IB/isert: race with isert_accept_np() when draining QP unlocked
- [Low] IB/isert: missing colon in Return section of isert_conn_terminate() kernel-doc
--

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

IB/isert: drain the QP without holding isert_conn->mutex

This commit changes isert_conn_terminate() to return whether it moved the
connection to ISER_CONN_TERMINATING, and moves ib_drain_qp() and
isert_handle_unbound_conn() outside isert_conn->mutex in
isert_disconnected_handler() to avoid deadlocking with
isert_login_recv_done().

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 8255208e30c7d..c6f417bb20690 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -578,21 +578,17 @@ isert_handle_unbound_conn(struct isert_conn *isert_conn)
>   * isert_conn_terminate() - Initiate connection termination
>   * @isert_conn: isert connection struct
>   *
> - * Notes:
> - * In case the connection state is BOUND, move state
> - * to TEMINATING and start teardown sequence (rdma_disconnect).
> - * In case the connection state is UP, complete flush as well.
> - *
> - * This routine must be called with mutex held. Thus it is
> - * safe to call multiple times.
> + * Returns true if this caller moved the connection to ISER_CONN_TERMINATING.
>   */

[Severity: Low]
This isn't a bug, but kernel-doc requires a colon after Returns (Returns:)
to recognize the return value section when building with W=2.

> -static void
> +static bool
>  isert_conn_terminate(struct isert_conn *isert_conn)
>  {
>  	int err;
>  
> +	lockdep_assert_held(&isert_conn->mutex);
> +
>  	if (isert_conn->state >= ISER_CONN_TERMINATING)
> -		return;
> +		return false;
>  
>  	isert_info("Terminating conn %p state %d\n",
>  		   isert_conn, isert_conn->state);
> @@ -601,6 +597,8 @@ isert_conn_terminate(struct isert_conn *isert_conn)
>  	if (err)
>  		isert_warn("Failed rdma_disconnect isert_conn %p\n",
>  			   isert_conn);
> +
> +	return true;
>  }
>  
>  static int
> @@ -635,15 +633,14 @@ 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;
> +	bool terminated = false;
>  
>  	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case 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);
> +		terminated = isert_conn_terminate(isert_conn);
>  		break;
>  	case ISER_CONN_BOUND:
>  	case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
> @@ -655,6 +652,11 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
>  	}
>  	mutex_unlock(&isert_conn->mutex);
>  
> +	if (terminated) {
> +		ib_drain_qp(isert_conn->qp);
> +		isert_handle_unbound_conn(isert_conn);
> +	}

[Severity: High]
Can dropping isert_conn->mutex before ib_drain_qp() and
isert_handle_unbound_conn() race with isert_accept_np()?

When a connection reaches isert_connected_handler(), its state is set to
ISER_CONN_UP, isert_conn->node is placed on isert_np->pending, and
up(&isert_np->sem) wakes the login thread in isert_accept_np().

If a disconnect arrives while the connection is still in ISER_CONN_UP,
isert_disconnected_handler() sets isert_conn->state to
ISER_CONN_TERMINATING and now unlocks isert_conn->mutex before removing
isert_conn->node from isert_np->pending in isert_handle_unbound_conn().

While ib_drain_qp() is waiting in isert_disconnected_handler(),
isert_login_recv_done() can complete isert_conn->login_req_comp for a login
PDU received just before the disconnect, and isert_accept_np() can dequeue
isert_conn from isert_np->pending and overwrite isert_conn->state without
holding isert_conn->mutex:

drivers/infiniband/ulp/isert/ib_isert.c:isert_accept_np() {
	...
	isert_conn = list_first_entry(&isert_np->pending,
			struct isert_conn, node);
	list_del_init(&isert_conn->node);
	mutex_unlock(&isert_np->mutex);

	conn->context = isert_conn;
	isert_conn->conn = conn;
	isert_conn->state = ISER_CONN_BOUND;
	...
}

Once list_del_init(&isert_conn->node) has run in isert_accept_np(),
isert_handle_unbound_conn() sees list_empty(&isert_conn->node) and becomes
a no-op.

If login then fails or times out, iscsi_target_login_sess_out() calls
isert_wait_conn() and isert_free_conn():

drivers/infiniband/ulp/isert/ib_isert.c:isert_wait_conn() {
	...
	mutex_lock(&isert_conn->mutex);
	isert_conn_terminate(isert_conn);
	mutex_unlock(&isert_conn->mutex);

	ib_drain_qp(isert_conn->qp);
	isert_put_conn(isert_conn);
	queue_work(isert_release_wq, &isert_conn->release_work);
}

Because isert_conn->state was reset to ISER_CONN_BOUND and
isert_conn->mutex is no longer held across ib_drain_qp(), wouldn't
isert_wait_conn() call ib_drain_qp(isert_conn->qp) a second time
concurrently with isert_disconnected_handler(), and allow both kref
references on isert_conn to be dropped while isert_disconnected_handler()
is still running?

Alternatively, if single-stage login succeeds in memory without posting to
the QP (isert_put_login_tx() sets isert_conn->login_rsp_pending = true),
wouldn't isert_get_rx_pdu() set isert_conn->state to ISER_CONN_FULL_FEATURE,
post to the already-disconnected QP, and block indefinitely in
wait_for_completion_interruptible(&comp) because the disconnect event was
already handled?

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

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

[Yehyeong Lee]

The RDMA_CM_EVENT_DEVICE_REMOVAL callback waits for isert_conn->state to
reach ISER_CONN_DOWN and then calls kfree(isert_conn) itself.
isert_release_work() publishes that state before dropping the last
reference, so the wait returns while the work is on its way into
kref_put().  It also deadlocks: the callback runs under the cm_id's
handler_mutex, which rdma_destroy_id() needs.

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, as commit f1d4ef7d8883
("nvmet-rdma: register ib_client to not deadlock in device removal")
did.  That walk runs without the handler_mutex that used to serialise a
teardown against isert_free_np(), so the portal is reference counted
too.

Without this series, one of twelve runs finished the removal and two
reported this, seventeen times and eleven times; with it, all twelve
finished and reported nothing.

  BUG: KASAN: slab-use-after-free in isert_connect_release+0x5c1/0x620
  Read of size 8 at addr ffff88810bca1160 by task kworker/u16:4/69

  CPU: 3 UID: 0 PID: 69 Comm: kworker/u16:4 Tainted: G    B               7.4.0-rc1-CTL #19 PREEMPT(full)
  Tainted: [B]=BAD_PAGE
  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
   ? isert_connect_release+0x5c1/0x620
   kasan_report+0xce/0x100
   ? isert_connect_release+0x5c1/0x620
   isert_connect_release+0x5c1/0x620
   process_one_work+0x697/0x1060
   ? __pfx_process_one_work+0x10/0x10
   ? _raw_spin_lock_irq+0xcb/0xe0
   ? __pfx_isert_release_work+0x10/0x10
   ? assign_work+0x11d/0x370
   worker_thread+0x45b/0xd10
   ? __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 219:
   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 ffff88810bca0000
   which belongs to the cache kmalloc-8k of size 8192
  The buggy address is located 4448 bytes inside of
   freed 8192-byte region [ffff88810bca0000, ffff88810bca2000)

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 | 232 ++++++++++++++++++------
 drivers/infiniband/ulp/isert/ib_isert.h |   9 +-
 2 files changed, 187 insertions(+), 54 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index c6f417bb206901..5486fbdc6aa394 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,10 @@ 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_put_np(struct isert_np *isert_np);
+static void
+isert_teardown_conn(struct isert_conn *isert_conn);
 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 +261,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 +317,8 @@ 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);
+	INIT_LIST_HEAD(&isert_conn->removal_node);
 	kref_init(&isert_conn->kref);
 	atomic_set(&isert_conn->ctrl_comp_cnt, 0);
 	mutex_init(&isert_conn->mutex);
@@ -417,6 +426,7 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 	struct iscsi_np *np = isert_np->np;
 	struct isert_conn *isert_conn;
 	struct isert_device *device;
+	bool accepted = false;
 	int ret = 0;
 
 	spin_lock_bh(&np->np_thread_lock);
@@ -456,20 +466,42 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 		goto out_rsp_dma_map;
 	}
 
+	if (READ_ONCE(device->removing)) {
+		isert_info("device %p is being removed, rejecting conn %p\n",
+			   device, isert_conn);
+		ret = -ENODEV;
+		goto out_destroy_qp;
+	}
+
 	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;
-
+		goto out_drain_qp;
+	accepted = true;
+
+	mutex_lock(&isert_conn_mutex);
+	if (device->removing) {
+		mutex_unlock(&isert_conn_mutex);
+		isert_info("device %p is being removed, dropping conn %p\n",
+			   device, isert_conn);
+		ret = -ENODEV;
+		goto out_drain_qp;
+	}
+	isert_conn->np = isert_np;
+	kref_get(&isert_np->kref);
+	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;
 
+out_drain_qp:
+	ib_drain_qp(isert_conn->qp);
 out_destroy_qp:
 	isert_destroy_qp(isert_conn);
 out_rsp_dma_map:
@@ -478,7 +510,9 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 	isert_device_put(device);
 out:
 	kfree(isert_conn);
-	rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
+	/* cma destroys an accepted cm id when this handler returns an error. */
+	if (!accepted)
+		rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
 	return ret;
 }
 
@@ -486,14 +520,18 @@ static void
 isert_connect_release(struct isert_conn *isert_conn)
 {
 	struct isert_device *device = isert_conn->device;
+	struct isert_np *isert_np = isert_conn->np;
 
 	isert_dbg("conn %p\n", 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 +542,11 @@ 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);
+	if (isert_np)
+		isert_put_np(isert_np);
+	isert_device_put(device);
 }
 
 static void
@@ -517,22 +554,37 @@ isert_connected_handler(struct rdma_cm_id *cma_id)
 {
 	struct isert_conn *isert_conn = cma_id->qp->qp_context;
 	struct isert_np *isert_np = cma_id->context;
+	bool owned, promote;
 
 	isert_info("conn %p\n", isert_conn);
 
+	mutex_lock(&isert_conn_mutex);
 	mutex_lock(&isert_conn->mutex);
 	mutex_lock(&isert_np->mutex);
-	if (list_empty(&isert_conn->node)) {
-		mutex_unlock(&isert_np->mutex);
-		mutex_unlock(&isert_conn->mutex);
-		isert_info("conn %p is being released already\n", isert_conn);
-		return;
+	owned = !list_empty(&isert_conn->node);
+	promote = owned && isert_conn->state < ISER_CONN_TERMINATING &&
+		  !isert_conn->device->removing;
+	if (promote) {
+		isert_conn->state = ISER_CONN_UP;
+		kref_get(&isert_conn->kref);
+		isert_conn->promoted = true;
+		list_move_tail(&isert_conn->node, &isert_np->pending);
 	}
-	isert_conn->state = ISER_CONN_UP;
-	kref_get(&isert_conn->kref);
-	list_move_tail(&isert_conn->node, &isert_np->pending);
 	mutex_unlock(&isert_np->mutex);
 	mutex_unlock(&isert_conn->mutex);
+	mutex_unlock(&isert_conn_mutex);
+
+	if (!owned) {
+		isert_info("conn %p is being released already\n", isert_conn);
+		return;
+	}
+
+	if (!promote) {
+		isert_info("conn %p is not promoted, state %d\n",
+			   isert_conn, isert_conn->state);
+		isert_teardown_conn(isert_conn);
+		return;
+	}
 
 	isert_info("np %p: Allow accept_np to continue\n", isert_np);
 	up(&isert_np->sem);
@@ -556,10 +608,24 @@ isert_put_conn(struct isert_conn *isert_conn)
 	kref_put(&isert_conn->kref, isert_release_kref);
 }
 
+static void
+isert_release_np_kref(struct kref *kref)
+{
+	struct isert_np *isert_np = container_of(kref, struct isert_np, kref);
+
+	kfree(isert_np);
+}
+
+static void
+isert_put_np(struct isert_np *isert_np)
+{
+	kref_put(&isert_np->kref, isert_release_np_kref);
+}
+
 static void
 isert_handle_unbound_conn(struct isert_conn *isert_conn)
 {
-	struct isert_np *isert_np = isert_conn->cm_id->context;
+	struct isert_np *isert_np = isert_conn->np;
 
 	mutex_lock(&isert_np->mutex);
 	if (!list_empty(&isert_conn->node)) {
@@ -568,7 +634,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 (isert_conn->promoted)
+			isert_put_conn(isert_conn);
 		queue_work(isert_release_wq, &isert_conn->release_work);
 	}
 	mutex_unlock(&isert_np->mutex);
@@ -628,17 +695,17 @@ 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;
 	bool terminated = false;
 
 	mutex_lock(&isert_conn->mutex);
 	switch (isert_conn->state) {
 	case ISER_CONN_TERMINATING:
+	case ISER_CONN_DOWN:
 		break;
+	case ISER_CONN_INIT:
 	case ISER_CONN_UP:
 		terminated = isert_conn_terminate(isert_conn);
 		break;
@@ -652,10 +719,20 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
 	}
 	mutex_unlock(&isert_conn->mutex);
 
-	if (terminated) {
-		ib_drain_qp(isert_conn->qp);
-		isert_handle_unbound_conn(isert_conn);
-	}
+	if (!terminated)
+		return;
+
+	ib_drain_qp(isert_conn->qp);
+	isert_handle_unbound_conn(isert_conn);
+}
+
+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;
+
+	isert_teardown_conn(isert_conn);
 
 	return 0;
 }
@@ -688,7 +765,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",
@@ -709,21 +785,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));
@@ -2335,6 +2400,7 @@ isert_setup_np(struct iscsi_np *np,
 	if (!isert_np)
 		return -ENOMEM;
 
+	kref_init(&isert_np->kref);
 	sema_init(&isert_np->sem, 0);
 	mutex_init(&isert_np->mutex);
 	INIT_LIST_HEAD(&isert_np->accepted);
@@ -2360,7 +2426,7 @@ isert_setup_np(struct iscsi_np *np,
 	return 0;
 
 out:
-	kfree(isert_np);
+	isert_put_np(isert_np);
 
 	return ret;
 }
@@ -2480,6 +2546,7 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
 	isert_conn = list_first_entry(&isert_np->pending,
 			struct isert_conn, node);
 	list_del_init(&isert_conn->node);
+	isert_conn->promoted = false;
 	mutex_unlock(&isert_np->mutex);
 
 	conn->context = isert_conn;
@@ -2494,20 +2561,17 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
 }
 
 static struct isert_conn *
-isert_np_take_conn(struct isert_np *isert_np, bool *promoted)
+isert_np_take_conn(struct isert_np *isert_np)
 {
 	struct isert_conn *isert_conn;
 
 	lockdep_assert_held(&isert_np->mutex);
 
-	*promoted = true;
 	isert_conn = list_first_entry_or_null(&isert_np->pending,
 					      struct isert_conn, node);
-	if (!isert_conn) {
-		*promoted = false;
+	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);
 
@@ -2540,7 +2604,8 @@ isert_free_np(struct iscsi_np *np)
 
 	for (;;) {
 		mutex_lock(&isert_np->mutex);
-		isert_conn = isert_np_take_conn(isert_np, &promoted);
+		isert_conn = isert_np_take_conn(isert_np);
+		promoted = isert_conn && isert_conn->promoted;
 		mutex_unlock(&isert_np->mutex);
 
 		if (!isert_conn)
@@ -2554,7 +2619,8 @@ isert_free_np(struct iscsi_np *np)
 	}
 
 	np->np_context = NULL;
-	kfree(isert_np);
+	isert_np->np = NULL;
+	isert_put_np(isert_np);
 }
 
 static void isert_release_work(struct work_struct *work)
@@ -2717,8 +2783,61 @@ 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, *tmp;
+	LIST_HEAD(removal_list);
+
+	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);
+	WRITE_ONCE(found->removing, true);
+	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
+		if (isert_conn->device != found)
+			continue;
+		/* A connection with no reference left is already releasing. */
+		if (!kref_get_unless_zero(&isert_conn->kref))
+			continue;
+		list_add_tail(&isert_conn->removal_node, &removal_list);
+	}
+	mutex_unlock(&isert_conn_mutex);
+
+	list_for_each_entry_safe(isert_conn, tmp, &removal_list, removal_node) {
+		list_del_init(&isert_conn->removal_node);
+		isert_info("removing conn %p state %d\n",
+			   isert_conn, isert_conn->state);
+		isert_teardown_conn(isert_conn);
+		isert_put_conn(isert_conn);
+	}
+
+	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");
@@ -2739,21 +2858,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..fbb58f3865f215 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -185,14 +185,16 @@ struct isert_conn {
 	struct ib_cq		*cq;
 	u32			cq_size;
 	struct isert_device	*device;
+	struct isert_np		*np;
+	bool			promoted;
 	struct mutex		mutex;
 	struct kref		kref;
 	struct work_struct	release_work;
+	struct list_head	removal_node;
 	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 +205,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 {
@@ -212,4 +216,5 @@ struct isert_np {
 	struct mutex		mutex;
 	struct list_head	accepted;
 	struct list_head	pending;
+	struct kref		kref;
 };
-- 
2.43.0

Re: [PATCH v4 3/3] 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: race between isert_teardown_conn() and isert_connect_error()
- [High] IB/isert: QP destroyed after accept in isert_connect_request() on removal
- [High] IB/isert: new isert_device created after isert_remove_one() completes
- [High] IB/isert: race between isert_teardown_conn() and isert_accept_np()
--

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

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

This commit registers an ib_client removal callback to tear down iSER
connections without holding the rdma_cm_id handler_mutex, waiting for the
last isert_device reference to be dropped and reference counting isert_np.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index c6f417bb20690..5486fbdc6aa39 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -456,20 +466,42 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>  		goto out_rsp_dma_map;
>  	}
>  
> +	if (READ_ONCE(device->removing)) {
> +		isert_info("device %p is being removed, rejecting conn %p\n",
> +			   device, isert_conn);
> +		ret = -ENODEV;
> +		goto out_destroy_qp;
> +	}
> +
>  	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;
> -
> +		goto out_drain_qp;
> +	accepted = true;
> +
> +	mutex_lock(&isert_conn_mutex);
> +	if (device->removing) {
> +		mutex_unlock(&isert_conn_mutex);
> +		isert_info("device %p is being removed, dropping conn %p\n",
> +			   device, isert_conn);
> +		ret = -ENODEV;
> +		goto out_drain_qp;
> +	}
> +	isert_conn->np = isert_np;
> +	kref_get(&isert_np->kref);
> +	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;
>  
> +out_drain_qp:
> +	ib_drain_qp(isert_conn->qp);
>  out_destroy_qp:
>  	isert_destroy_qp(isert_conn);
>  out_rsp_dma_map:
> @@ -478,7 +510,9 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>  	isert_device_put(device);
>  out:
>  	kfree(isert_conn);
> -	rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
> +	/* cma destroys an accepted cm id when this handler returns an error. */
> +	if (!accepted)
> +		rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
>  	return ret;
>  }

[Severity: High]
As reported by Sashiko on v3 [1], if device->removing becomes true between
the unlocked READ_ONCE(device->removing) check and mutex_lock(&isert_conn_mutex)
in isert_connect_request(), isert_rdma_accept() has already succeeded, yet
the error path jumps to out_drain_qp -> out_destroy_qp and calls
ib_destroy_qp(isert_conn->qp) before returning -ENODEV to the RDMA CM
CONNECT_REQUEST handler.

On iWARP transports, cma_accept_iw() -> iw_cm_accept() sets
IWCM_F_CONNECT_WAIT, stores cm_id_priv->qp = qp, and takes a provider QP
reference via iw_add_ref(qp) that is not released until iw_destroy_cm_id() or
cm_close_handler() runs on iwcm_wq after the CONNECT_REQUEST callback
returns. Won't calling ib_destroy_qp() here before returning from
isert_connect_request() deadlock in iWARP provider destroy_qp callbacks that
wait for the QP reference count to drop to zero (such as siw_destroy_qp() or
_irdma_destroy_qp()), or deadlock destroy_cm_id() on iwcm_wq waiting for
IWCM_F_CONNECT_WAIT to clear after iw_conn_req_handler() returns an error?

Also on IB/RoCE, doesn't destroying isert_conn->qp directly with
ib_destroy_qp() after cma_accept_ib() has already sent the CM REP leave
cma_id->qp pointing to a freed QP while the CM ID is still being torn down?

[ ... ]

> @@ -628,17 +695,17 @@ 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;
>  	bool terminated = false;
>  
>  	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
> +	case ISER_CONN_DOWN:
>  		break;
> +	case ISER_CONN_INIT:
>  	case ISER_CONN_UP:
>  		terminated = isert_conn_terminate(isert_conn);
>  		break;
> @@ -652,10 +719,20 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
>  	}
>  	mutex_unlock(&isert_conn->mutex);
>  
> -	if (terminated) {
> -		ib_drain_qp(isert_conn->qp);
> -		isert_handle_unbound_conn(isert_conn);
> -	}
> +	if (!terminated)
> +		return;
> +
> +	ib_drain_qp(isert_conn->qp);
> +	isert_handle_unbound_conn(isert_conn);
> +}

[Severity: Critical]
Can isert_teardown_conn() called from isert_remove_one() race with
isert_connect_error()?

Previously, connection teardown and connection error handling both ran under
the cm_id handler_mutex, and ISER_CONN_INIT was not handled in
isert_disconnected_handler(). With this change, isert_remove_one() takes a
reference on isert_conn via kref_get_unless_zero(&isert_conn->kref) and calls
isert_teardown_conn() outside the cm_id handler_mutex, while
isert_connect_error() does not take isert_conn->mutex or update
isert_conn->state:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_error() {
	mutex_lock(&isert_np->mutex);
	owned = !list_empty(&isert_conn->node);
	list_del_init(&isert_conn->node);
	mutex_unlock(&isert_np->mutex);

	if (!owned) {
		isert_info("conn %p is being released already\n", isert_conn);
		return 0;
	}

	ib_drain_qp(isert_conn->qp);
	isert_conn->cm_id = NULL;
	isert_put_conn(isert_conn);

	return -1;
}

If isert_connect_error() runs after isert_remove_one() grabs a kref on a
connection in ISER_CONN_INIT and before isert_remove_one() calls
isert_teardown_conn(), isert_connect_error() sets isert_conn->cm_id = NULL,
drops only the initial kref, and returns -1 (causing RDMA CM to destroy
cma_id), leaving isert_conn->state as ISER_CONN_INIT. When
isert_remove_one() then calls isert_teardown_conn() ->
isert_conn_terminate(), won't rdma_disconnect(isert_conn->cm_id) dereference
a NULL (or freed) cm_id?

Conversely, if isert_teardown_conn() runs isert_conn_terminate() first, it
sets isert_conn->state = ISER_CONN_TERMINATING and drops isert_conn->mutex to
call ib_drain_qp(isert_conn->qp) before isert_handle_unbound_conn() unlinks
isert_conn->node. While ib_drain_qp() is sleeping, can't a concurrent
isert_connect_error() see !list_empty(&isert_conn->node), unlink it, call
ib_drain_qp(isert_conn->qp) concurrently on the same QP, clear
isert_conn->cm_id = NULL, and free cma_id?

Also, if RDMA_CM_EVENT_UNREACHABLE triggers isert_connect_error() after
isert_connected_handler() has promoted the connection to ISER_CONN_UP (with
isert_conn->promoted == true and an extra kref), isert_connect_error()
unlinks isert_conn->node without dropping the promoted reference. Won't that
leak isert_conn and hang wait_for_completion(&removal_comp) in
isert_remove_one()?

[ ... ]

> @@ -2480,6 +2546,7 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
>  	isert_conn = list_first_entry(&isert_np->pending,
>  			struct isert_conn, node);
>  	list_del_init(&isert_conn->node);
> +	isert_conn->promoted = false;
>  	mutex_unlock(&isert_np->mutex);
>  
>  	conn->context = isert_conn;

[Severity: High]
What happens if isert_teardown_conn() races with isert_accept_np() on a
connection in ISER_CONN_UP?

In isert_teardown_conn(), when isert_conn->state is ISER_CONN_UP, it sets
isert_conn->state = ISER_CONN_TERMINATING under isert_conn->mutex, unlocks
isert_conn->mutex, and calls ib_drain_qp(isert_conn->qp) before calling
isert_handle_unbound_conn() to unlink isert_conn->node from
isert_np->pending.

During that ib_drain_qp() window, isert_accept_np() only acquires
isert_np->mutex (without acquiring isert_conn->mutex or checking
isert_conn->state):

	mutex_lock(&isert_np->mutex);
	if (list_empty(&isert_np->pending)) {
		mutex_unlock(&isert_np->mutex);
		goto accept_wait;
	}
	isert_conn = list_first_entry(&isert_np->pending,
			struct isert_conn, node);
	list_del_init(&isert_conn->node);
	isert_conn->promoted = false;
	mutex_unlock(&isert_np->mutex);

	conn->context = isert_conn;
	isert_conn->conn = conn;
	isert_conn->state = ISER_CONN_BOUND;

If isert_accept_np() pops the connection from isert_np->pending while
ib_drain_qp() is in progress, it overwrites isert_conn->state from
ISER_CONN_TERMINATING to ISER_CONN_BOUND. Then isert_handle_unbound_conn()
sees list_empty(&isert_conn->node) and does nothing, while
isert_teardown_conn() never called iscsit_cause_connection_reinstatement()
because the state was ISER_CONN_UP when it checked. If the login request PDU
already arrived before the QP drained and finishes a single-stage login, the
login timer is stopped and iscsi_target_rx_thread() blocks in
isert_get_rx_pdu() -> wait_for_completion_interruptible(&comp) on the
already-drained QP. Doesn't this leave the connection stuck and hang
isert_remove_one() at wait_for_completion(&removal_comp)?

[ ... ]

> @@ -2717,8 +2783,61 @@ 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, *tmp;
> +	LIST_HEAD(removal_list);
> +
> +	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);
> +	WRITE_ONCE(found->removing, true);
> +	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> +		if (isert_conn->device != found)
> +			continue;
> +		/* A connection with no reference left is already releasing. */
> +		if (!kref_get_unless_zero(&isert_conn->kref))
> +			continue;
> +		list_add_tail(&isert_conn->removal_node, &removal_list);
> +	}
> +	mutex_unlock(&isert_conn_mutex);
> +
> +	list_for_each_entry_safe(isert_conn, tmp, &removal_list, removal_node) {
> +		list_del_init(&isert_conn->removal_node);
> +		isert_info("removing conn %p state %d\n",
> +			   isert_conn, isert_conn->state);
> +		isert_teardown_conn(isert_conn);
> +		isert_put_conn(isert_conn);
> +	}
> +
> +	isert_device_put(found);
> +	wait_for_completion(&removal_comp);
> +}

[Severity: High]
As reported by Sashiko on v2 [2], because isert_ib_client is registered after
cma_client, disable_device() invokes isert_remove_one() before
cma_remove_one() destroys listening RDMA CM IDs.

In ib_isert.c, struct isert_device is only present on device_list while its
refcount is non-zero: if there are no active connections when
isert_remove_one() runs (or once existing connections drop the refcount to 0
in isert_device_put() and complete removal_comp), the isert_device is removed
from device_list and freed, and isert_remove_one() returns.

If an incoming RDMA_CM_EVENT_CONNECT_REQUEST arrives on a listener before
cma_remove_one() runs, won't isert_device_get() find no matching entry in
device_list, allocate a fresh struct isert_device with removing == false, and
accept the new connection after isert_remove_one() has already completed?
When cma_remove_one() then delivers RDMA_CM_EVENT_DEVICE_REMOVAL to that
connection, isert_disconnected_handler() queues release_work asynchronously,
and isert_connect_release() calls rdma_destroy_id() (which unblocks
cma_remove_one()) before destroying the QP, returning the shared CQ to the
pool, unmapping DMA buffers, and deallocating the PD.

[1] https://lore.kernel.org/r/20261102061608.617186-3-yhlee@isslab.korea.ac.kr
[2] https://lore.kernel.org/r/sashiko-outbox-184744@kernel.org

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

Re: [PATCH v4 0/3] IB/isert: fix the device removal use-after-free

[Yehyeong Lee]

This series is superseded by v5:

  https://lore.kernel.org/r/20261103125955.1530355-1-yhlee@isslab.korea.ac.kr