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

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

[Yingjie Wang]

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.

Both request and full TCP sockets currently free saved_syn with plain
kfree().  Embed an rcu_head in struct saved_syn and use kfree_rcu() for
both teardown paths.  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 this change.  A patched kernel without the delay
instrumentation also completed a successful migration and seven saved-SYN
reads without a KASAN report.

Full allmodconfig and allyesconfig builds completed with W=1.  Existing
warnings outside the modified files were made non-fatal with
KCFLAGS=-Wno-error; the modified files emitted none.

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@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 include/linux/tcp.h             | 2 +-
 include/net/request_sock.h      | 2 ++
 net/ipv4/inet_connection_sock.c | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index d18f7bc1e5d0..dd3b097daac9 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -624,7 +624,7 @@ static inline void tcp_move_syn(struct tcp_sock *tp,
 
 static inline void tcp_saved_syn_free(struct tcp_sock *tp)
 {
-	kfree(tp->saved_syn);
+	kfree_rcu(tp->saved_syn, rcu);
 	tp->saved_syn = NULL;
 }
 
diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 5a9c826a7092..b6ccfc31147d 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;
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 93b306d91b6d..a2622eccaff1 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -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(req->saved_syn, rcu);
 	kmem_cache_free(req->rsk_ops->slab, req);
 }
 

base-commit: 902407bc588575af0ceae5de4f635fe1e024f258
-- 
2.43.0

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

[Kuniyuki Iwashima]

On Mon, 02 Nov 2026 10:55:58 +0000, Yingjie Wang wrote:
> 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.
> 
> Both request and full TCP sockets currently free saved_syn with plain
> kfree().  Embed an rcu_head in struct saved_syn and use kfree_rcu() for
> both teardown paths.

Please add rcu_dereference() in bpf_sock_ops_get_syn()
and RCU_INIT_POINTER() on the writer side.



> 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 this change.  A patched kernel without the delay
> instrumentation also completed a successful migration and seven saved-SYN
> reads without a KASAN report.
> 
> Full allmodconfig and allyesconfig builds completed with W=1.  Existing
> warnings outside the modified files were made non-fatal with
> KCFLAGS=-Wno-error; the modified files emitted none.

I'd drop this paragraph or move after --- since it does not
add any value after review.

---
pw-bot: cr

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

[Yingjie Wang]

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@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 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;
 };
 
 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);
+	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();
+			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);
 	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);
 		}
 	}
 }

base-commit: c3e67bd82b9b9b1a5118f09a429ccefe2aab0741
-- 
2.43.0

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@outlook.com> 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@vger.kernel.org
> Signed-off-by: Yingjie Wang <x-professor@outlook.com>
> ---
>  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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.