* [PATCH v5 1/5] IB/isert: do not publish connections being released on a second list
2026-11-03 12:59 [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
@ 2026-11-03 12:59 ` Yehyeong Lee
2026-11-03 13:26 ` sashiko-bot
2026-11-03 12:59 ` [PATCH v5 2/5] IB/isert: drain the QP without holding isert_conn->mutex Yehyeong Lee
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Yehyeong Lee @ 2026-11-03 12:59 UTC (permalink / raw)
To: Sagi Grimberg, Max Gurtovoy, Jason Gunthorpe, Leon Romanovsky
Cc: Martin K . Petersen, Mike Christie, linux-rdma, target-devel,
linux-kernel, Yehyeong Lee, stable
isert_connect_error() and isert_disconnected_handler() take a connection
off isert_np->accepted or isert_np->pending and release it, but they do not
check whether the other one got there first, and the connection also stays
reachable from the list that isert_free_np() walks. Two callers can
therefore release the same connection, and isert_free_np() can release one
a CM handler has already freed.
Make the list the single owner token. A caller that empties
isert_conn->node under isert_np->mutex owns the drain and the release; one
that finds it already empty stands back and returns zero, so cma keeps the
id and the owner destroys it. isert_connect_error() drains before it takes
the connection, which is the order mainline already uses, so no drain can
run on a QP whose id another handler is tearing down.
A test-only delay in isert_free_np(), right after it has taken a
connection, opens the window; it does not open on its own in this setup.
With it, two runs of 7.4-rc1 reported 22 use-after-frees each and then an
oops at a NULL instruction pointer in the rmdir task, which was killed.
Two runs with this patch reported neither, and the rmdir returned in seven
seconds.
BUG: KASAN: slab-use-after-free in isert_connect_release+0x5c1/0x620
Read of size 8 at addr ffff888109c5d160 by task rmdir/191
CPU: 1 UID: 0 PID: 191 Comm: rmdir Not tainted 7.4.0-rc1-NSR-00001-gffec6cbc0e7e #129 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_connect_release
? isert_connect_release+0x5c1/0x620
kasan_report+0xce/0x100
? isert_connect_release+0x5c1/0x620
isert_connect_release+0x5c1/0x620
isert_free_np+0x4cf/0xb20
? __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/0x7b0
? lookup_one_qstr_excl+0x1a/0x1d0
filename_rmdir+0x390/0x560
? __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
RIP: 0033:0x463b6b
Code: 77 05 c3 0f 1f 40 00 48 c7 c2 e0 ff ff ff f7 d8 64 89 02 48 c7 c0 ff ff ff ff c3 0f 1f 40 00 f3 0f 1e fa b8 54 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 c7 c2 e0 ff ff ff f7 d8
RSP: 002b:00007ffcc34f26c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000054
RAX: ffffffffffffffda RBX: 000000003b607218 RCX: 0000000000463b6b
RDX: 0000000000000000 RSI: 000000003b5ea9e0 RDI: 000000003b607218
RBP: 00007ffcc34f2700 R08: 0000000000000000 R09: 0000000000000000
R10: 00007ffcc34f2040 R11: 0000000000000246 R12: 000000003b607288
R13: 0000000000000000 R14: 0000000000000000 R15: 00000000005e6245
</TASK>
Allocated by task 72:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
__kasan_kmalloc+0x8f/0xa0
__kmalloc_cache_noprof+0x148/0x360
isert_cma_handler+0x1e3/0x28a0
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 10:
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+0x1148/0x28a0
cma_cm_event_handler+0x3e/0x240
cma_ib_handler+0x190/0x750
cm_process_work+0x41/0x330
cm_work_handler+0x1de2/0xc160
process_one_work+0x697/0x1060
worker_thread+0x45b/0xd10
kthread+0x2c8/0x3b0
ret_from_fork+0x36e/0x5a0
ret_from_fork_asm+0x1a/0x30
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 | 81 ++++++++++++++++---------
1 file changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index efa8d7b7844c97..ef2edaa3218fee 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,20 @@ 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;
+ bool owned;
ib_drain_qp(isert_conn->qp);
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;
+ }
+
isert_conn->cm_id = NULL;
isert_put_conn(isert_conn);
@@ -2479,12 +2492,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 +2531,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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 1/5] IB/isert: do not publish connections being released on a second list
2026-11-03 12:59 ` [PATCH v5 1/5] IB/isert: do not publish connections being released on a second list Yehyeong Lee
@ 2026-11-03 13:26 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-03 13:26 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma
> isert_connect_error() and isert_disconnected_handler() take a connection
> off isert_np->accepted or isert_np->pending and release it, but they do not
> check whether the other one got there first, and the connection also stays
> reachable from the list that isert_free_np() walks. Two callers can
> therefore release the same connection, and isert_free_np() can release one
> a CM handler has already freed.
>
> Make the list the single owner token. A caller that empties
> isert_conn->node under isert_np->mutex owns the drain and the release; one
> that finds it already empty stands back and returns zero, so cma keeps the
> id and the owner destroys it. isert_connect_error() drains before it takes
> the connection, which is the order mainline already uses, so no drain can
> run on a QP whose id another handler is tearing down.
>
> A test-only delay in isert_free_np(), right after it has taken a
> [ ... ]
> ret_from_fork_asm+0x1a/0x30
>
> 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>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103125955.1530355-1-yhlee@isslab.korea.ac.kr?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 2/5] IB/isert: drain the QP without holding isert_conn->mutex
2026-11-03 12:59 [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
2026-11-03 12:59 ` [PATCH v5 1/5] IB/isert: do not publish connections being released on a second list Yehyeong Lee
@ 2026-11-03 12:59 ` Yehyeong Lee
2026-11-03 13:21 ` sashiko-bot
2026-11-03 12:59 ` [PATCH v5 3/5] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Yehyeong Lee @ 2026-11-03 12:59 UTC (permalink / raw)
To: Sagi Grimberg, Max Gurtovoy, Jason Gunthorpe, Leon Romanovsky
Cc: Martin K . Petersen, Mike Christie, linux-rdma, target-devel,
linux-kernel, Yehyeong Lee, stable
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 drain after dropping
isert_conn->mutex. Only the owner of isert_conn->node drains, so two
callers cannot drain the same QP; the send and receive queues reserve room
for one drain each, and a second concurrent drain hits the WARN_ONCE in
__ib_drain_sq() and returns without draining.
Two test-only delays open the window, one in isert_login_recv_done() and
one before the drain. Three runs of 7.4-rc1 plus patch 1 of this series
printed ten hung task warnings each, eight, five and five of them waiting
in ib_drain_qp(). Three runs with this patch printed none, and
PROVE_LOCKING reported no splat in two runs a side.
INFO: task kworker/0:1:10 blocked for more than 10 seconds.
Not tainted 7.4.0-rc1-NS1-00002-g7b7ecb49489e #121
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:kworker/0:1 state:D stack:26248 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
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/0x27d0
? kasan_save_stack+0x42/0x60
? kasan_save_stack+0x33/0x60
? kasan_save_track+0x14/0x30
? kasan_save_free_info+0x3b/0x60
? __kasan_slab_free+0x43/0x70
? kfree+0x121/0x390
? cm_process_work+0x8b/0x330
? __pfx_isert_cma_handler+0x10/0x10
? kthread+0x2c8/0x3b0
? ret_from_fork+0x36e/0x5a0
? ret_from_fork_asm+0x1a/0x30
? _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>
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 | 75 ++++++++++++-------------
1 file changed, 36 insertions(+), 39 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index ef2edaa3218fee..61b214012a70b0 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -556,43 +556,21 @@ isert_put_conn(struct isert_conn *isert_conn)
kref_put(&isert_conn->kref, isert_release_kref);
}
-static void
-isert_handle_unbound_conn(struct isert_conn *isert_conn)
-{
- struct isert_np *isert_np = isert_conn->cm_id->context;
-
- mutex_lock(&isert_np->mutex);
- if (!list_empty(&isert_conn->node)) {
- /*
- * This means iscsi doesn't know this connection
- * so schedule a cleanup ourselves
- */
- list_del_init(&isert_conn->node);
- isert_put_conn(isert_conn);
- queue_work(isert_release_wq, &isert_conn->release_work);
- }
- mutex_unlock(&isert_np->mutex);
-}
-
/**
* 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.
+ * Return: 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 +579,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,26 +615,44 @@ 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;
+ struct isert_np *isert_np = cma_id->context;
+ bool owned = false, bound = false, 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);
+ mutex_lock(&isert_np->mutex);
+ if (list_empty(&isert_conn->node)) {
+ bound = isert_conn->state == ISER_CONN_BOUND ||
+ isert_conn->state == ISER_CONN_FULL_FEATURE;
+ } else {
+ owned = true;
+ list_del_init(&isert_conn->node);
+ }
+ mutex_unlock(&isert_np->mutex);
break;
case ISER_CONN_BOUND:
case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
- iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
+ bound = true;
break;
default:
- isert_warn("conn %p terminating in state %d\n",
- isert_conn, isert_conn->state);
+ break;
}
+
+ if (owned)
+ terminated = isert_conn_terminate(isert_conn);
+ else if (bound)
+ iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
mutex_unlock(&isert_conn->mutex);
+ if (!owned)
+ return 0;
+
+ if (terminated)
+ ib_drain_qp(isert_conn->qp);
+ isert_put_conn(isert_conn);
+ queue_work(isert_release_wq, &isert_conn->release_work);
+
return 0;
}
@@ -665,8 +663,6 @@ isert_connect_error(struct rdma_cm_id *cma_id)
struct isert_np *isert_np = cma_id->context;
bool owned;
- ib_drain_qp(isert_conn->qp);
-
mutex_lock(&isert_np->mutex);
owned = !list_empty(&isert_conn->node);
list_del_init(&isert_conn->node);
@@ -677,6 +673,7 @@ isert_connect_error(struct rdma_cm_id *cma_id)
return 0;
}
+ ib_drain_qp(isert_conn->qp);
isert_conn->cm_id = NULL;
isert_put_conn(isert_conn);
@@ -2479,11 +2476,10 @@ 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);
- mutex_unlock(&isert_np->mutex);
-
conn->context = isert_conn;
isert_conn->conn = conn;
isert_conn->state = ISER_CONN_BOUND;
+ mutex_unlock(&isert_np->mutex);
isert_set_conn_info(np, conn, isert_conn);
@@ -2547,6 +2543,7 @@ isert_free_np(struct iscsi_np *np)
isert_info("cleaning isert_conn %p state (%d)\n",
isert_conn, isert_conn->state);
+ ib_drain_qp(isert_conn->qp);
if (promoted)
isert_put_conn(isert_conn);
isert_put_conn(isert_conn);
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 2/5] IB/isert: drain the QP without holding isert_conn->mutex
2026-11-03 12:59 ` [PATCH v5 2/5] IB/isert: drain the QP without holding isert_conn->mutex Yehyeong Lee
@ 2026-11-03 13:21 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-03 13:21 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma
> 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 drain after dropping
> isert_conn->mutex. Only the owner of isert_conn->node drains, so two
> callers cannot drain the same QP; the send and receive queues reserve room
> for one drain each, and a second concurrent drain hits the WARN_ONCE in
> __ib_drain_sq() and returns without draining.
>
> [ ... ]
> </TASK>
>
> Fixes: 6d1fba0c2cc7 ("iser-target: Rework connection termination")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103125955.1530355-1-yhlee@isslab.korea.ac.kr?part=2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 3/5] IB/isert: tear connections down from an ib_client on device removal
2026-11-03 12:59 [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
2026-11-03 12:59 ` [PATCH v5 1/5] IB/isert: do not publish connections being released on a second list Yehyeong Lee
2026-11-03 12:59 ` [PATCH v5 2/5] IB/isert: drain the QP without holding isert_conn->mutex Yehyeong Lee
@ 2026-11-03 12:59 ` Yehyeong Lee
2026-11-03 13:32 ` sashiko-bot
2026-11-03 12:59 ` [PATCH v5 4/5] IB/isert: drop the promotion reference when a connection errors out Yehyeong Lee
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Yehyeong Lee @ 2026-11-03 12:59 UTC (permalink / raw)
To: Sagi Grimberg, Max Gurtovoy, Jason Gunthorpe, Leon Romanovsky
Cc: Martin K . Petersen, Mike Christie, linux-rdma, target-devel,
linux-kernel, Yehyeong Lee, stable
The RDMA_CM_EVENT_DEVICE_REMOVAL callback waits for the connection it was
handed to reach ISER_CONN_DOWN and then frees it. isert_release_work()
publishes that state before it drops its reference, so the handler can free
a connection that isert_connect_release() is still running on. The wait
itself deadlocks against the handler_mutex rdma_destroy_id() needs.
Register an ib_client instead, as nvmet-rdma did in commit f1d4ef7d8883
("nvmet-rdma: register ib_client to not deadlock in device removal"), and
tear the device's connections down from its remove callback. It walks
isert_conn_list under isert_conn_mutex, takes a reference with
kref_get_unless_zero(), tears each one down after dropping the lock, and
asks iscsi to reinstate the session. isert_device_put() completes
removal_comp once the CQs and the PD are gone. Publishing now happens in
the same isert_conn_mutex section that tests isert_device->removing, and
isert_np is reference counted because a connection can outlive its portal.
This cell drives 32 logins and then deletes the device, with no delay knob.
One of twelve runs of 7.4-rc1 finished the removal, and two reported the
use-after-free below, 17 and 11 times; all twelve runs of this series
finished and none reported it. The sides ran in separate rounds.
BUG: KASAN: slab-use-after-free in isert_connect_release+0x5cb/0x620
Read of size 4 at addr ffff88810af75158 by task kworker/u16:6/76
CPU: 1 UID: 0 PID: 76 Comm: kworker/u16:6 Not tainted 7.4.0-rc1-CTL #19 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
? kfree+0x121/0x390
? isert_connect_release+0x5cb/0x620
kasan_report+0xce/0x100
? isert_connect_release+0x5cb/0x620
isert_connect_release+0x5cb/0x620
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
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 51:
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 217:
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
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 | 274 +++++++++++++++++-------
drivers/infiniband/ulp/isert/ib_isert.h | 9 +-
2 files changed, 208 insertions(+), 75 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 61b214012a70b0..4812dabd939453 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);
@@ -456,17 +465,34 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
goto out_rsp_dma_map;
}
+ 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;
+ }
+
ret = isert_login_post_recv(isert_conn);
- if (ret)
+ if (ret) {
+ mutex_unlock(&isert_conn_mutex);
goto out_destroy_qp;
+ }
ret = isert_rdma_accept(isert_conn);
- if (ret)
+ if (ret) {
+ mutex_unlock(&isert_conn_mutex);
goto out_destroy_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;
@@ -486,14 +512,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 +534,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 +546,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,6 +600,20 @@ 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);
+}
+
/**
* isert_conn_terminate() - Initiate connection termination
* @isert_conn: isert connection struct
@@ -610,48 +668,50 @@ 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;
- struct isert_np *isert_np = cma_id->context;
- bool owned = false, bound = false, terminated = false;
+ struct isert_np *isert_np = isert_conn->np;
+ bool owned = false, bound = false;
+ bool promoted = false, terminated = false;
mutex_lock(&isert_conn->mutex);
- switch (isert_conn->state) {
- case ISER_CONN_UP:
- mutex_lock(&isert_np->mutex);
- if (list_empty(&isert_conn->node)) {
- bound = isert_conn->state == ISER_CONN_BOUND ||
- isert_conn->state == ISER_CONN_FULL_FEATURE;
- } else {
- owned = true;
- list_del_init(&isert_conn->node);
- }
- mutex_unlock(&isert_np->mutex);
- break;
- case ISER_CONN_BOUND:
- case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
- bound = true;
- break;
- default:
- break;
+ mutex_lock(&isert_np->mutex);
+ if (list_empty(&isert_conn->node)) {
+ bound = isert_conn->state == ISER_CONN_BOUND ||
+ isert_conn->state == ISER_CONN_FULL_FEATURE;
+ } else {
+ owned = true;
+ promoted = isert_conn->promoted;
+ list_del_init(&isert_conn->node);
}
+ mutex_unlock(&isert_np->mutex);
- if (owned)
+ if (owned) {
terminated = isert_conn_terminate(isert_conn);
- else if (bound)
+ } else if (bound) {
+ isert_conn_terminate(isert_conn);
iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
+ }
mutex_unlock(&isert_conn->mutex);
if (!owned)
- return 0;
+ return;
if (terminated)
ib_drain_qp(isert_conn->qp);
- isert_put_conn(isert_conn);
+ if (promoted)
+ isert_put_conn(isert_conn);
queue_work(isert_release_wq, &isert_conn->release_work);
+}
+
+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;
}
@@ -661,20 +721,33 @@ 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;
- bool owned;
+ bool owned = false, bound = false;
+ mutex_lock(&isert_conn->mutex);
mutex_lock(&isert_np->mutex);
- owned = !list_empty(&isert_conn->node);
- list_del_init(&isert_conn->node);
+ if (list_empty(&isert_conn->node)) {
+ bound = isert_conn->state == ISER_CONN_BOUND ||
+ isert_conn->state == ISER_CONN_FULL_FEATURE;
+ } else {
+ owned = true;
+ list_del_init(&isert_conn->node);
+ }
mutex_unlock(&isert_np->mutex);
+ if (owned) {
+ isert_conn->cm_id = NULL;
+ } else if (bound) {
+ isert_conn_terminate(isert_conn);
+ iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
+ }
+ mutex_unlock(&isert_conn->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;
@@ -684,7 +757,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",
@@ -705,21 +777,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));
@@ -2331,6 +2392,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);
@@ -2356,7 +2418,7 @@ isert_setup_np(struct iscsi_np *np,
return 0;
out:
- kfree(isert_np);
+ isert_put_np(isert_np);
return ret;
}
@@ -2476,6 +2538,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;
conn->context = isert_conn;
isert_conn->conn = conn;
isert_conn->state = ISER_CONN_BOUND;
@@ -2489,20 +2552,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);
@@ -2535,7 +2595,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)
@@ -2550,7 +2611,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)
@@ -2679,6 +2741,10 @@ static void isert_get_rx_pdu(struct iscsit_conn *conn)
/* Now we are in FULL_FEATURE phase */
mutex_lock(&isert_conn->mutex);
+ if (isert_conn->state >= ISER_CONN_TERMINATING) {
+ mutex_unlock(&isert_conn->mutex);
+ return;
+ }
isert_conn->state = ISER_CONN_FULL_FEATURE;
mutex_unlock(&isert_conn->mutex);
@@ -2713,8 +2779,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);
+ 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");
@@ -2735,21 +2854,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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 3/5] IB/isert: tear connections down from an ib_client on device removal
2026-11-03 12:59 ` [PATCH v5 3/5] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
@ 2026-11-03 13:32 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-03 13:32 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma, target-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] IB/isert: premature reinstatement in isert_teardown_conn() hangs login
--
commit fa784d352b38db9ec9f326e2a339d1ae4eb8f372
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 to tear down iSER target connections
during RDMA device removal instead of waiting for connection release inside
the RDMA_CM_EVENT_DEVICE_REMOVAL callback. It also adds reference counting
for isert_np and tracks active connections on isert_conn_list.
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 61b214012a70b..4812dabd93945 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
[ ... ]
> @@ -610,48 +668,50 @@ 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;
> - struct isert_np *isert_np = cma_id->context;
> - bool owned = false, bound = false, terminated = false;
> + struct isert_np *isert_np = isert_conn->np;
> + bool owned = false, bound = false;
> + bool promoted = false, terminated = false;
>
> mutex_lock(&isert_conn->mutex);
> - switch (isert_conn->state) {
> - case ISER_CONN_UP:
> - mutex_lock(&isert_np->mutex);
> - if (list_empty(&isert_conn->node)) {
> - bound = isert_conn->state == ISER_CONN_BOUND ||
> - isert_conn->state == ISER_CONN_FULL_FEATURE;
> - } else {
> - owned = true;
> - list_del_init(&isert_conn->node);
> - }
> - mutex_unlock(&isert_np->mutex);
> - break;
> - case ISER_CONN_BOUND:
> - case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
> - bound = true;
> - break;
> - default:
> - break;
> + mutex_lock(&isert_np->mutex);
> + if (list_empty(&isert_conn->node)) {
> + bound = isert_conn->state == ISER_CONN_BOUND ||
> + isert_conn->state == ISER_CONN_FULL_FEATURE;
> + } else {
> + owned = true;
> + promoted = isert_conn->promoted;
> + list_del_init(&isert_conn->node);
> }
> + mutex_unlock(&isert_np->mutex);
>
> - if (owned)
> + if (owned) {
> terminated = isert_conn_terminate(isert_conn);
> - else if (bound)
> + } else if (bound) {
> + isert_conn_terminate(isert_conn);
> iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
> + }
> mutex_unlock(&isert_conn->mutex);
[ ... ]
> @@ -661,20 +721,33 @@ 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;
> - bool owned;
> + bool owned = false, bound = false;
>
> + mutex_lock(&isert_conn->mutex);
> mutex_lock(&isert_np->mutex);
> - owned = !list_empty(&isert_conn->node);
> - list_del_init(&isert_conn->node);
> + if (list_empty(&isert_conn->node)) {
> + bound = isert_conn->state == ISER_CONN_BOUND ||
> + isert_conn->state == ISER_CONN_FULL_FEATURE;
> + } else {
> + owned = true;
> + list_del_init(&isert_conn->node);
> + }
> mutex_unlock(&isert_np->mutex);
>
> + if (owned) {
> + isert_conn->cm_id = NULL;
> + } else if (bound) {
> + isert_conn_terminate(isert_conn);
> + iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
> + }
> + mutex_unlock(&isert_conn->mutex);
[ ... ]
> @@ -2679,6 +2741,10 @@ static void isert_get_rx_pdu(struct iscsit_conn *conn)
>
> /* Now we are in FULL_FEATURE phase */
> mutex_lock(&isert_conn->mutex);
> + if (isert_conn->state >= ISER_CONN_TERMINATING) {
> + mutex_unlock(&isert_conn->mutex);
> + return;
> + }
> isert_conn->state = ISER_CONN_FULL_FEATURE;
> mutex_unlock(&isert_conn->mutex);
[Severity: High]
Can calling iscsit_cause_connection_reinstatement() in isert_teardown_conn()
and isert_connect_error() while isert_conn->state is ISER_CONN_BOUND leak
isert_conn and hang device removal in isert_remove_one()?
In isert_accept_np(), isert_conn->state is set to ISER_CONN_BOUND and stays
there throughout login negotiation until isert_get_rx_pdu() sets
ISER_CONN_FULL_FEATURE. At the end of login negotiation,
iscsi_target_do_tx_login_io() starts the RX and TX kthreads while
conn->conn_state is still TARG_CONN_STATE_IN_LOGIN and before
iscsi_post_login_handler() completes conn->rx_login_comp:
iscsi_target_do_login_io()
iscsi_target_do_tx_login_io()
iscsit_start_kthreads(conn)
conn->tx_thread_active = true;
conn->rx_thread_active = true;
iscsit_put_login_tx() -> isert_put_login_tx()
isert_conn->login_rsp_pending = true;
If isert_teardown_conn() or isert_connect_error() runs in this window,
bound is true because isert_conn->state == ISER_CONN_BOUND, so it calls
iscsit_cause_connection_reinstatement(isert_conn->conn, 0), sending SIGINT
to both conn->rx_thread and conn->tx_thread.
In drivers/target/iscsi/iscsi_target.c:iscsi_target_rx_thread(), SIGINT
interrupts wait_for_completion_interruptible(&conn->rx_login_comp) before
iscsit_get_rx_pdu() -> isert_get_rx_pdu() is reached:
rc = wait_for_completion_interruptible(&conn->rx_login_comp);
if (rc < 0 || iscsi_target_check_conn_state(conn))
goto out;
And in drivers/target/iscsi/iscsi_target.c:iscsi_target_tx_thread(), waking
on SIGINT jumps to transport_err, where conn->conn_state is still
TARG_CONN_STATE_IN_LOGIN:
transport_err:
if (conn->conn_state != TARG_CONN_STATE_IN_LOGIN)
iscsit_take_action_for_connection_exit(conn, &conn_freed);
Neither thread calls iscsit_take_action_for_connection_exit() to close the
connection and invoke isert_free_conn(), leaving both threads sleeping in
while (!kthread_should_stop()) and causing isert_remove_one() to wait
indefinitely on removal_comp.
Furthermore, if iscsi_target_tx_thread() reaches transport_err just after
iscsi_post_login_handler() sets conn->conn_state = TARG_CONN_STATE_LOGGED_IN
but before it adds conn->conn_list to sess->sess_conn_list,
iscsit_close_connection() will run list_del(&conn->conn_list) on an
uninitialized list head while iscsi_post_login_handler() is still running.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103125955.1530355-1-yhlee@isslab.korea.ac.kr?part=3
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 4/5] IB/isert: drop the promotion reference when a connection errors out
2026-11-03 12:59 [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
` (2 preceding siblings ...)
2026-11-03 12:59 ` [PATCH v5 3/5] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
@ 2026-11-03 12:59 ` Yehyeong Lee
2026-11-03 13:16 ` sashiko-bot
2026-11-03 12:59 ` [PATCH v5 5/5] IB/isert: do not build a new isert_device for a device being removed Yehyeong Lee
2026-11-03 22:39 ` [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
5 siblings, 1 reply; 12+ messages in thread
From: Yehyeong Lee @ 2026-11-03 12:59 UTC (permalink / raw)
To: Sagi Grimberg, Max Gurtovoy, Jason Gunthorpe, Leon Romanovsky
Cc: Martin K . Petersen, Mike Christie, linux-rdma, target-devel,
linux-kernel, Yehyeong Lee, stable
isert_connected_handler() takes a reference when it moves a connection to
ISER_CONN_UP, and the paths that give a promoted connection up drop it
again. isert_connect_error() does not: it drops one reference for any
state. A promoted connection that then gets an error event keeps one
reference for ever, so isert_connect_release() never runs and the
isert_device reference it holds is never returned. isert_remove_one()
waits for that in wait_for_completion(&removal_comp).
A REJ after the RTU reaches this path. cm_rej_handler() accepts
IB_CM_ESTABLISHED while lap_state is IB_CM_LAP_UNINIT, and a passive id
stays in RDMA_CM_CONNECT after ESTABLISHED, so cma_ib_handler() passes the
event on as RDMA_CM_EVENT_REJECTED.
Drop the promotion reference too when the connection being given up was
promoted. A test-only knob delivered that event to eight promoted
connections. On patches 1 to 3 of this series the device delete did not
return within 240 seconds in any of three runs, each printed ten hung task
warnings, and isert_device_put() ran once, twice and twice, the first of
those being the one isert_remove_one() does itself. Three runs with this
patch ran it nine times and the delete returned in 15 seconds.
INFO: task rdma:206 blocked for more than 10 seconds.
Not tainted 7.4.0-rc1-NR3-00004-g11e5479fd20e #123
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:rdma state:D stack:26592 pid:206 tgid:206 ppid:1 task_flags:0x400100 flags:0x00080800
Call Trace:
<TASK>
__schedule+0x180f/0x4bb0
? filter_irq_stacks+0x71/0x90
? __pfx___schedule+0x10/0x10
? kasan_save_track+0x14/0x30
schedule+0x70/0x1f0
schedule_timeout+0x1cd/0x200
? __pfx_schedule_timeout+0x10/0x10
? vprintk_emit+0x200/0x2f0
? __pfx__raw_spin_lock_irq+0x10/0x10
__wait_for_common+0x29e/0x420
? __pfx_schedule_timeout+0x10/0x10
? __pfx___wait_for_common+0x10/0x10
? mutex_lock+0x79/0xd0
? mutex_unlock+0x7b/0xd0
? __pfx_mutex_unlock+0x10/0x10
isert_remove_one+0x529/0x710
? xas_load+0x1a/0x250
? __pfx_isert_remove_one+0x10/0x10
? xa_load+0xbf/0x120
? __pfx_xa_load+0x10/0x10
? __pfx_up_read+0x10/0x10
remove_client_context+0xa4/0xf0
disable_device+0x128/0x240
? __pfx_disable_device+0x10/0x10
? __pfx_mutex_lock+0x10/0x10
__ib_unregister_device+0x226/0x3f0
? __pfx_mutex_unlock+0x10/0x10
ib_unregister_device_and_put+0x50/0x70
nldev_dellink+0x28e/0x3d0
? kernel_text_address+0xa7/0x130
? __pfx_nldev_dellink+0x10/0x10
? __pfx_down_read+0x10/0x10
rdma_nl_rcv_msg+0x29c/0x4e0
? __pfx_rdma_nl_rcv_msg+0x10/0x10
rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
? __pfx_rdma_nl_rcv_skb.constprop.0.isra.0+0x10/0x10
netlink_unicast+0x68f/0x940
? __pfx_selinux_socket_getpeersec_dgram+0x10/0x10
? __pfx_netlink_unicast+0x10/0x10
netlink_sendmsg+0x810/0xf20
? __pfx_netlink_sendmsg+0x10/0x10
? __pfx__raw_spin_lock_irqsave+0x10/0x10
? selinux_socket_sendmsg+0x8c/0x270
? __sys_sendto+0x337/0x390
__sys_sendto+0x337/0x390
? __pfx___sys_sendto+0x10/0x10
? fdget+0x53/0x3b0
__x64_sys_sendto+0xdb/0x1b0
? fpregs_assert_state_consistent+0x56/0xe0
do_syscall_64+0xdd/0x4a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fe0ecec20d7
RSP: 002b:00007fffe9d34978 EFLAGS: 00000202 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 00005621db1ab390 RCX: 00007fe0ecec20d7
RDX: 0000000000000018 RSI: 00005621db1aa2a0 RDI: 0000000000000004
RBP: 00007fffe9d349b0 R08: 00007fe0ecfb9200 R09: 000000000000000c
R10: 0000000000000000 R11: 0000000000000202 R12: 00007fffe9d34ba0
R13: 00007fffe9d34984 R14: 00007fffe9d34bc0 R15: 00007fffe9d34ba0
</TASK>
Fixes: 954f23722b57 ("iscsi,iser-target: Initiate termination only once")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/infiniband/ulp/isert/ib_isert.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 4812dabd939453..712d6165150e3c 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -721,7 +721,7 @@ 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;
- bool owned = false, bound = false;
+ bool owned = false, bound = false, promoted = false;
mutex_lock(&isert_conn->mutex);
mutex_lock(&isert_np->mutex);
@@ -730,6 +730,7 @@ isert_connect_error(struct rdma_cm_id *cma_id)
isert_conn->state == ISER_CONN_FULL_FEATURE;
} else {
owned = true;
+ promoted = isert_conn->promoted;
list_del_init(&isert_conn->node);
}
mutex_unlock(&isert_np->mutex);
@@ -748,6 +749,8 @@ isert_connect_error(struct rdma_cm_id *cma_id)
}
ib_drain_qp(isert_conn->qp);
+ if (promoted)
+ isert_put_conn(isert_conn);
isert_put_conn(isert_conn);
return -1;
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 4/5] IB/isert: drop the promotion reference when a connection errors out
2026-11-03 12:59 ` [PATCH v5 4/5] IB/isert: drop the promotion reference when a connection errors out Yehyeong Lee
@ 2026-11-03 13:16 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-03 13:16 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma
> isert_connected_handler() takes a reference when it moves a connection to
> ISER_CONN_UP, and the paths that give a promoted connection up drop it
> again. isert_connect_error() does not: it drops one reference for any
> state. A promoted connection that then gets an error event keeps one
> reference for ever, so isert_connect_release() never runs and the
> isert_device reference it holds is never returned. isert_remove_one()
> waits for that in wait_for_completion(&removal_comp).
>
> A REJ after the RTU reaches this path. cm_rej_handler() accepts
> IB_CM_ESTABLISHED while lap_state is IB_CM_LAP_UNINIT, and a passive id
> stays in RDMA_CM_CONNECT after ESTABLISHED, so cma_ib_handler() passes the
> event on as RDMA_CM_EVENT_REJECTED.
>
> Drop the promotion reference too when the connection being given up was
> promoted. A test-only knob delivered that event to eight promoted
> [ ... ]
> </TASK>
>
> Fixes: 954f23722b57 ("iscsi,iser-target: Initiate termination only once")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103125955.1530355-1-yhlee@isslab.korea.ac.kr?part=4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 5/5] IB/isert: do not build a new isert_device for a device being removed
2026-11-03 12:59 [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
` (3 preceding siblings ...)
2026-11-03 12:59 ` [PATCH v5 4/5] IB/isert: drop the promotion reference when a connection errors out Yehyeong Lee
@ 2026-11-03 12:59 ` Yehyeong Lee
2026-11-03 13:39 ` sashiko-bot
2026-11-03 22:39 ` [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
5 siblings, 1 reply; 12+ messages in thread
From: Yehyeong Lee @ 2026-11-03 12:59 UTC (permalink / raw)
To: Sagi Grimberg, Max Gurtovoy, Jason Gunthorpe, Leon Romanovsky
Cc: Martin K . Petersen, Mike Christie, linux-rdma, target-devel,
linux-kernel, Yehyeong Lee, stable
isert_device_get() looks the device up by node_guid and has no reason to
refuse one. A CONNECT_REQUEST arriving while isert_remove_one() runs
therefore builds a new isert_device on an ib_device the core is about to
free, and nothing waits for its QP, CQ or PD. cma_process_remove() waits
only for id destruction, and the core takes no device reference for them,
so ib_dealloc_device() can run while they are still being released.
Keep the struct as a marker when the last reference of a device being
removed goes away, refuse isert_device_get() for an ib_device that has one,
and clear it from an ib_client add callback. The lookup runs in two
passes, so a marker cannot hide a live device whose ib_device pointer was
reused. The marker uses __GFP_NOFAIL because isert_remove_one() cannot
fail, and preallocating in the add callback does not work:
add_client_context() erases the client data and never calls remove on a
failed add.
A test-only delay before isert_device_get() opens the window. It opened in
one of twelve runs of patches 1 to 4 of this series, and that run built a
second isert_device and took the ten warnings below. It opened in six of
twelve runs with this patch, which built none and took none.
WARNING: drivers/infiniband/core/cq.c:370 at ib_cq_pool_cleanup+0x189/0x250, CPU#1: rdma/201
Modules linked in:
CPU: 1 UID: 0 PID: 201 Comm: rdma Not tainted 7.4.0-rc1-NW4-00006-ga65579195c15 #125 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
RIP: 0010:ib_cq_pool_cleanup+0x189/0x250
Code: 85 19 ff ff ff 48 8b 04 24 49 83 c4 10 49 39 c4 0f 85 cd fe ff ff 48 83 c4 18 5b 5d 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 90 <0f> 0b 90 e9 20 ff ff ff 48 89 44 24 08 e8 15 16 0b fe 48 8b 44 24
RSP: 0018:ffff88810b867308 EFLAGS: 00010206
RAX: ffff888109a7ec40 RBX: ffff888109ac5c00 RCX: 0000000000000007
RDX: 000000000000010c RSI: 0000000000000000 RDI: ffff888109ac5c2c
RBP: dffffc0000000000 R08: 0000000000000001 R09: 00000000000000bf
R10: 0000000000000003 R11: 000000000a739171 R12: ffff888104954ad0
R13: dead000000000122 R14: dead000000000100 R15: ffff888109a7ec00
FS: 00007f9437ecb740(0000) GS:ffff88818cade000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f5331739000 CR3: 0000000109a19004 CR4: 0000000000770ef0
PKRU: 55555554
Call Trace:
<TASK>
? remove_client_context+0xb7/0xf0
disable_device+0x13c/0x240
? __pfx_disable_device+0x10/0x10
? __pfx_mutex_lock+0x10/0x10
__ib_unregister_device+0x226/0x3f0
? __pfx_mutex_unlock+0x10/0x10
ib_unregister_device_and_put+0x50/0x70
nldev_dellink+0x28e/0x3d0
? kernel_text_address+0xa7/0x130
? __pfx_nldev_dellink+0x10/0x10
? __pfx_down_read+0x10/0x10
rdma_nl_rcv_msg+0x29c/0x4e0
? __pfx_rdma_nl_rcv_msg+0x10/0x10
rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
? __pfx_rdma_nl_rcv_skb.constprop.0.isra.0+0x10/0x10
netlink_unicast+0x68f/0x940
? __pfx_selinux_socket_getpeersec_dgram+0x10/0x10
? __pfx_netlink_unicast+0x10/0x10
netlink_sendmsg+0x810/0xf20
? __pfx_netlink_sendmsg+0x10/0x10
? __pfx__raw_spin_lock_irqsave+0x10/0x10
? selinux_socket_sendmsg+0x8c/0x270
? __sys_sendto+0x337/0x390
__sys_sendto+0x337/0x390
? __pfx___sys_sendto+0x10/0x10
? fdget+0x53/0x3b0
__x64_sys_sendto+0xdb/0x1b0
? fpregs_assert_state_consistent+0x56/0xe0
do_syscall_64+0xdd/0x4a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f9437ffa0d7
Code: c7 c0 ff ff ff ff eb be 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 80 3d 75 ef 0d 00 00 41 89 ca 74 10 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 69 c3 55 48 89 e5 53 48 83 ec 38 44 89 4d d0
RSP: 002b:00007ffd602e2628 EFLAGS: 00000202 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000561d7b001390 RCX: 00007f9437ffa0d7
RDX: 0000000000000018 RSI: 0000561d7b0002a0 RDI: 0000000000000004
RBP: 00007ffd602e2660 R08: 00007f94380f1200 R09: 000000000000000c
R10: 0000000000000000 R11: 0000000000000202 R12: 00007ffd602e2850
R13: 00007ffd602e2634 R14: 00007ffd602e2870 R15: 00007ffd602e2850
</TASK>
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 | 73 +++++++++++++++++++++----
1 file changed, 61 insertions(+), 12 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 712d6165150e3c..4bcd14f5045ac6 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -260,10 +260,12 @@ isert_device_put(struct isert_device *device)
isert_info("device %p refcount %d\n", device, device->refcount);
if (!device->refcount) {
isert_free_device_ib_res(device);
- list_del(&device->dev_node);
- if (device->removal_comp)
+ if (device->removing) {
complete(device->removal_comp);
- kfree(device);
+ } else {
+ list_del(&device->dev_node);
+ kfree(device);
+ }
}
mutex_unlock(&device_list_mutex);
}
@@ -276,14 +278,21 @@ isert_device_get(struct rdma_cm_id *cma_id)
mutex_lock(&device_list_mutex);
list_for_each_entry(device, &device_list, dev_node) {
- if (device->ib_device->node_guid == cma_id->device->node_guid) {
- device->refcount++;
- isert_info("Found iser device %p refcount %d\n",
- device, device->refcount);
+ if (device->removing && device->ib_device == cma_id->device) {
mutex_unlock(&device_list_mutex);
- return device;
+ return ERR_PTR(-ENODEV);
}
}
+ list_for_each_entry(device, &device_list, dev_node) {
+ if (device->removing ||
+ device->ib_device->node_guid != cma_id->device->node_guid)
+ continue;
+ device->refcount++;
+ isert_info("Found iser device %p refcount %d\n",
+ device, device->refcount);
+ mutex_unlock(&device_list_mutex);
+ return device;
+ }
device = kzalloc_obj(struct isert_device);
if (!device) {
@@ -2792,20 +2801,29 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
mutex_lock(&device_list_mutex);
list_for_each_entry(device, &device_list, dev_node) {
- if (device->ib_device != ib_device)
+ if (device->ib_device != ib_device || device->removing)
continue;
device->refcount++;
device->removal_comp = &removal_comp;
found = device;
break;
}
- mutex_unlock(&device_list_mutex);
-
- if (!found)
+ if (!found) {
+ device = kzalloc_obj(struct isert_device,
+ GFP_KERNEL | __GFP_NOFAIL);
+ INIT_LIST_HEAD(&device->dev_node);
+ device->ib_device = ib_device;
+ device->removing = true;
+ list_add_tail(&device->dev_node, &device_list);
+ mutex_unlock(&device_list_mutex);
return;
+ }
+ mutex_unlock(&device_list_mutex);
mutex_lock(&isert_conn_mutex);
+ mutex_lock(&device_list_mutex);
found->removing = true;
+ mutex_unlock(&device_list_mutex);
list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
if (isert_conn->device != found)
continue;
@@ -2828,8 +2846,38 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
wait_for_completion(&removal_comp);
}
+static int isert_add_one(struct ib_device *ib_device)
+{
+ struct isert_device *device, *tmp;
+
+ mutex_lock(&device_list_mutex);
+ list_for_each_entry_safe(device, tmp, &device_list, dev_node) {
+ if (device->ib_device != ib_device || !device->removing ||
+ device->refcount)
+ continue;
+ list_del(&device->dev_node);
+ kfree(device);
+ }
+ mutex_unlock(&device_list_mutex);
+
+ return 0;
+}
+
+static void isert_free_markers(void)
+{
+ struct isert_device *device, *tmp;
+
+ mutex_lock(&device_list_mutex);
+ list_for_each_entry_safe(device, tmp, &device_list, dev_node) {
+ list_del(&device->dev_node);
+ kfree(device);
+ }
+ mutex_unlock(&device_list_mutex);
+}
+
static struct ib_client isert_ib_client = {
.name = "isert",
+ .add = isert_add_one,
.remove = isert_remove_one,
};
@@ -2881,6 +2929,7 @@ static int __init isert_init(void)
static void __exit isert_exit(void)
{
ib_unregister_client(&isert_ib_client);
+ isert_free_markers();
flush_workqueue(isert_login_wq);
destroy_workqueue(isert_release_wq);
destroy_workqueue(isert_comp_wq);
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 5/5] IB/isert: do not build a new isert_device for a device being removed
2026-11-03 12:59 ` [PATCH v5 5/5] IB/isert: do not build a new isert_device for a device being removed Yehyeong Lee
@ 2026-11-03 13:39 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-03 13:39 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma, target-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] IB/isert: stale removal marker in isert_device_get() before isert_add_one()
- [Medium] IB/isert: unbounded leak of isert_device markers in device_list
- [Medium] IB/isert: memory leak of removal markers in isert_init() on error
--
commit 812d1184eb1546b67f3290f713769f5e49331bb1
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
IB/isert: do not build a new isert_device for a device being removed
This patch retains or allocates a removing struct isert_device marker on
device_list during isert_remove_one() so that concurrent isert_device_get()
calls reject devices being removed with -ENODEV. The markers are then freed
when a matching ib_device pointer is seen in isert_add_one() or when the
module unloads in isert_exit().
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 712d6165150e3..4bcd14f5045ac 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -260,10 +260,12 @@ isert_device_put(struct isert_device *device)
> isert_info("device %p refcount %d\n", device, device->refcount);
> if (!device->refcount) {
> isert_free_device_ib_res(device);
> - list_del(&device->dev_node);
> - if (device->removal_comp)
> + if (device->removing) {
> complete(device->removal_comp);
> - kfree(device);
> + } else {
> + list_del(&device->dev_node);
> + kfree(device);
> + }
> }
> mutex_unlock(&device_list_mutex);
> }
> @@ -276,14 +278,21 @@ isert_device_get(struct rdma_cm_id *cma_id)
>
> mutex_lock(&device_list_mutex);
> list_for_each_entry(device, &device_list, dev_node) {
> - if (device->ib_device->node_guid == cma_id->device->node_guid) {
> - device->refcount++;
> - isert_info("Found iser device %p refcount %d\n",
> - device, device->refcount);
> + if (device->removing && device->ib_device == cma_id->device) {
> mutex_unlock(&device_list_mutex);
> - return device;
> + return ERR_PTR(-ENODEV);
> }
> }
[Severity: High]
Can a stale removal marker in device_list cause isert_device_get() to reject
valid connection requests on a newly enabled or namespace-moved RDMA device
before isert_add_one() has a chance to clear it?
Because ib_isert depends on rdma_cm, cma_client is registered before
isert_ib_client. In enable_device_and_get(), client add callbacks run in
registration order:
drivers/infiniband/core/device.c:enable_device_and_get()
xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED)
add_client_context(device, client)
When cma_add_one() runs first, it immediately activates wildcard listeners
(such as an isert_np bound to INADDR_ANY) via cma_listen_on_dev() before
returning:
drivers/infiniband/core/cma.c:cma_add_one()
list_for_each_entry(id_priv, &listen_any_list, listen_any_item)
cma_listen_on_dev(id_priv, cma_dev, &to_destroy)
When the same struct ib_device pointer is re-enabled by
rdma_dev_change_netns() (which calls disable_device() followed by
enable_device_and_get() on the same struct ib_device) or when a freed
struct ib_device address is reused by the slab allocator, the marker left by
isert_remove_one() is still on device_list in the window after cma_add_one()
starts listening and before isert_add_one() runs. Any incoming
RDMA_CM_EVENT_CONNECT_REQUEST during that window will match
device->removing && device->ib_device == cma_id->device in the first pass of
isert_device_get() and fail with -ENODEV.
Additionally, if a connection request arrives on a new ib_device after
cma_add_one() starts listening and add_client_context() subsequently fails
before setting CLIENT_DATA_REGISTERED for isert_ib_client, isert_device_get()
will have built a live isert_device, but the error unwind in
disable_device() will skip isert_remove_one(), leaving its IB resources
unreleased.
> + list_for_each_entry(device, &device_list, dev_node) {
> + if (device->removing ||
> + device->ib_device->node_guid != cma_id->device->node_guid)
> + continue;
> + device->refcount++;
> + isert_info("Found iser device %p refcount %d\n",
> + device, device->refcount);
> + mutex_unlock(&device_list_mutex);
> + return device;
> + }
>
> device = kzalloc_obj(struct isert_device);
> if (!device) {
> @@ -2792,20 +2801,29 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
>
> mutex_lock(&device_list_mutex);
> list_for_each_entry(device, &device_list, dev_node) {
> - if (device->ib_device != ib_device)
> + if (device->ib_device != ib_device || device->removing)
> continue;
> device->refcount++;
> device->removal_comp = &removal_comp;
> found = device;
> break;
> }
> - mutex_unlock(&device_list_mutex);
> -
> - if (!found)
> + if (!found) {
> + device = kzalloc_obj(struct isert_device,
> + GFP_KERNEL | __GFP_NOFAIL);
> + INIT_LIST_HEAD(&device->dev_node);
> + device->ib_device = ib_device;
> + device->removing = true;
> + list_add_tail(&device->dev_node, &device_list);
> + mutex_unlock(&device_list_mutex);
> return;
> + }
> + mutex_unlock(&device_list_mutex);
>
> mutex_lock(&isert_conn_mutex);
> + mutex_lock(&device_list_mutex);
> found->removing = true;
> + mutex_unlock(&device_list_mutex);
> list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> if (isert_conn->device != found)
> continue;
> @@ -2828,8 +2846,38 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
> wait_for_completion(&removal_comp);
> }
>
> +static int isert_add_one(struct ib_device *ib_device)
> +{
> + struct isert_device *device, *tmp;
> +
> + mutex_lock(&device_list_mutex);
> + list_for_each_entry_safe(device, tmp, &device_list, dev_node) {
> + if (device->ib_device != ib_device || !device->removing ||
> + device->refcount)
> + continue;
> + list_del(&device->dev_node);
> + kfree(device);
> + }
> + mutex_unlock(&device_list_mutex);
> +
> + return 0;
> +}
[Severity: Medium]
Does this accumulate struct isert_device markers on device_list without
bound across RDMA device unregistrations?
Whenever any IB device is unregistered via __ib_unregister_device() ->
disable_device() -> remove_client_context() -> isert_remove_one() (even a
device never used by ib_isert), isert_remove_one() either keeps the existing
struct isert_device on device_list via isert_device_put() (with dangling
pointers in device->ib_device, device->pd, and the stack-allocated
device->removal_comp) or allocates a new struct isert_device marker with
GFP_KERNEL | __GFP_NOFAIL and appends it to device_list.
Because isert_add_one() only frees a marker if a newly registered device
happens to reuse the exact same struct ib_device pointer address
(device->ib_device == ib_device), any removed ib_device whose slab memory is
not reused by a later ib_device allocation (for example, during repeated
rxe/siw link deletion or VF hotplug cycles) leaves its struct isert_device
marker on device_list until module unload.
> +
> +static void isert_free_markers(void)
> +{
> + struct isert_device *device, *tmp;
> +
> + mutex_lock(&device_list_mutex);
> + list_for_each_entry_safe(device, tmp, &device_list, dev_node) {
> + list_del(&device->dev_node);
> + kfree(device);
> + }
> + mutex_unlock(&device_list_mutex);
> +}
> +
> static struct ib_client isert_ib_client = {
> .name = "isert",
> + .add = isert_add_one,
> .remove = isert_remove_one,
> };
>
> @@ -2881,6 +2929,7 @@ static int __init isert_init(void)
> static void __exit isert_exit(void)
> {
> ib_unregister_client(&isert_ib_client);
> + isert_free_markers();
[Severity: Medium]
Does isert_init() leak the struct isert_device markers if
ib_register_client(&isert_ib_client) fails partway through?
In ib_register_client(), if add_client_context() succeeds for one or more
devices and then fails on a later device, it rolls back by calling
ib_unregister_client():
drivers/infiniband/core/device.c:ib_register_client()
xa_for_each_marked (&devices, index, device, DEVICE_REGISTERED) {
ret = add_client_context(device, client);
if (ret)
goto out;
}
...
out:
...
if (need_unreg && ret)
ib_unregister_client(client);
That rollback invokes remove_client_context() -> isert_remove_one() for each
device where add_client_context() succeeded, which allocates a struct
isert_device marker and adds it to device_list. In isert_init(), the error
path jumps to destroy_release_wq without calling isert_free_markers():
drivers/infiniband/ulp/isert/ib_isert.c:isert_init()
ret = ib_register_client(&isert_ib_client);
if (ret) {
isert_err("Unable to register isert ib client\n");
goto destroy_release_wq;
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103125955.1530355-1-yhlee@isslab.korea.ac.kr?part=5
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 0/5] IB/isert: fix the device removal use-after-free
2026-11-03 12:59 [PATCH v5 0/5] IB/isert: fix the device removal use-after-free Yehyeong Lee
` (4 preceding siblings ...)
2026-11-03 12:59 ` [PATCH v5 5/5] IB/isert: do not build a new isert_device for a device being removed Yehyeong Lee
@ 2026-11-03 22:39 ` Yehyeong Lee
5 siblings, 0 replies; 12+ messages in thread
From: Yehyeong Lee @ 2026-11-03 22:39 UTC (permalink / raw)
To: Sagi Grimberg, Max Gurtovoy, Jason Gunthorpe, Leon Romanovsky
Cc: Martin K . Petersen, Mike Christie, linux-rdma, target-devel,
linux-kernel, Yehyeong Lee
This series is superseded by v6:
https://lore.kernel.org/r/20261103223802.1697607-1-yhlee@isslab.korea.ac.kr
^ permalink raw reply [flat|nested] 12+ messages in thread