Re: [PATCH net v2] tcp: free saved_syn after an RCU grace period

[Eric Dumazet]

Le mar. 3 nov. 2026 à 12:19, Yingjie Wang <x-professor@xxxxxxxxxxx> a écrit :
>
> tcp_reqsk_record_syn() allocates saved_syn separately from its request
> socket. bpf_sock_ops_get_syn() can load that pointer while processing a
> retransmitted SYN under RCU.
>
> There are two ownership transfers which can let another CPU reclaim the
> allocation while such a reader is still using it.
>
> First, tcp_move_syn() transfers req->saved_syn to a newly created child
> socket. The child can be closed and tcp_saved_syn_free() can reclaim the
> allocation before a reader of the old request finishes.
>
> Second, inet_reqsk_clone() shallow-copies request_sock during request
> migration. The migrated clone becomes the owner after reqsk_migrate_reset()
> clears the old request's pointer, but a reader which already loaded the
> pointer from the old request can still be using it when the clone is
> released.
>
> Mark the request and full-socket saved_syn pointers as RCU protected. Use
> rcu_dereference() in the BPF readers, rcu_assign_pointer() when publishing
> the saved SYN to the child socket, and RCU_INIT_POINTER() at initialization
> and clearing sites. Embed an rcu_head in struct saved_syn and defer
> reclamation in both release paths. Clear a live full-socket pointer before
> scheduling reclamation, so a new RCU reader cannot acquire it after the
> grace period starts.
>
> This follows commit e3f33b0a1d89 ("ipv4: free inet_opt and ireq_opt
> after an RCU grace period"), which fixed the same ownership-transfer
> pattern for IPv4 options.
>
> The issue was confirmed with KASAN on v7.3-16007-g36aad21d50d1 using
> temporary kernel instrumentation to widen the reader and request-migration
> window. KASAN reported a slab-use-after-free read in
> bpf_sock_ops_get_syn(), with the allocation freed by __reqsk_free() from
> reqsk_timer_handler(). The same explicit migration test completed without
> a report with deferred reclamation. A patched kernel without the delay
> instrumentation also completed a successful migration and seven saved-SYN
> reads without a KASAN report.
>
> Fixes: 0813a841566f ("bpf: tcp: Allow bpf prog to write and parse TCP header option")
> Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Yingjie Wang <x-professor@xxxxxxxxxxx>
> ---
>  include/linux/tcp.h             | 16 +++++++++++-----
>  include/net/request_sock.h      |  4 +++-
>  net/core/filter.c               | 16 +++++++++++-----
>  net/ipv4/inet_connection_sock.c |  6 +++---
>  net/ipv4/tcp.c                  | 14 +++++++++-----
>  net/ipv4/tcp_input.c            |  2 +-
>  6 files changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index d18f7bc1e5d0..5ba4d81e431a 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -536,7 +536,7 @@ struct tcp_sock {
>          * socket. Used to retransmit SYNACKs etc.
>          */
>         struct request_sock __rcu *fastopen_rsk;
> -       struct saved_syn *saved_syn;
> +       struct saved_syn __rcu *saved_syn;

Full sockets do not need RCU protection for tp->saved_syn : all the
accesses (do_tcp_getsockopt(), sol_tcp_sockopt(), bpf_sock_ops_get_syn()
on a full socket, tcp_saved_syn_free()) are done with the socket lock
held. Only the lifetime of the object matters, because a request socket
reader might still use it.

I would only add __rcu to req->saved_syn, and keep tp->saved_syn
a plain pointer. This would avoid the net/ipv4/tcp.c changes.


>  };
>
>  enum tsq_enum {
> @@ -618,14 +618,20 @@ static inline void fastopen_queue_tune(struct sock *sk, int backlog)
>  static inline void tcp_move_syn(struct tcp_sock *tp,
>                                 struct request_sock *req)
>  {
> -       tp->saved_syn = req->saved_syn;
> -       req->saved_syn = NULL;
> +       struct saved_syn *saved_syn;
> +
> +       saved_syn = rcu_dereference(req->saved_syn);
> +       rcu_assign_pointer(tp->saved_syn, saved_syn);

The content was already published via the request socket, and the
child is still bh-locked at this point : rcu_assign_pointer() is not
needed.

       RCU_INIT_PONTER(tp->saved_syn, unrcu_pointer(req->saved_syn));

> +       RCU_INIT_POINTER(req->saved_syn, NULL);
>  }
>
>  static inline void tcp_saved_syn_free(struct tcp_sock *tp)
>  {
> -       kfree(tp->saved_syn);
> -       tp->saved_syn = NULL;
> +       struct saved_syn *saved_syn;
> +
> +       saved_syn = rcu_dereference_protected(tp->saved_syn, 1);
> +       RCU_INIT_POINTER(tp->saved_syn, NULL);
> +       kfree_rcu(saved_syn, rcu);
>  }
>
>  static inline u32 tcp_saved_syn_len(const struct saved_syn *saved_syn)
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 5a9c826a7092..ddc134f1d15d 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -16,6 +16,7 @@
>  #include <linux/types.h>
>  #include <linux/bug.h>
>  #include <linux/refcount.h>
> +#include <linux/rcupdate.h>
>
>  #include <net/sock.h>
>  #include <net/rstreason.h>
> @@ -39,6 +40,7 @@ struct request_sock_ops {
>  };
>
>  struct saved_syn {
> +       struct rcu_head rcu;
>         u32 mac_hdrlen;
>         u32 network_hdrlen;
>         u32 tcp_hdrlen;
> @@ -68,7 +70,7 @@ struct request_sock {
>         struct timer_list               rsk_timer;
>         const struct request_sock_ops   *rsk_ops;
>         struct sock                     *sk;
> -       struct saved_syn                *saved_syn;
> +       struct saved_syn __rcu          *saved_syn;
>         u32                             secid;
>         u32                             peer_secid;
>         u32                             timeout;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 09c60a58c82a..38a4b48e1fb0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5714,12 +5714,18 @@ static int sol_tcp_sockopt(struct sock *sk, int optname,
>
>         if (getopt) {
>                 if (optname == TCP_SAVED_SYN) {
> +                       struct saved_syn *saved_syn;
>                         struct tcp_sock *tp = tcp_sk(sk);
>
> -                       if (!tp->saved_syn ||
> -                           *optlen > tcp_saved_syn_len(tp->saved_syn))
> +                       rcu_read_lock();

This rcu_read_lock() seems redundant. I would use
rcu_dereference_protected(tp->saved_syn, lockdep_sock_is_held(sk))

> +                       saved_syn = rcu_dereference(tp->saved_syn);
> +                       if (!saved_syn ||
> +                           *optlen > tcp_saved_syn_len(saved_syn)) {
> +                               rcu_read_unlock();
>                                 return -EINVAL;
> -                       memcpy(optval, tp->saved_syn->data, *optlen);
> +                       }
> +                       memcpy(optval, saved_syn->data, *optlen);
> +                       rcu_read_unlock();
>                         /* It cannot free tp->saved_syn here because it
>                          * does not know if the user space still needs it.
>                          */
> @@ -6125,9 +6131,9 @@ static int bpf_sock_ops_get_syn(struct bpf_sock_ops_kern *bpf_sock,
>                          * not be available.  It has to resort to
>                          * saved_syn (if it is saved).
>                          */
> -                       saved_syn = inet_reqsk(sk)->saved_syn;
> +                       saved_syn = rcu_dereference(inet_reqsk(sk)->saved_syn);
>                 else
> -                       saved_syn = tcp_sk(sk)->saved_syn;
> +                       saved_syn = rcu_dereference(tcp_sk(sk)->saved_syn);
>
>                 if (!saved_syn)
>                         return -ENOENT;
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 93b306d91b6d..1d54057a86ef 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -869,7 +869,7 @@ reqsk_alloc_noprof(const struct request_sock_ops *ops, struct sock *sk_listener,
>         req_to_sk(req)->sk_prot = sk_listener->sk_prot;
>         sk_node_init(&req_to_sk(req)->sk_node);
>         sk_tx_queue_clear(req_to_sk(req));
> -       req->saved_syn = NULL;
> +       RCU_INIT_POINTER(req->saved_syn, NULL);
>         req->syncookie = 0;
>         req->num_timeout = 0;
>         req->num_retrans = 0;
> @@ -908,7 +908,7 @@ void __reqsk_free(struct request_sock *req)
>         req->rsk_ops->destructor(req);
>         if (req->rsk_listener)
>                 sock_put(req->rsk_listener);
> -       kfree(req->saved_syn);
> +       kfree_rcu(rcu_dereference_protected(req->saved_syn, 1), rcu);

Or kfree_rcu(unrcu_pointer(req->saved_syn), rcu)

>         kmem_cache_free(req->rsk_ops->slab, req);
>  }
>
> @@ -979,7 +979,7 @@ static void reqsk_queue_migrated(struct request_sock_queue *queue,
>
>  static void reqsk_migrate_reset(struct request_sock *req)
>  {
> -       req->saved_syn = NULL;
> +       RCU_INIT_POINTER(req->saved_syn, NULL);
>  #if IS_ENABLED(CONFIG_IPV6)
>         inet_rsk(req)->ipv6_opt = NULL;
>         inet_rsk(req)->pktopts = NULL;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index b9de7c302ba8..7e8f7c9d5f53 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4722,13 +4722,17 @@ int do_tcp_getsockopt(struct sock *sk, int level,
>                 val = tp->save_syn;
>                 break;
>         case TCP_SAVED_SYN: {
> +               struct saved_syn *saved_syn;
> +
>                 if (copy_from_sockptr(&len, optlen, sizeof(int)))
>                         return -EFAULT;
>
>                 sockopt_lock_sock(sk);
> -               if (tp->saved_syn) {
> -                       if (len < tcp_saved_syn_len(tp->saved_syn)) {
> -                               len = tcp_saved_syn_len(tp->saved_syn);
> +               saved_syn = rcu_dereference_protected(tp->saved_syn,
> +                                                     lockdep_sock_is_held(sk));
> +               if (saved_syn) {
> +                       if (len < tcp_saved_syn_len(saved_syn)) {
> +                               len = tcp_saved_syn_len(saved_syn);
>                                 if (copy_to_sockptr(optlen, &len, sizeof(int))) {
>                                         sockopt_release_sock(sk);
>                                         return -EFAULT;
> @@ -4736,12 +4740,12 @@ int do_tcp_getsockopt(struct sock *sk, int level,
>                                 sockopt_release_sock(sk);
>                                 return -EINVAL;
>                         }
> -                       len = tcp_saved_syn_len(tp->saved_syn);
> +                       len = tcp_saved_syn_len(saved_syn);
>                         if (copy_to_sockptr(optlen, &len, sizeof(int))) {
>                                 sockopt_release_sock(sk);
>                                 return -EFAULT;
>                         }
> -                       if (copy_to_sockptr(optval, tp->saved_syn->data, len)) {
> +                       if (copy_to_sockptr(optval, saved_syn->data, len)) {
>                                 sockopt_release_sock(sk);
>                                 return -EFAULT;
>                         }
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index ebd2c12e42d5..5151bb16a8b9 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -7638,7 +7638,7 @@ static void tcp_reqsk_record_syn(const struct sock *sk,
>                         saved_syn->network_hdrlen = skb_network_header_len(skb);
>                         saved_syn->tcp_hdrlen = tcp_hdrlen(skb);
>                         memcpy(saved_syn->data, base, len);
> -                       req->saved_syn = saved_syn;
> +                       RCU_INIT_POINTER(req->saved_syn, saved_syn);
>                 }
>         }
>  }
>

Also, for the next version:

- Please start a new thread, do not reply to the previous version.
- Add a changelog after the "---" line, with a link to the previous
  version on lore.
- Please answer the question asked by netdev-bot : how was this issue
  discovered ? The changelog says how it was confirmed, not how it was
  found.

Thanks.

pw-bot: cr