Re: [PATCH net] net/smc: avoid recursive sk_callback_lock in listen data_ready

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error path in smc_listen() fails to restore icsk_af_ops, which leads to infinite recursion if listen() is retried.
--

commit e36405ff19adb1bf696672be6024d5801fec608d
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

net/smc: avoid recursive sk_callback_lock in listen data_ready

smc_listen() installs smc_clcsock_data_ready() as the TCP listen
socket's sk_data_ready callback. This patch adds a state check
to smc_clcsock_data_ready() to avoid acquiring sk_callback_lock
recursively when the TCP socket is leaving TCP_LISTEN.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index b5db69073e20f..4c638765611ce 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -2630,6 +2630,9 @@ static void smc_clcsock_data_ready(struct sock *listen_clcsock)
>  {
>  	struct smc_sock *lsmc;
>  
> +	if (READ_ONCE(listen_clcsock->sk_state) != TCP_LISTEN)
> +		return;
> +
>  	read_lock_bh(&listen_clcsock->sk_callback_lock);
>  	lsmc = smc_clcsock_user_data(listen_clcsock);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the changes
around the listen callback, I noticed a potential infinite recursion issue
in the error path of smc_listen().

When kernel_listen() fails (e.g., due to port exhaustion), the error path
rolls back the callback hooks but omits the restoration of icsk_af_ops:

net/smc/af_smc.c:smc_listen() {
    ...
	/* save original ops */
	smc->ori_af_ops = inet_csk(smc->clcsock->sk)->icsk_af_ops;

	smc->af_ops = *smc->ori_af_ops;
	smc->af_ops.syn_recv_sock = smc_tcp_syn_recv_sock;

	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;

	if (smc->limit_smc_hs)
		tcp_sk(smc->clcsock->sk)->smc_hs_congested = smc_hs_congested;

	rc = kernel_listen(smc->clcsock, backlog);
	if (rc) {
		write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
		smc_clcsock_restore_cb(&smc->clcsock->sk->sk_data_ready,
				       &smc->clcsk_data_ready);
		rcu_assign_sk_user_data(smc->clcsock->sk, NULL);
		write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
		goto out;
	}
    ...
}

Does this leave the internal socket with a corrupted state? 

If userspace retries the listen() call on the same socket after a failure,
smc_listen() reads the currently active icsk_af_ops into smc->ori_af_ops.
Since the previous cleanup was missed, wouldn't smc->ori_af_ops become a
pointer to its own wrapper structure (&smc->af_ops)?

If the socket eventually successfully listens and accepts a connection,
when smc_tcp_syn_recv_sock() is invoked, it calls:

smc->ori_af_ops->syn_recv_sock(...)

Could this result in smc_tcp_syn_recv_sock() calling itself infinitely and
causing a stack overflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617152855.1039151-1-runyu.xiao@seu.edu.cn?part=1