netdev - Re: [PATCH net] tcp: drop skb extensions before skb_attempt_defer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z6yWOCAeKqaj2BfP@hog>
Date: Wed, 12 Feb 2025 13:38:16 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
	Neal Cardwell <ncardwell@...gle.com>,
	Kuniyuki Iwashima <kuniyu@...zon.com>,
	David Ahern <dsahern@...nel.org>, Xiumei Mu <xmu@...hat.com>,
	Paul Moore <paul@...l-moore.com>
Subject: Re: [PATCH net] tcp: drop skb extensions before
 skb_attempt_defer_free

2025-02-11, 20:17:17 +0100, Eric Dumazet wrote:
> On Tue, Feb 11, 2025 at 7:51 PM Sabrina Dubroca <sd@...asysnail.net> wrote:
> > An additional patch could maybe add DEBUG_NET_WARN_ON_ONCE at the time
> > we add skbs to sk_receive_queue, to check we didn't miss (or remove in
> > the future) places where the dst or secpath should have been dropped?
> 
> Sure, adding the  DEBUG_NET_WARN_ON_ONCE() is absolutely fine.

Something like this would be ok?
(on top of the previous diff)

The main drawback is that we can't just look for "sk_receive_queue" in
net/ipv4/tcp*.

-------- 8< --------
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4d106d13db22..930cda5b5eb9 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -690,6 +690,13 @@ static inline void tcp_cleanup_skb(struct sk_buff *skb)
 	secpath_reset(skb);
 }
 
+static inline void tcp_add_receive_queue(struct sock *sk, struct sk_buff *skb)
+{
+	DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
+	DEBUG_NET_WARN_ON_ONCE(secpath_exists(skb));
+	__skb_queue_tail(&sk->sk_receive_queue, skb);
+}
+
 /* tcp_timer.c */
 void tcp_init_xmit_timers(struct sock *);
 static inline void tcp_clear_xmit_timers(struct sock *sk)
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index b815b9fc604c..32b28fc21b63 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -195,7 +195,7 @@ void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
 	TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_SYN;
 
 	tp->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
-	__skb_queue_tail(&sk->sk_receive_queue, skb);
+	tcp_add_receive_queue(sk, skb);
 	tp->syn_data_acked = 1;
 
 	/* u64_stats_update_begin(&tp->syncp) not needed here,
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bb0811c38908..6821e5540a53 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4970,7 +4970,7 @@ static void tcp_ofo_queue(struct sock *sk)
 		tcp_rcv_nxt_update(tp, TCP_SKB_CB(skb)->end_seq);
 		fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN;
 		if (!eaten)
-			__skb_queue_tail(&sk->sk_receive_queue, skb);
+			tcp_add_receive_queue(sk, skb);
 		else
 			kfree_skb_partial(skb, fragstolen);
 
@@ -5162,7 +5162,7 @@ static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb,
 				  skb, fragstolen)) ? 1 : 0;
 	tcp_rcv_nxt_update(tcp_sk(sk), TCP_SKB_CB(skb)->end_seq);
 	if (!eaten) {
-		__skb_queue_tail(&sk->sk_receive_queue, skb);
+		tcp_add_receive_queue(sk, skb);
 		skb_set_owner_r(skb, sk);
 	}
 	return eaten;