Re: [PATCH] net/smc: reject CDC messages after connection teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] net/smc: bh_lock_sock() in smc_cdc_msg_recv() does not exclude smc_conn_free()
--

commit 64de84c513a98ece4d44144df98874a465343ddf
Author: Weiming Shi <bestswngs@gmail.com>

net/smc: reject CDC messages after connection teardown

This commit checks smc->conn.freed in smc_cdc_msg_recv() after acquiring
bh_lock_sock(&smc->sk) to avoid processing CDC messages and dereferencing
released receive buffer descriptors after connection teardown.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..499a2473b79b4 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -428,7 +428,8 @@ static void smc_cdc_msg_recv(struct smc_sock *smc, struct smc_cdc_msg *cdc)
>  {
>  	sock_hold(&smc->sk);
>  	bh_lock_sock(&smc->sk);
> -	smc_cdc_msg_recv_action(smc, cdc);
> +	if (!smc->conn.freed)
> +		smc_cdc_msg_recv_action(smc, cdc);
>  	bh_unlock_sock(&smc->sk);
>  	sock_put(&smc->sk); /* no free sk in softirq-context */
>  }

[Severity: Critical]
Does checking smc->conn.freed under bh_lock_sock(&smc->sk) actually serialize
smc_cdc_msg_recv() against smc_conn_free()?

Process-context callers of smc_conn_free(), such as smc_release() ->
__smc_release() -> smc_conn_free(), hold the sleeping socket lock via
lock_sock(&smc->sk). In net/core/sock.c:lock_sock_nested(), acquiring the
user-context socket lock sets sk->sk_lock.owned = 1 and immediately drops
sk->sk_lock.slock so the caller can sleep:

net/core/sock.c:lock_sock_nested() {
	spin_lock_bh(&sk->sk_lock.slock);
	if (unlikely(sock_owned_by_user_nocheck(sk)))
		__lock_sock(sk);
	sk->sk_lock.owned = 1;
	spin_unlock_bh(&sk->sk_lock.slock);
}

Meanwhile, smc_cdc_msg_recv() runs in tasklet context via
smc_wr_rx_tasklet_fn() -> smc_cdc_rx_handler() -> smc_cdc_msg_recv() and
calls bh_lock_sock(&smc->sk), which only acquires sk->sk_lock.slock without
waiting on sk->sk_lock.owned. In fact, smc_cdc_msg_recv_action() in
net/smc/smc_cdc.c explicitly checks sock_owned_by_user(&smc->sk) because it
can run while a process holds lock_sock(&smc->sk):

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
	...
		if (!sock_owned_by_user(&smc->sk))
			smc_tx_pending(conn);
		else
			conn->tx_in_release_sock = true;
	...
}

In net/smc/smc_core.c:smc_conn_free(), conn->freed = 1 and smc_buf_unuse()
run without holding bh_lock_sock(&smc->sk) and before
smc_lgr_unregister_conn(conn):

net/smc/smc_core.c:smc_conn_free() {
	...
	conn->freed = 1;
	...
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
	...
}

If CPU 0 enters smc_cdc_msg_recv(), acquires bh_lock_sock(&smc->sk), and sees
!smc->conn.freed just before CPU 1 in smc_conn_free() sets conn->freed = 1
and frees conn->rmb_desc or conn->sndbuf_desc via smc_buf_unuse(), can CPU 0
still concurrently dereference the freed conn->rmb_desc->len in
smc_cdc_msg_recv_action():

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
	...
	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
				  &conn->local_rx_ctrl.prod);
	...
}

Additionally, if smc_cdc_msg_recv_action() observes
sock_owned_by_user(&smc->sk) while __smc_release() holds lock_sock(&smc->sk),
won't setting conn->tx_in_release_sock = true cause the subsequent
release_sock(&smc->sk) in smc_release() to invoke smc_release_cb() ->
smc_tx_pending() on a connection that smc_conn_free() has already torn down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021171256.166870-2-bestswngs@gmail.com?part=1