* [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf
@ 2026-07-27 2:14 ` Bryam Vargas
0 siblings, 0 replies; 4+ messages in thread
From: Bryam Vargas @ 2026-07-27 2:14 UTC (permalink / raw)
To: Dust Li, Wenjia Zhang, D. Wythe, Sidraya Jayagond
Cc: Mahanta Jambigi, netdev, linux-s390, linux-kernel, Tony Lu,
Paolo Abeni, Wen Gu, linux-rdma
On the SMC-D DMB-nocopy path smcd_buf_detach() frees conn->sndbuf_desc
while the receive tasklet can still be running: both teardown sites free
first and drain afterwards. A tasklet already past the conn->killed check
then dereferences the freed buffer in smc_cdc_msg_recv_action(), which
reaches the same field again through smc_tx_prepared_sends().
Drain the tasklet before the detach at both sites, mirroring rmb_desc,
which smc_buf_unuse() only releases after the drain.
Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Found with KASAN on an SMC-D loopback rig (CONFIG_SMC_LO, x86_64).
The teardown is driven through smc_conn_kill() by arming abort_work from a debug
module parameter: on SMC-D nothing in the receive path queues that work, while on
SMC-R smc_cdc_msg_validate() queues it for an out-of-range peer seqno. Everything
after the abort is the unmodified path. A second parameter holds the receive tasklet
just past the conn->killed check, so the free lands mid-run.
arm ordering abort armed result
vulnerable detach then drain yes KASAN slab-use-after-free
control detach then drain no clean
patched drain then detach yes clean
Same module, same boot, same trigger, same delay; only the ordering differs.
BUG: KASAN: slab-use-after-free in smc_cdc_msg_recv_action+0x1ed9/0x1f20 [smc]
Read of size 4 at addr ffff88810f6c9720 by task kworker/3:1/147
Workqueue: smc_close_wq smc_conn_abort_work [smc]
<IRQ>
smc_cdc_msg_recv_action+0x1ed9/0x1f20 [smc]
smcd_cdc_rx_tsklet+0x1f2/0x2f0 [smc]
tasklet_action_common+0x2fd/0x850
handle_softirqs+0x18c/0x4f0
do_softirq+0x3a/0x60
</IRQ>
<TASK>
__local_bh_enable_ip+0x5d/0x60
smc_cdc_get_slot_and_msg_send+0x253/0x680 [smc]
smc_conn_kill+0xa8/0x4d0 [smc]
smc_conn_abort_work+0x34/0x90 [smc]
</TASK>
Allocated by task 70:
smcd_buf_attach+0xbd/0x2a0 [smc]
smc_listen_work+0x2499/0x49a0 [smc]
Freed by task 44:
kfree+0x135/0x390
smc_conn_kill+0x3ab/0x4d0 [smc]
smc_conn_abort_work+0x34/0x90 [smc]
The buggy address belongs to the object at ffff88810f6c9700
which belongs to the cache kmalloc-128 of size 128
The buggy address is located 32 bytes inside of
freed 128-byte region [ffff88810f6c9700, ffff88810f6c9780)
The read is conn->sndbuf_desc->len.
A plain close() does not get there: smc_close_active() always moves the socket to
SMC_PEERCLOSEWAIT1, so the free happens in smc_close_passive_work() after the peer's
close CDC -- the same event that stops feeding the tasklet. About 18000 such teardowns
produced nothing. The ordering is wrong either way, and smc_conn_kill() has no such
coupling.
Logs on request.
---
net/smc/smc_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index cf6b620fef05..da64e7a6bd73 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1270,9 +1270,9 @@ void smc_conn_free(struct smc_connection *conn)
if (lgr->is_smcd) {
if (!list_empty(&lgr->list))
smc_ism_unset_conn(conn);
+ tasklet_kill(&conn->rx_tsklet);
if (smc_ism_support_dmb_nocopy(lgr->smcd))
smcd_buf_detach(conn);
- tasklet_kill(&conn->rx_tsklet);
} else {
smc_cdc_wait_pend_tx_wr(conn);
if (current_work() != &conn->abort_work)
@@ -1525,12 +1525,12 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
smc_sk_wake_ups(smc);
if (conn->lgr->is_smcd) {
smc_ism_unset_conn(conn);
- if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
- smcd_buf_detach(conn);
if (soft)
tasklet_kill(&conn->rx_tsklet);
else
tasklet_unlock_wait(&conn->rx_tsklet);
+ if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
+ smcd_buf_detach(conn);
} else {
smc_cdc_wait_pend_tx_wr(conn);
}
---
base-commit: 4235cb24ec1e8e96843f3671ba4da2a6ccca2c7b
change-id: 20260726-b4-disp-a135b4e5-945574bd0938
Best regards,
--
Bryam Vargas <hexlabsecurity@proton.me>
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf
2026-07-27 2:14 ` Bryam Vargas
(?)
@ 2026-07-28 2:14 ` sashiko-bot
-1 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-28 2:14 UTC (permalink / raw)
To: Bryam Vargas; +Cc: Alexander Gordeev, Heiko Carstens, linux-s390, Vasily Gorbik
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] The receive tasklet can be scheduled after `smc_conn_free()` frees `sndbuf_desc`, leading to a NULL pointer dereference crash.
- [High] Bitfield data race between softirq and process contexts causing silent loss of connection state updates (`killed` or `freed`).
- [High] Using `tasklet_unlock_wait()` fails to unschedule pending tasklets, leading to a UAF when the socket is freed.
- [High] The patch fails to resolve the UAF in the `soft == false` path due to a missing memory barrier, leading to a Store-Load reordering race.
--
--- Patch [1]: [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf ---
commit 95efe812ae27436c2b5367e550974e031445da1a
Author: Bryam Vargas <hexlabsecurity@proton.me>
net/smc: drain the rx tasklet before detaching the ghost sndbuf
This commit addresses a use-after-free issue where the receive tasklet
dereferences a freed send buffer descriptor. It reorders the teardown
sequence in smc_conn_free() and smc_conn_kill() to drain the receive
tasklet before detaching the buffer.
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index cf6b620fef05f..da64e7a6bd731 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1270,9 +1270,9 @@ void smc_conn_free(struct smc_connection *conn)
> if (lgr->is_smcd) {
> if (!list_empty(&lgr->list))
> smc_ism_unset_conn(conn);
> + tasklet_kill(&conn->rx_tsklet);
> if (smc_ism_support_dmb_nocopy(lgr->smcd))
> smcd_buf_detach(conn);
[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference if a concurrent socket closure races with device teardown?
During device teardown, __smc_lgr_terminate() removes the link group from the
list, so list_empty(&lgr->list) is true. This causes smc_conn_free() to skip
calling smc_ism_unset_conn(), leaving the connection registered in smcd->conn.
Even though tasklet_kill() is called here, a subsequent ISM hardware IRQ will
find the connection, check conn->killed (which may still be 0), and schedule
the tasklet again. The tasklet executes smc_cdc_msg_recv_action(), which then
dereferences the NULL conn->sndbuf_desc.
> - tasklet_kill(&conn->rx_tsklet);
> } else {
> smc_cdc_wait_pend_tx_wr(conn);
[ ... ]
> @@ -1525,12 +1525,12 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
> smc_sk_wake_ups(smc);
> if (conn->lgr->is_smcd) {
> smc_ism_unset_conn(conn);
> - if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
> - smcd_buf_detach(conn);
> if (soft)
> tasklet_kill(&conn->rx_tsklet);
> else
> tasklet_unlock_wait(&conn->rx_tsklet);
[Severity: High]
This is a pre-existing issue, but does using tasklet_unlock_wait() here
properly unschedule pending tasklets?
When soft is false, smc_conn_kill() calls tasklet_unlock_wait(). This
spins if the tasklet is actively running, but it does not remove the tasklet
from the queue or clear the SCHED bit if it is scheduled but not yet running.
Immediately after returning, __smc_lgr_terminate() drops the socket reference.
If the refcount drops to zero, the socket memory (including the embedded
tasklet_struct) is freed while still linked into the per-CPU tasklet list,
leading to a use-after-free or list corruption when the softirq thread
traverses the list.
[Severity: High]
This is also a pre-existing issue, but is there a missing memory barrier
preventing a Store-Load reordering race here?
Earlier in smc_conn_kill(), conn->killed is set to 1:
smc_core.c:smc_conn_kill() {
...
conn->killed = 1;
...
smc_ism_unset_conn(conn);
if (soft)
...
else
tasklet_unlock_wait(&conn->rx_tsklet);
}
Without a full memory barrier (like smp_mb()), the CPU can reorder the load of
the tasklet RUN bit in tasklet_unlock_wait() before the store to conn->killed
becomes visible.
CPU A might load RUN == 0 and proceed to detach and free the buffer.
Concurrently, CPU B (softirq) executes test_and_set_bit(RUN) (a full barrier)
and reads conn->killed. Due to the store buffer delay on CPU A, CPU B might
read killed == 0 and proceed to access the freed buffer.
[Severity: High]
This isn't a bug introduced by this patch, but does the bitfield layout in
struct smc_connection cause silent data loss due to read-modify-write races?
The connection structure defines these overlapping bitfields in the same byte:
smc.h:struct smc_connection {
...
u8 killed : 1;
u8 freed : 1;
u8 out_of_sync : 1;
...
}
smc_cdc_msg_validate() sets conn->out_of_sync = 1 in softirq context without
holding the socket lock. Concurrently, smc_conn_kill() sets conn->killed = 1
or smc_conn_free() sets conn->freed = 1 from process context.
Since the compiler generates non-atomic read-modify-write operations on the
shared byte, a softirq interrupt can silently overwrite and discard the
process context's updates. If the freed bit update is lost, the connection can
be freed twice, corrupting refcounts. If the killed bit update is lost, the
tasklet will continue to process data on a tearing down connection.
> + if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
> + smcd_buf_detach(conn);
> } else {
> smc_cdc_wait_pend_tx_wr(conn);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-b4-disp-a135b4e5-v1-1-705aaec79986@proton.me?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf
2026-07-27 2:14 ` Bryam Vargas
(?)
(?)
@ 2026-07-29 9:46 ` Dust Li
-1 siblings, 0 replies; 4+ messages in thread
From: Dust Li @ 2026-07-29 9:46 UTC (permalink / raw)
To: hexlabsecurity, Wenjia Zhang, D. Wythe, Sidraya Jayagond
Cc: Mahanta Jambigi, netdev, linux-s390, linux-kernel, Tony Lu,
Paolo Abeni, Wen Gu, linux-rdma
On 2026-07-26 21:14:04, Bryam Vargas via B4 Relay wrote:
>From: Bryam Vargas <hexlabsecurity@proton.me>
>
>On the SMC-D DMB-nocopy path smcd_buf_detach() frees conn->sndbuf_desc
>while the receive tasklet can still be running: both teardown sites free
>first and drain afterwards. A tasklet already past the conn->killed check
>then dereferences the freed buffer in smc_cdc_msg_recv_action(), which
>reaches the same field again through smc_tx_prepared_sends().
>
>Drain the tasklet before the detach at both sites, mirroring rmb_desc,
>which smc_buf_unuse() only releases after the drain.
>
>Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
>Cc: stable@kernel.org
>Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
>---
>Found with KASAN on an SMC-D loopback rig (CONFIG_SMC_LO, x86_64).
>
>The teardown is driven through smc_conn_kill() by arming abort_work from a debug
>module parameter: on SMC-D nothing in the receive path queues that work, while on
>SMC-R smc_cdc_msg_validate() queues it for an out-of-range peer seqno. Everything
>after the abort is the unmodified path. A second parameter holds the receive tasklet
>just past the conn->killed check, so the free lands mid-run.
>
> arm ordering abort armed result
> vulnerable detach then drain yes KASAN slab-use-after-free
> control detach then drain no clean
> patched drain then detach yes clean
>
>Same module, same boot, same trigger, same delay; only the ordering differs.
>
> BUG: KASAN: slab-use-after-free in smc_cdc_msg_recv_action+0x1ed9/0x1f20 [smc]
> Read of size 4 at addr ffff88810f6c9720 by task kworker/3:1/147
> Workqueue: smc_close_wq smc_conn_abort_work [smc]
> <IRQ>
> smc_cdc_msg_recv_action+0x1ed9/0x1f20 [smc]
> smcd_cdc_rx_tsklet+0x1f2/0x2f0 [smc]
> tasklet_action_common+0x2fd/0x850
> handle_softirqs+0x18c/0x4f0
> do_softirq+0x3a/0x60
> </IRQ>
> <TASK>
> __local_bh_enable_ip+0x5d/0x60
> smc_cdc_get_slot_and_msg_send+0x253/0x680 [smc]
> smc_conn_kill+0xa8/0x4d0 [smc]
> smc_conn_abort_work+0x34/0x90 [smc]
> </TASK>
>
> Allocated by task 70:
> smcd_buf_attach+0xbd/0x2a0 [smc]
> smc_listen_work+0x2499/0x49a0 [smc]
>
> Freed by task 44:
> kfree+0x135/0x390
> smc_conn_kill+0x3ab/0x4d0 [smc]
> smc_conn_abort_work+0x34/0x90 [smc]
>
> The buggy address belongs to the object at ffff88810f6c9700
> which belongs to the cache kmalloc-128 of size 128
> The buggy address is located 32 bytes inside of
> freed 128-byte region [ffff88810f6c9700, ffff88810f6c9780)
>
>The read is conn->sndbuf_desc->len.
>
>A plain close() does not get there: smc_close_active() always moves the socket to
>SMC_PEERCLOSEWAIT1, so the free happens in smc_close_passive_work() after the peer's
>close CDC -- the same event that stops feeding the tasklet. About 18000 such teardowns
>produced nothing. The ordering is wrong either way, and smc_conn_kill() has no such
>coupling.
>
>Logs on request.
>---
> net/smc/smc_core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>index cf6b620fef05..da64e7a6bd73 100644
>--- a/net/smc/smc_core.c
>+++ b/net/smc/smc_core.c
>@@ -1270,9 +1270,9 @@ void smc_conn_free(struct smc_connection *conn)
> if (lgr->is_smcd) {
> if (!list_empty(&lgr->list))
> smc_ism_unset_conn(conn);
>+ tasklet_kill(&conn->rx_tsklet);
> if (smc_ism_support_dmb_nocopy(lgr->smcd))
> smcd_buf_detach(conn);
>- tasklet_kill(&conn->rx_tsklet);
> } else {
> smc_cdc_wait_pend_tx_wr(conn);
> if (current_work() != &conn->abort_work)
>@@ -1525,12 +1525,12 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
> smc_sk_wake_ups(smc);
> if (conn->lgr->is_smcd) {
> smc_ism_unset_conn(conn);
>- if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
>- smcd_buf_detach(conn);
> if (soft)
> tasklet_kill(&conn->rx_tsklet);
> else
> tasklet_unlock_wait(&conn->rx_tsklet);
>+ if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
>+ smcd_buf_detach(conn);
Hi Bryam,
I think Sashiko's review makes sense. Your change doesn't seem to fully
eliminate the race here. I'm wondering if this should be solved with
RCU rather than just moving smcd_buf_detach() later?
Best regards,
Dust
> } else {
> smc_cdc_wait_pend_tx_wr(conn);
> }
>
>---
>base-commit: 4235cb24ec1e8e96843f3671ba4da2a6ccca2c7b
>change-id: 20260726-b4-disp-a135b4e5-945574bd0938
>
>Best regards,
>--
>Bryam Vargas <hexlabsecurity@proton.me>
>
^ permalink raw reply [flat|nested] 4+ messages in thread