Re: [PATCH net v2] net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()

[D. Wythe]

On Mon, Mar 09, 2026 at 07:48:54AM +0000, Jiayuan Chen wrote:
> March 9, 2026 at 14:06, "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx mailto:alibuda@xxxxxxxxxxxxxxxxx?to=%22D.%20Wythe%22%20%3Calibuda%40linux.alibaba.com%3E > wrote:
> 
> 
> [...]
> > >  Reproducer was verified with mdelay injection and smc_run,
> > >  the issue no longer occurs with this patch applied.
> > >  
> > >  [1] https://syzkaller.appspot.com/bug?extid=827ae2bfb3a3529333e9
> > >  
> > >  Fixes: 8270d9c21041 ("net/smc: Limit backlog connections")
> > >  Reported-by: syzbot+827ae2bfb3a3529333e9@xxxxxxxxxxxxxxxxxxxxxxxxx
> > >  Closes: https://lore.kernel.org/all/67eaf9b8.050a0220.3c3d88.004a.GAE@xxxxxxxxxx/T/
> > >  Suggested-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> > >  Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> > >  ---
> > >  v2:
> > >  - Use rcu_read_lock() + refcount_inc_not_zero() instead of
> > >  read_lock_bh(sk_callback_lock) + sock_hold(), since this
> > >  is the TCP handshake hot path and read_lock_bh is too
> > >  expensive under SYN flood.
> > >  - Set SOCK_RCU_FREE on SMC listen socket to ensure
> > >  RCU-deferred freeing.
> > >  
> > >  v1: https://lore.kernel.org/netdev/20260307032158.372165-1-jiayuan.chen@xxxxxxxxx/
> > >  ---
> > >  net/smc/af_smc.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >  
> > >  diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> > >  index d0119afcc6a1..72ac1d8c62d4 100644
> > >  --- a/net/smc/af_smc.c
> > >  +++ b/net/smc/af_smc.c
> > >  @@ -131,7 +131,13 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> > >  struct smc_sock *smc;
> > >  struct sock *child;
> > >  
> > >  + rcu_read_lock();
> > >  smc = smc_clcsock_user_data(sk);
> > >  + if (!smc || !refcount_inc_not_zero(&smc->sk.sk_refcnt)) {
> > >  + rcu_read_unlock();
> > >  + return NULL;
> > >  + }
> > >  + rcu_read_unlock();
> > >  
> > >  if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
> > >  sk->sk_max_ack_backlog)
> > >  @@ -153,11 +159,13 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> > >  if (inet_csk(child)->icsk_af_ops == inet_csk(sk)->icsk_af_ops)
> > >  inet_csk(child)->icsk_af_ops = smc->ori_af_ops;
> > >  }
> > >  + sock_put(&smc->sk);
> > >  return child;
> > >  
> > >  drop:
> > >  dst_release(dst);
> > >  tcp_listendrop(sk);
> > >  + sock_put(&smc->sk);
> > >  return NULL;
> > >  }
> > >  
> > >  @@ -2691,6 +2699,7 @@ int smc_listen(struct socket *sock, int backlog)
> > >  write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
> > >  goto out;
> > >  }
> > >  + sock_set_flag(sk, SOCK_RCU_FREE);
> > > 
> > This RCU approach looks good to me. Since SOCK_RCU_FREE is now enabled,
> > other callers of smc_clcsock_user_data() should also follow this
> > RCU-based pattern. It will eventually allow us to completely remove the
> > annoying sk_callback_lock.
> > 
> > D. Wythe
> > 
> 
> 
> Hi D. Wythe,
> 
> Thanks for the suggestion. I agree that converting all smc_clcsock_user_data()
> callers to RCU is a reasonable direction, and it would allow us to eventually
> remove the sk_callback_lock dependency for sk_user_data access.
> 
> However, I'd prefer to keep this patch focused on fixing the specific bug in
> smc_tcp_syn_recv_sock(), since it needs to be backported to stable trees.
> Mixing a bug fix with broader refactoring makes backporting harder and increases
> the risk of regressions.
> 
> Also, converting the other callers is not entirely trivial. For example:
> 
> - smc_fback_state_change/data_ready/write_space/error_report():
>   the sk_callback_lock there protects not only sk_user_data but also the
>   consistency of the saved callback pointers (e.g.,smc->clcsk_state_change).
>   Switching to RCU requires careful ordering analysis against the write side
>   in smc_fback_restore_callbacks(). Additionally, fallback sockets would also
>   need SOCK_RCU_FREE.
> 
> - smc_clcsock_data_ready():
>   the sock_hold() would need to become refcount_inc_not_zero() to handle the
>   case where the refcount has already reached zero.
> 
> I'd like to address these conversions in a follow-up patch series.
> 
> What do you think?

Thanks for the detailed explanation. I agree with your plan, it's
better to keep the bug fix and the optimization separate.

> 
> > > 
> > > sk->sk_max_ack_backlog = backlog;
> > >  sk->sk_ack_backlog = 0;
> > >  sk->sk_state = SMC_LISTEN;
> > >  -- 
> > >  2.43.0
> > >
> >