[PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf

[PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf

[Bryam Vargas via B4 Relay]

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);
 	} 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>

Re: [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf

[Dust Li]

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>
>

Re: [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf

[Bryam Vargas]

Dust,

> 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?

You're right, it doesn't. I built both shapes and measured them on an SMC-D
loopback rig. "path" is connections reaching either teardown site with the link
group already unlinked, "armable" how many of those still had both gates in
smcd_handle_irq() open when the drain returned, "re-armed" the device arming
the tasklet again afterwards:

  form                                   path  armable  re-armed
  upstream                                169       73        29
  v1 (drain, then detach)                 172       78        33
  unregister first, then drain             31        0         0
  v1 + RCU                                 24        9         3

Two things about that table before you read anything into it. The last two arms
ran much shorter than the first two, so the ratios are the comparable part, not
the absolute path counts. And the third arm also forced tasklet_kill() in the
!soft path; that was inert here because smc_lgr_terminate_work() passes
soft=true, but it wasn't only the unregister.

The reorder leaves the window open because smc_conn_free() calls
smc_ism_unset_conn() only while the link group is still on its device list,
and smc_lgr_terminate_sched() unlinks it before the worker kills the
connections. It never sets conn->killed either, so both gates stay open across
tasklet_kill().

RCU does not close it. smcd_buf_detach() frees the descriptor and clears the
field, and RCU defers only the free -- the re-armed tasklet still runs and
still finds conn->sndbuf_desc NULL. RCU on the descriptor would still be a
reasonable thing to want for the free itself; it just isn't what shuts this
window, so I didn't fold it in. Say the word if you want it anyway.

v2 unregisters unconditionally before the drain, and clears conn->sndbuf_desc
before freeing it so a reader that samples the pointer can't get a freed one.
It carries a second patch for a dereference the same teardown reaches:
smc_close_stream_wait() calls smc_tx_prepared_sends() from inside
sk_wait_event(), which evaluates its condition once with the socket lock
released. SIOCOUTQ reads the field by hand, so that one is bounded too.

Thanks for catching it.

Bryam