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@xxxxxxxxx>
>
>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@xxxxxxxxxx
>Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
>---
>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@xxxxxxxxx>
>