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

["Jiayuan Chen" <jiayuan.chen@linux.dev>]

March 9, 2026 at 14:06, "D. Wythe" <alibuda@linux.alibaba.com mailto:alibuda@linux.alibaba.com?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@syzkaller.appspotmail.com
> >  Closes: https://lore.kernel.org/all/67eaf9b8.050a0220.3c3d88.004a.GAE@google.com/T/
> >  Suggested-by: Eric Dumazet <edumazet@google.com>
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> >  ---
> >  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@linux.dev/
> >  ---
> >  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?

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