netdev - Re: [PATCH v3 bpf-next 10/11] bpf: tcp: Support arbitrary SYN Cookie.

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: <20231123003154.56710-1-kuniyu@amazon.com>
Date: Wed, 22 Nov 2023 16:31:54 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <martin.lau@...ux.dev>
CC: <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
	<daniel@...earbox.net>, <davem@...emloft.net>, <dsahern@...nel.org>,
	<edumazet@...gle.com>, <haoluo@...gle.com>, <john.fastabend@...il.com>,
	<jolsa@...nel.org>, <kpsingh@...nel.org>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <mykolal@...com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <sdf@...gle.com>,
	<song@...nel.org>, <yonghong.song@...ux.dev>
Subject: Re: [PATCH v3 bpf-next 10/11] bpf: tcp: Support arbitrary SYN Cookie.

From: Martin KaFai Lau <martin.lau@...ux.dev>
Date: Wed, 22 Nov 2023 15:19:29 -0800
> On 11/21/23 10:42 AM, Kuniyuki Iwashima wrote:
> > diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
> > index 533a7337865a..9a67f47a5e64 100644
> > --- a/include/net/inet6_hashtables.h
> > +++ b/include/net/inet6_hashtables.h
> > @@ -116,9 +116,23 @@ struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> >   	if (!sk)
> >   		return NULL;
> >   
> > -	if (!prefetched || !sk_fullsock(sk))
> > +	if (!prefetched)
> >   		return sk;
> >   
> > +	if (sk->sk_state == TCP_NEW_SYN_RECV) {
> > +#if IS_ENABLED(CONFIG_SYN_COOKIE)
> > +		if (inet_reqsk(sk)->syncookie) {
> > +			*refcounted = false;
> > +			skb->sk = sk;
> > +			skb->destructor = sock_pfree;
> 
> Instead of re-init the skb->sk and skb->destructor, can skb_steal_sock() avoid 
> resetting them to NULL in the first place and skb_steal_sock() returns the 
> rsk_listener instead?

Yes, but we need to move skb_steal_sock() to request_sock.h or include it just
before skb_steal_sock() in sock.h like below.  When I include request_sock.h in
top of sock.h, there were many build errors.


> btw, can inet_reqsk(sk)->rsk_listener be set to NULL after 
> this point?

except for sock_pfree(), we will not set NULL until cookie_bpf_check().


> Beside, it is essentially assigning the incoming request to a listening sk. Does 
> it need to call the inet6_lookup_reuseport() a few lines below to avoid skipping 
> the bpf reuseport selection that was fixed in commit 9c02bec95954 ("bpf, net: 
> Support SO_REUSEPORT sockets with bpf_sk_assign")?

Ah, good point.  I assumed bpf_sk_lookup() will do the random pick, but we
need to call it just in case sk is picked from bpf map.

As you suggested, if we return rsk_listener from skb_steal_sock(), we can
reuse the reuseport_lookup call in inet6_steal_sock().

Thanks!


---8<---
diff --git a/include/net/sock.h b/include/net/sock.h
index 1d6931caf0c3..83efbe0e7c3b 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2838,6 +2838,8 @@ sk_is_refcounted(struct sock *sk)
        return !sk_fullsock(sk) || !sock_flag(sk, SOCK_RCU_FREE);
 }
 
+#include <net/request_sock.h>
+
 /**
  * skb_steal_sock - steal a socket from an sk_buff
  * @skb: sk_buff to steal the socket from
@@ -2847,20 +2849,38 @@ sk_is_refcounted(struct sock *sk)
 static inline struct sock *
 skb_steal_sock(struct sk_buff *skb, bool *refcounted, bool *prefetched)
 {
-       if (skb->sk) {
-               struct sock *sk = skb->sk;
+       struct sock *sk = skb->sk;
 
+       if (!sk) {
+               *prefetched = false;
+               *refcounted = false;
+               return NULL;
+       }
+
+       *prefetched = skb_sk_is_prefetched(skb);
+       if (!*prefetched) {
                *refcounted = true;
-               *prefetched = skb_sk_is_prefetched(skb);
-               if (*prefetched)
-                       *refcounted = sk_is_refcounted(sk);
-               skb->destructor = NULL;
-               skb->sk = NULL;
-               return sk;
+               goto out;
        }
-       *prefetched = false;
-       *refcounted = false;
-       return NULL;
+
+       switch (sk->sk_state) {
+       case TCP_NEW_SYN_RECV:
+               if (inet_reqsk(sk)->syncookie) {
+                       *refcounted = false;
+                       return inet_reqsk(sk)->rsk_listener;
+               }
+               fallthrough;
+       case TCP_TIME_WAIT:
+               *refcounted = true;
+               break;
+       default:
+               *refcounted = !sock_flag(sk, SOCK_RCU_FREE);
+       }
+
+out:
+       skb->destructor = NULL;
+       skb->sk = NULL;
+       return sk;
 }
 
 /* Checks if this SKB belongs to an HW offloaded socket
---8<---