Re: [PATCH net v2 1/2] net/smc: unregister the connection before draining the rx tasklet

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch attempts to prevent concurrent readers from accessing a freed `sndbuf_desc` by setting the pointer to `NULL` before `kfree()`. However, this is insufficient to protect lockless readers like the Netlink diagnostic dump (`__smc_diag_dump`), leaving a Time-of-Check to Time-of-Use (TOCTOU) NULL pointer dereference or Use-After-Free vulnerability.
--

--- Patch [1]: [PATCH net v2 1/2] net/smc: unregister the connection before draining the rx tasklet ---
commit 776266687d64ab630969455645f01d4387577460
Author: Bryam Vargas <[email protected]>

net/smc: unregister the connection before draining the rx tasklet

This commit modifies the teardown path to unregister the connection and
drain the receive tasklet before detaching the buffers. It also clears
conn->sndbuf_desc before freeing it to prevent readers from getting a
freed pointer.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index b4208cb186c5e..181647982490b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1209,14 +1209,16 @@ static void smcd_buf_detach(struct smc_connection *conn)
>  {
>  	struct smcd_dev *smcd = conn->lgr->smcd;
>  	u64 peer_token = conn->peer_token;
> +	struct smc_buf_desc *buf_desc;
>  
>  	if (!conn->sndbuf_desc)
>  		return;
>  
>  	smc_ism_detach_dmb(smcd, peer_token);
>  
> -	kfree(conn->sndbuf_desc);
> +	buf_desc = conn->sndbuf_desc;
>  	conn->sndbuf_desc = NULL;
> +	kfree(buf_desc);

[Severity: High]
This is a pre-existing issue, but is setting the pointer to NULL before
kfree() sufficient to protect concurrent lockless readers?

Looking at __smc_diag_dump() in net/smc/smc_diag.c, which executes without
holding the socket lock, it evaluates the pointer like this:

    .sndbuf_size = conn->sndbuf_desc ?
                   conn->sndbuf_desc->len : 0,

Since it doesn't use READ_ONCE(), couldn't the compiler generate two fetches?

If the first fetch passes the NULL check but the second fetch gets NULL after
the pointer is cleared here, would it lead to a NULL pointer dereference?

Alternatively, if it only fetches once, couldn't it dereference memory that
was just synchronously freed via kfree(), causing a Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1