* [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released
@ 2026-10-26 0:07 Yehyeong Lee
2026-10-26 0:20 ` sashiko-bot
2026-10-26 4:19 ` [PATCH v2] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
0 siblings, 2 replies; 5+ messages in thread
From: Yehyeong Lee @ 2026-10-26 0:07 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
Removing an RDMA device (rdma link delete, module unload, hot-unplug -
a local administrative action) while an iSER connection is in
ISER_CONN_UP, i.e. the CM handshake has completed but iscsi has not
yet accepted it through isert_accept_np(), lets the
RDMA_CM_EVENT_DEVICE_REMOVAL handler free the isert_conn while
isert_connect_release() is still tearing it down. A remote initiator
alone cannot trigger this; it can only supply the ISER_CONN_UP
connections that the local device removal then races against.
isert_cma_handler() waits for the connection to reach ISER_CONN_DOWN and
then frees it:
wait_event_interruptible(isert_conn->rem_wait,
isert_conn->state == ISER_CONN_DOWN);
kfree(isert_conn);
but isert_release_work() published ISER_CONN_DOWN before dropping the
last reference, so the condition becomes true at the start of the
teardown instead of at its end. The wake-up at the end of
isert_connect_release() is in the right place; the condition was not.
The handler could return and free isert_conn while
isert_connect_release() was still dereferencing it.
That ordering requirement is not incidental. Sagi Grimberg spelled it
out when this handling was introduced (2014-10-28,
<544FC5B2.70104@dev.mellanox.co.il>): "cma_remove_one sequence ...
requires the cmd is owner to finish cleanup before the end of the
callback because in the end of it, it will allow the device to remove."
Publish ISER_CONN_DOWN from isert_connect_release() instead, immediately
before the wake-up, so the condition and the signal refer to the same
point. It has exactly one store and one load in the tree, so nothing
else is affected. The store no longer takes isert_conn->mutex: the last
reference has been dropped, the only legitimate accessor left is the
rem_wait waiter, and wake_up_interruptible() provides the ordering.
isert_conn->state is already accessed without that mutex in
isert_accept_np() and in the wait condition itself.
One visible side effect: the connection now stays in
ISER_CONN_TERMINATING until isert_connect_release() runs, so a second CM
event in that window takes the ISER_CONN_TERMINATING case instead of
default:, and the "conn %p terminating in state %d" warning is no longer
logged.
Reproduced with soft-RoCE (rdma_rxe) under KASAN, preempt=full, an
initiator opening 32 CM-only connections (no login PDU) concurrently, so
that they queue up in ISER_CONN_UP, followed by "rdma link delete". The
window is sub-microsecond, so the rate is low. The splat below
reproduces on an unmodified kernel and does not with this patch applied
(36 of 60 runs unpatched, 0 of 60 patched).
BUG: KASAN: slab-use-after-free in isert_connect_release+0x5cb/0x620
Read of size 4 at addr ffff88810ca1d158 by task kworker/u16:1/43
CPU: 0 UID: 0 PID: 43 Comm: kworker/u16:1 Not tainted 7.3.0-CTL2-13247-g330517c63aa9 #1 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 24:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
__kasan_kmalloc+0x8f/0xa0
__kmalloc_cache_noprof+0x148/0x360
isert_cma_handler+0x1e3/0x2840
cma_cm_event_handler+0x3e/0x240
cma_ib_req_handler+0x17d9/0x4480
cm_process_work+0x41/0x330
cm_work_handler+0x5727/0xc160
process_one_work+0x697/0x1060
worker_thread+0x45b/0xd10
kthread+0x2c8/0x3b0
ret_from_fork+0x36e/0x5a0
ret_from_fork_asm+0x1a/0x30
Freed by task 218:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
kasan_save_free_info+0x3b/0x60
__kasan_slab_free+0x43/0x70
kfree+0x121/0x390
isert_cma_handler+0x10ff/0x2840
cma_cm_event_handler+0x3e/0x240
cma_process_remove+0xff/0x5a0
cma_remove_one+0xcc/0x250
remove_client_context+0xa4/0xf0
disable_device+0x128/0x240
__ib_unregister_device+0x226/0x3f0
ib_unregister_device_and_put+0x50/0x70
nldev_dellink+0x28e/0x3d0
rdma_nl_rcv_msg+0x29c/0x4e0
rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
netlink_unicast+0x68f/0x940
netlink_sendmsg+0x810/0xf20
__sys_sendto+0x337/0x390
__x64_sys_sendto+0xdb/0x1b0
do_syscall_64+0xdd/0x4a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Not fixed here: a connection still in ISER_CONN_INIT when the device is
removed is never cleaned up - isert_disconnected_handler() only warns -
so nothing makes the wait condition true and the removal blocks. That
pre-existing case is why the wait is left interruptible rather than
converted to wait_event().
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 | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index efa8d7b7844c9..cd0ca3cf131cb 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -506,10 +506,18 @@ isert_connect_release(struct isert_conn *isert_conn)
isert_device_put(device);
- if (isert_conn->dev_removed)
+ if (isert_conn->dev_removed) {
+ /*
+ * Publish ISER_CONN_DOWN only now. The
+ * RDMA_CM_EVENT_DEVICE_REMOVAL handler waits for it and then
+ * frees isert_conn, so it must not become visible before the
+ * teardown above has finished using the connection.
+ */
+ isert_conn->state = ISER_CONN_DOWN;
wake_up_interruptible(&isert_conn->rem_wait);
- else
+ } else {
kfree(isert_conn);
+ }
}
static void
@@ -2537,10 +2545,6 @@ static void isert_release_work(struct work_struct *work)
isert_info("Starting release conn %p\n", isert_conn);
- mutex_lock(&isert_conn->mutex);
- isert_conn->state = ISER_CONN_DOWN;
- mutex_unlock(&isert_conn->mutex);
-
isert_info("Destroying conn %p\n", isert_conn);
isert_put_conn(isert_conn);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released
2026-10-26 0:07 [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released Yehyeong Lee
@ 2026-10-26 0:20 ` sashiko-bot
2026-10-26 4:43 ` Yehyeong Lee
2026-10-26 4:19 ` [PATCH v2] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
1 sibling, 1 reply; 5+ messages in thread
From: sashiko-bot @ 2026-10-26 0:20 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma, target-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] IB/isert: UAF on rem_wait in isert_connect_release() after ISER_CONN_DOWN
- [High] IB/isert: deadlock on handler_mutex in isert_connect_release() on device removal
--
commit dc719d99e88528503ed27ec0d20e073beee4b8f9
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
IB/isert: set ISER_CONN_DOWN only after the connection has been released
This commit moves the store setting isert_conn->state to ISER_CONN_DOWN from
isert_release_work() into isert_connect_release() right before waking
isert_conn->rem_wait. This is intended to prevent the
RDMA_CM_EVENT_DEVICE_REMOVAL handler in isert_cma_handler() from freeing
isert_conn while isert_connect_release() is still tearing down connection
resources.
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..cd0ca3cf131cb 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -506,10 +506,18 @@ isert_connect_release(struct isert_conn *isert_conn)
>
> isert_device_put(device);
>
> - if (isert_conn->dev_removed)
> + if (isert_conn->dev_removed) {
> + /*
> + * Publish ISER_CONN_DOWN only now. The
> + * RDMA_CM_EVENT_DEVICE_REMOVAL handler waits for it and then
> + * frees isert_conn, so it must not become visible before the
> + * teardown above has finished using the connection.
> + */
> + isert_conn->state = ISER_CONN_DOWN;
> wake_up_interruptible(&isert_conn->rem_wait);
[Severity: Critical]
Can publishing isert_conn->state = ISER_CONN_DOWN before calling
wake_up_interruptible(&isert_conn->rem_wait) cause a use-after-free on
isert_conn->rem_wait when racing with kfree(isert_conn) in
isert_cma_handler()?
In drivers/infiniband/ulp/isert/ib_isert.c, isert_cma_handler() handles
RDMA_CM_EVENT_DEVICE_REMOVAL with:
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);
Because rem_wait is embedded inside struct isert_conn and
wait_event_interruptible() checks isert_conn->state == ISER_CONN_DOWN before
acquiring rem_wait.lock (and again after prepare_to_wait_event() before
schedule()), as soon as the store to isert_conn->state becomes visible on
another CPU, wait_event_interruptible() can return immediately and call
kfree(isert_conn) before wake_up_interruptible(&isert_conn->rem_wait) even
begins. In that fast-path case, no barrier in wake_up_interruptible() has
executed to order prior accesses in isert_connect_release() before
kfree(isert_conn), and wake_up_interruptible() -> __wake_up_common_lock() ->
spin_lock_irqsave(&wq_head->lock, flags) will then dereference and write to
the freed isert_conn->rem_wait.
Even when the waiter is asleep in schedule(), __wake_up_common_lock() invokes
autoremove_wake_function(), which wakes the task and removes wq_entry from
rem_wait.head via list_del_init_careful(&wq_entry->entry) before releasing
rem_wait.lock. The woken waiter can observe isert_conn->state ==
ISER_CONN_DOWN, see list_empty_careful(&wq_entry->entry) in finish_wait()
without taking rem_wait.lock, and call kfree(isert_conn) while
__wake_up_common_lock() in isert_connect_release() is still traversing
rem_wait.head and unlocking isert_conn->rem_wait.lock.
Would replacing rem_wait with a struct completion (using complete() and
wait_for_completion_interruptible(), which serialize completion state and
waiter exit under the completion's wait.lock) avoid this race?
> - else
> + } else {
> kfree(isert_conn);
> + }
> }
>
> static void
> @@ -2537,10 +2545,6 @@ static void isert_release_work(struct work_struct *work)
>
> isert_info("Starting release conn %p\n", isert_conn);
>
> - mutex_lock(&isert_conn->mutex);
> - isert_conn->state = ISER_CONN_DOWN;
> - mutex_unlock(&isert_conn->mutex);
[Severity: High]
Does moving the ISER_CONN_DOWN transition from isert_release_work() to the end
of isert_connect_release() introduce a deadlock on id_priv->handler_mutex when
a normal connection teardown races with RDMA device removal?
In drivers/infiniband/ulp/isert/ib_isert.c, isert_connect_release() runs after
isert_release_work() drops the last reference and checks dev_removed before
destroying the CM ID:
static void
isert_connect_release(struct isert_conn *isert_conn)
{
...
if (isert_conn->cm_id &&
!isert_conn->dev_removed)
rdma_destroy_id(isert_conn->cm_id);
If a connection begins normal teardown prior to device removal (for example,
via RDMA_CM_EVENT_DISCONNECTED or iscsit_close_connection() ->
isert_wait_conn()), isert_conn->state is set to ISER_CONN_TERMINATING with
isert_conn->dev_removed == false, and isert_release_work() invokes
isert_connect_release().
If isert_connect_release() observes !isert_conn->dev_removed and enters
rdma_destroy_id(isert_conn->cm_id) just as cma_remove_one() ->
cma_process_remove() -> cma_send_device_removal_put() acquires
id_priv->handler_mutex first:
CPU0 (isert_release_wq) CPU1 (device removal)
isert_release_work()
isert_put_conn()
isert_connect_release()
sees !isert_conn->dev_removed
rdma_destroy_id(cm_id)
cma_send_device_removal_put()
mutex_lock(&id_priv->handler_mutex)
cma_ndev_work / cma_cm_event_handler
isert_cma_handler()
isert_conn->dev_removed = true
isert_disconnected_handler()
(sees ISER_CONN_TERMINATING)
wait_event_interruptible(
isert_conn->rem_wait,
state == ISER_CONN_DOWN)
mutex_lock(&id_priv->handler_mutex)
Because ISER_CONN_DOWN is now set only after rdma_destroy_id() returns in
isert_connect_release(), CPU0 blocks waiting for id_priv->handler_mutex inside
rdma_destroy_id() while CPU1 holds id_priv->handler_mutex waiting for
isert_conn->state == ISER_CONN_DOWN. Furthermore, if the interruptible wait on
CPU1 is interrupted by a signal, isert_cma_handler() frees isert_conn and
returns 1 to destroy cm_id while rdma_destroy_id() on CPU0 proceeds to destroy
cm_id a second time and then accesses the freed isert_conn.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026000733.4178856-1-yhlee@isslab.korea.ac.kr?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] IB/isert: tear connections down from an ib_client on device removal
2026-10-26 0:07 [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released Yehyeong Lee
2026-10-26 0:20 ` sashiko-bot
@ 2026-10-26 4:19 ` Yehyeong Lee
2026-10-26 9:09 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Yehyeong Lee @ 2026-10-26 4:19 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 to
reach ISER_CONN_DOWN and then frees it:
wait_event_interruptible(isert_conn->rem_wait,
isert_conn->state == ISER_CONN_DOWN);
kfree(isert_conn);
That is a use-after-free. isert_release_work() publishes ISER_CONN_DOWN
before the last reference is dropped, so the condition is already true
when isert_connect_release() starts and the callback frees isert_conn
while isert_connect_release() is still dereferencing it. Publishing the
state next to the wake-up instead does not close it either, because the
condition is evaluated without any lock, so the waiter can leave the wait
and free isert_conn while the releasing thread is still inside
wake_up_interruptible() taking isert_conn->rem_wait.lock.
The wait also deadlocks. cma_send_device_removal_put() runs the callback
with the cm_id's handler_mutex held and rdma_destroy_id() takes that same
mutex, so a teardown that has already passed the isert_conn->dev_removed
test in isert_connect_release() blocks there while the callback waits for
that teardown to publish ISER_CONN_DOWN. Nothing in the callback can
test whether the teardown has reached that point.
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 once it is done with the
device. nvmet-rdma fixed the same two problems the same way, see
commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock in
device removal").
The cm callback now returns zero for RDMA_CM_EVENT_DEVICE_REMOVAL and
keeps the cm_id, which leaves isert_connect_release() as the only place
that destroys it, so isert_conn->dev_removed and isert_conn->rem_wait are
gone. A connection that never reached RDMA_CM_EVENT_ESTABLISHED now
delays the removal until the CM reports RDMA_CM_EVENT_UNREACHABLE and
isert_connect_error() drops the last reference. Before this patch that
case blocked for good, because that CM event needs the handler_mutex the
waiting callback holds.
Measured with soft-RoCE under KASAN on a local device removal racing 32
connections: 11 of 20 unpatched runs hit the splat below and 0 of 20
patched runs did, and the removal itself finished in all 20 patched runs
and in none of the unpatched ones. With the wait kept and its window
widened, 3 of 3 runs deadlocked and 0 of 3 did with this patch. Normal
login and I/O passed 24 of 30 runs on either side of the patch.
BUG: KASAN: slab-use-after-free in isert_connect_release+0x5cb/0x620
Read of size 4 at addr ffff88810acbd158 by task kworker/u16:5/70
CPU: 1 UID: 0 PID: 70 Comm: kworker/u16:5 Not tainted 7.3.0-CTL2-13247-g330517c63aa9 #1 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_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 81:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
__kasan_kmalloc+0x8f/0xa0
__kmalloc_cache_noprof+0x148/0x360
isert_cma_handler+0x1e3/0x2840
cma_cm_event_handler+0x3e/0x240
cma_ib_req_handler+0x17d9/0x4480
cm_process_work+0x41/0x330
cm_work_handler+0x5727/0xc160
process_one_work+0x697/0x1060
worker_thread+0x45b/0xd10
kthread+0x2c8/0x3b0
ret_from_fork+0x36e/0x5a0
ret_from_fork_asm+0x1a/0x30
Freed by task 218:
kasan_save_stack+0x33/0x60
kasan_save_track+0x14/0x30
kasan_save_free_info+0x3b/0x60
__kasan_slab_free+0x43/0x70
kfree+0x121/0x390
isert_cma_handler+0x10ff/0x2840
cma_cm_event_handler+0x3e/0x240
cma_process_remove+0xff/0x5a0
cma_remove_one+0xcc/0x250
remove_client_context+0xa4/0xf0
disable_device+0x128/0x240
__ib_unregister_device+0x226/0x3f0
ib_unregister_device_and_put+0x50/0x70
nldev_dellink+0x28e/0x3d0
rdma_nl_rcv_msg+0x29c/0x4e0
rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
netlink_unicast+0x68f/0x940
netlink_sendmsg+0x810/0xf20
__sys_sendto+0x337/0x390
__x64_sys_sendto+0xdb/0x1b0
do_syscall_64+0xdd/0x4a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
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>
---
v1 introduced a handler_mutex deadlock and did not close the rem_wait
use-after-free (review); redesigned to tear down from an ib_client
(nvmet-rdma pattern).
drivers/infiniband/ulp/isert/ib_isert.c | 129 +++++++++++++++++++-----
drivers/infiniband/ulp/isert/ib_isert.h | 4 +-
2 files changed, 103 insertions(+), 30 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index efa8d7b7844c9..35e4b9c917bd6 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;
@@ -255,6 +257,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 +313,7 @@ isert_init_conn(struct isert_conn *isert_conn)
INIT_LIST_HEAD(&isert_conn->node);
init_completion(&isert_conn->login_comp);
init_completion(&isert_conn->login_req_comp);
- init_waitqueue_head(&isert_conn->rem_wait);
+ INIT_LIST_HEAD(&isert_conn->conn_list);
kref_init(&isert_conn->kref);
atomic_set(&isert_conn->ctrl_comp_cnt, 0);
mutex_init(&isert_conn->mutex);
@@ -468,6 +472,10 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
list_add_tail(&isert_conn->node, &isert_np->accepted);
mutex_unlock(&isert_np->mutex);
+ mutex_lock(&isert_conn_mutex);
+ list_add_tail(&isert_conn->conn_list, &isert_conn_list);
+ mutex_unlock(&isert_conn_mutex);
+
return 0;
out_destroy_qp:
@@ -491,9 +499,12 @@ isert_connect_release(struct isert_conn *isert_conn)
BUG_ON(!device);
+ mutex_lock(&isert_conn_mutex);
+ list_del_init(&isert_conn->conn_list);
+ mutex_unlock(&isert_conn_mutex);
+
isert_free_rx_descriptors(isert_conn);
- if (isert_conn->cm_id &&
- !isert_conn->dev_removed)
+ if (isert_conn->cm_id)
rdma_destroy_id(isert_conn->cm_id);
if (isert_conn->qp)
@@ -504,12 +515,13 @@ 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);
+ /*
+ * Releases isert_remove_one() once the last connection running on
+ * this device is gone.
+ */
+ isert_device_put(device);
}
static void
@@ -625,13 +637,10 @@ 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)
+/* Must be called with isert_conn->mutex held. */
+static void
+isert_teardown_conn(struct isert_conn *isert_conn)
{
- struct isert_conn *isert_conn = cma_id->qp->qp_context;
-
- mutex_lock(&isert_conn->mutex);
switch (isert_conn->state) {
case ISER_CONN_TERMINATING:
break;
@@ -648,6 +657,16 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
isert_warn("conn %p terminating in state %d\n",
isert_conn, isert_conn->state);
}
+}
+
+static int
+isert_disconnected_handler(struct rdma_cm_id *cma_id,
+ enum rdma_cm_event_type event)
+{
+ struct isert_conn *isert_conn = cma_id->qp->qp_context;
+
+ mutex_lock(&isert_conn->mutex);
+ isert_teardown_conn(isert_conn);
mutex_unlock(&isert_conn->mutex);
return 0;
@@ -674,7 +693,6 @@ static int
isert_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
{
struct isert_np *isert_np = cma_id->context;
- struct isert_conn *isert_conn;
int ret = 0;
isert_info("%s (%d): status %d id %p np %p\n",
@@ -695,21 +713,15 @@ 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 */
- ret = isert_disconnected_handler(cma_id, event->event);
- break;
+ case RDMA_CM_EVENT_TIMEWAIT_EXIT:
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
+ * Device removal is driven by isert_remove_one(). Returning
+ * zero here keeps the cm_id; isert_connect_release() destroys
+ * it once the connection has been released.
*/
- return 1;
+ ret = isert_disconnected_handler(cma_id, event->event);
+ break;
case RDMA_CM_EVENT_REJECTED:
isert_info("Connection rejected: %s\n",
rdma_reject_msg(cma_id, event->status));
@@ -2689,8 +2701,60 @@ 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;
+
+ /*
+ * Not from the RDMA_CM_EVENT_DEVICE_REMOVAL callback: that one runs
+ * with the cm_id's handler mutex held, which a teardown already
+ * destroying that cm_id needs. Same reason as
+ * commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not
+ * deadlock in device removal").
+ */
+ 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);
+ list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
+ if (isert_conn->device != found)
+ continue;
+
+ isert_info("removing conn %p state %d\n",
+ isert_conn, isert_conn->state);
+ mutex_lock(&isert_conn->mutex);
+ isert_teardown_conn(isert_conn);
+ mutex_unlock(&isert_conn->mutex);
+ }
+ mutex_unlock(&isert_conn_mutex);
+
+ isert_device_put(found);
+ wait_for_completion(&removal_comp);
+}
+
+static struct ib_client isert_ib_client = {
+ .name = "isert",
+ .remove = isert_remove_one,
+};
+
static int __init isert_init(void)
{
+ int ret = -ENOMEM;
+
isert_login_wq = alloc_workqueue("isert_login_wq", WQ_PERCPU, 0);
if (!isert_login_wq) {
isert_err("Unable to allocate isert_login_wq\n");
@@ -2711,21 +2775,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 519b17e54bd34..41023300be21b 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -191,8 +191,7 @@ struct isert_conn {
atomic_t ctrl_comp_cnt;
bool logout_posted;
bool snd_w_inv;
- wait_queue_head_t rem_wait;
- bool dev_removed;
+ struct list_head conn_list;
};
struct isert_device {
@@ -203,6 +202,7 @@ struct isert_device {
struct isert_comp *comps;
int comps_used;
struct list_head dev_node;
+ struct completion *removal_comp;
};
struct isert_np {
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released
2026-10-26 0:20 ` sashiko-bot
@ 2026-10-26 4:43 ` Yehyeong Lee
0 siblings, 0 replies; 5+ messages in thread
From: Yehyeong Lee @ 2026-10-26 4:43 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-rdma, target-devel, Yehyeong Lee
Thanks for the review. Both are real. v2 drops the rem_wait wait entirely
and tears the connections down from an ib_client removal callback, so
neither the use-after-free nor the handler_mutex deadlock can happen:
https://lore.kernel.org/linux-rdma/20261026041949.225619-1-yhlee@isslab.korea.ac.kr/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] IB/isert: tear connections down from an ib_client on device removal
2026-10-26 4:19 ` [PATCH v2] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
@ 2026-10-26 9:09 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-26 9:09 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-rdma, target-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] IB/isert: race between isert_connected_handler() and isert_remove_one()
- [High] IB/isert: hang in isert_remove_one() on ISER_CONN_INIT and racing connects
--
commit 73bf46b98b888cdf4cb06784ff6926d47808ad06
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
IB/isert: tear connections down from an ib_client on device removal
This commit replaces the device removal wait in isert_cma_handler() with
an ib_client remove callback, isert_remove_one(), to tear down active
connections and wait for the final isert_device reference to be dropped
without holding the cm_id handler mutex.
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..35e4b9c917bd6 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
[ ... ]
> @@ -625,13 +637,10 @@ 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)
> +/* Must be called with isert_conn->mutex held. */
> +static void
> +isert_teardown_conn(struct isert_conn *isert_conn)
> {
> - struct isert_conn *isert_conn = cma_id->qp->qp_context;
> -
> - mutex_lock(&isert_conn->mutex);
> switch (isert_conn->state) {
> case ISER_CONN_TERMINATING:
> break;
> @@ -648,6 +657,16 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
> isert_warn("conn %p terminating in state %d\n",
> isert_conn, isert_conn->state);
> }
> +}
[ ... ]
> @@ -695,21 +713,15 @@ 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 */
> - ret = isert_disconnected_handler(cma_id, event->event);
> - break;
> + case RDMA_CM_EVENT_TIMEWAIT_EXIT:
> 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
> + * Device removal is driven by isert_remove_one(). Returning
> + * zero here keeps the cm_id; isert_connect_release() destroys
> + * it once the connection has been released.
> */
> - return 1;
> + ret = isert_disconnected_handler(cma_id, event->event);
> + break;
[ ... ]
> @@ -2689,8 +2701,60 @@ 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;
> +
> + /*
> + * Not from the RDMA_CM_EVENT_DEVICE_REMOVAL callback: that one runs
> + * with the cm_id's handler mutex held, which a teardown already
> + * destroying that cm_id needs. Same reason as
> + * commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not
> + * deadlock in device removal").
> + */
> + 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);
> + list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> + if (isert_conn->device != found)
> + continue;
> +
> + isert_info("removing conn %p state %d\n",
> + isert_conn, isert_conn->state);
> + mutex_lock(&isert_conn->mutex);
> + isert_teardown_conn(isert_conn);
> + mutex_unlock(&isert_conn->mutex);
[Severity: Critical]
Can calling isert_teardown_conn() here without the cm_id handler_mutex
race with isert_connected_handler() and corrupt isert_np->pending or cause
a use-after-free of isert_conn?
Previously, both isert_connected_handler() and
isert_disconnected_handler() were only called from isert_cma_handler(),
which is serialized per cm_id by id_priv->handler_mutex.
Because disable_device() invokes ib_client remove callbacks before
cma_remove_one(), isert_remove_one() runs while RDMA_CM_EVENT_ESTABLISHED
can still be delivered to isert_connected_handler():
drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
mutex_lock(&isert_conn->mutex);
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);
}
If isert_remove_one() acquires isert_conn->mutex in the window after
isert_connected_handler() drops isert_conn->mutex and before it acquires
isert_np->mutex, isert_teardown_conn() sees ISER_CONN_UP, transitions the
connection to ISER_CONN_TERMINATING, and calls
isert_handle_unbound_conn():
drivers/infiniband/ulp/isert/ib_isert.c:isert_handle_unbound_conn() {
mutex_lock(&isert_np->mutex);
if (!list_empty(&isert_conn->node)) {
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);
}
Since isert_conn->node is still on isert_np->accepted at that moment,
isert_handle_unbound_conn() deletes it from isert_np->accepted, drops one
kref, and queues isert_conn->release_work.
When isert_connected_handler() then acquires isert_np->mutex, it executes
list_move_tail(&isert_conn->node, &isert_np->pending) and links the
terminating connection onto isert_np->pending while isert_release_work()
runs concurrently, drops the remaining kref, and frees isert_conn in
isert_connect_release().
> + }
> + mutex_unlock(&isert_conn_mutex);
> +
> + isert_device_put(found);
> + wait_for_completion(&removal_comp);
[Severity: High]
Could wait_for_completion(&removal_comp) hang indefinitely here when a
connection is still in ISER_CONN_INIT or when new connect requests arrive
during device removal?
In drivers/infiniband/core/device.c, disable_device() invokes client
remove callbacks in reverse registration order. Because ib_isert registers
isert_ib_client after rdma_cm and ib_cm register their clients,
isert_remove_one() runs to completion before cma_remove_one() starts (and
during module unload in isert_exit(), cma_remove_one() does not run at
all). This leaves a few paths where found->refcount is never dropped:
1. If a connection has been accepted by isert_connect_request() and is in
ISER_CONN_INIT when isert_remove_one() scans isert_conn_list,
isert_teardown_conn() hits the default case, logs a warning, and leaves
the connection in ISER_CONN_INIT. Because the CM is still active while
isert_remove_one() waits on removal_comp, an incoming RTU from the peer
will deliver RDMA_CM_EVENT_ESTABLISHED to isert_connected_handler(),
transitioning the connection to ISER_CONN_UP after isert_remove_one() has
already finished scanning isert_conn_list. At that point
RDMA_CM_EVENT_UNREACHABLE will never fire and cma_remove_one() is blocked
behind isert_remove_one(), so found->refcount never reaches zero.
2. Because the listening cm_id (isert_np->cm_id) is not destroyed until
cma_remove_one() runs after isert_remove_one(), a racing
RDMA_CM_EVENT_CONNECT_REQUEST in isert_connect_request() can call
isert_device_get() (which increments found->refcount even when
found->removal_comp is set) and append a new connection to isert_conn_list
after isert_remove_one() has already scanned the list.
3. If a new RDMA_CM_EVENT_CONNECT_REQUEST arrives after isert_remove_one()
returns (for instance when found was NULL or right after removal_comp
completes) and before cma_remove_one() destroys the listener,
isert_connect_request() creates a connection in ISER_CONN_INIT. When
cma_process_remove() -> cma_send_device_removal_put() then runs on that
cm_id, it sets id_priv->state = RDMA_CM_DEVICE_REMOVAL (suppressing any
later RDMA_CM_EVENT_UNREACHABLE in cma_ib_handler() / cma_iw_handler())
and calls isert_cma_handler(RDMA_CM_EVENT_DEVICE_REMOVAL). That calls
isert_teardown_conn(), which does nothing for ISER_CONN_INIT, and returns
0, leaving the cm_id undestroyed and blocking cma_process_remove() in
wait_for_completion(&cma_dev->comp).
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026041949.225619-1-yhlee@isslab.korea.ac.kr?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-10-26 9:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-26 0:07 [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released Yehyeong Lee
2026-10-26 0:20 ` sashiko-bot
2026-10-26 4:43 ` Yehyeong Lee
2026-10-26 4:19 ` [PATCH v2] IB/isert: tear connections down from an ib_client on device removal Yehyeong Lee
2026-10-26 9:09 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox