netdev - Re: [PATCH net-next 1/5] net: add TIME_WAIT logic to sk_to_full

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] [day] [month] [year] [list]

Message-ID: <CANn89iKSchhx-NTbhZYyi5zp4HX=JsYbecK+bh6UbQ_FKjn1qQ@mail.gmail.com>
Date: Sun, 6 Oct 2024 22:14:52 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, eric.dumazet@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/5] net: add TIME_WAIT logic to sk_to_full_sk()

On Sat, Oct 5, 2024 at 2:37 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 10/4/24 2:56 PM, Eric Dumazet wrote:
> > On Fri, Oct 4, 2024 at 9:16 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >>
> >> TCP will soon attach TIME_WAIT sockets to some ACK and RST.
> >>
> >> Make sure sk_to_full_sk() detects this and does not return
> >> a non full socket.
> >>
> >> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> >> ---
> >>   include/net/inet_sock.h | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> >> index 394c3b66065e20d34594d6e2a2010c55bb457810..cec093b78151b9a3b95ad4b3672a72b0aa9a8305 100644
> >> --- a/include/net/inet_sock.h
> >> +++ b/include/net/inet_sock.h
> >> @@ -319,8 +319,10 @@ static inline unsigned long inet_cmsg_flags(const struct inet_sock *inet)
> >>   static inline struct sock *sk_to_full_sk(struct sock *sk)
> >>   {
> >>   #ifdef CONFIG_INET
> >> -       if (sk && sk->sk_state == TCP_NEW_SYN_RECV)
> >> +       if (sk && READ_ONCE(sk->sk_state) == TCP_NEW_SYN_RECV)
> >>                  sk = inet_reqsk(sk)->rsk_listener;
> >> +       if (sk && READ_ONCE(sk->sk_state) == TCP_TIME_WAIT)
> >> +               sk = NULL;
> >>   #endif
> >>          return sk;
> >>   }
> >
> > It appears some callers do not check if the return value could be NULL.
> > I will have to add in v2 :
> >
> > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> > index ce91d9b2acb9f8991150ceead4475b130bead438..c3ffb45489a6924c1bc80355e862e243ec195b01
> > 100644
> > --- a/include/linux/bpf-cgroup.h
> > +++ b/include/linux/bpf-cgroup.h
> > @@ -209,7 +209,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
> >          int __ret = 0;                                                         \
> >          if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {                    \
>
> The above "&& sk" test can probably be removed after the "__sk &&" addition below.

Yes, I can do that.

>
> >                  typeof(sk) __sk = sk_to_full_sk(sk);                           \
> > -               if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) &&        \
> > +               if (__sk && sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) &&        \
>
> sk_to_full_sk() includes the TCP_TIME_WAIT check now. I wonder if testing __sk
> for NULL is good enough and the sk_fullsock(__sk) check can be removed also.

+2

>
> Thanks for working on this series. It is useful for the bpf prog.

Thanks.