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

[sashiko-bot@kernel.org]

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