| 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
| ||
|
Message-ID: <CAMzD94RYOysq0zmDzqqSwHUZNt57-Vob_zvBmJ=em7iZEd=9AQ@mail.gmail.com>
Date: Mon, 14 Oct 2024 11:39:38 -0400
From: Brian Vazquez <brianvv@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Martin KaFai Lau <martin.lau@...nel.org>,
Kuniyuki Iwashima <kuniyu@...zon.com>, Neal Cardwell <ncardwell@...gle.com>, netdev@...r.kernel.org,
eric.dumazet@...il.com
Subject: Re: [PATCH v3 net-next 1/5] net: add TIME_WAIT logic to sk_to_full_sk()
On Mon, Oct 14, 2024 at 11:24 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Mon, Oct 14, 2024 at 5:03 PM Brian Vazquez <brianvv@...gle.com> wrote:
> >
> > On Mon, Oct 14, 2024 at 10:28 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >>
> >> On Mon, Oct 14, 2024 at 4:01 PM Brian Vazquez <brianvv@...gle.com> wrote:
> >> >
> >> > Thanks Eric for the patch series! I left some comments inline
> >> >
> >> >
> >> > On Thu, Oct 10, 2024 at 1:48 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.
> >> > >
> >> > > v3: also changed sk_const_to_full_sk()
> >> > >
> >> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> >> > > ---
> >> > > include/linux/bpf-cgroup.h | 2 +-
> >> > > include/net/inet_sock.h | 8 ++++++--
> >> > > net/core/filter.c | 6 +-----
> >> > > 3 files changed, 8 insertions(+), 8 deletions(-)
> >> > >
> >> > > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> >> > > index ce91d9b2acb9f8991150ceead4475b130bead438..f0f219271daf4afea2666c4d09fd4d1a8091f844 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) { \
> >> > > typeof(sk) __sk = sk_to_full_sk(sk); \
> >> > > - if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) && \
> >> > > + if (__sk && __sk == skb_to_full_sk(skb) && \
> >> > > cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS)) \
> >> > > __ret = __cgroup_bpf_run_filter_skb(__sk, skb, \
> >> > > CGROUP_INET_EGRESS); \
> >> > > diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> >> > > index f01dd273bea69d2eaf7a1d28274d7f980942b78a..56d8bc5593d3dfffd5f94cf7c6383948881917df 100644
> >> > > --- a/include/net/inet_sock.h
> >> > > +++ b/include/net/inet_sock.h
> >> > > @@ -321,8 +321,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;
> >> > > }
> >> > > @@ -331,8 +333,10 @@ static inline struct sock *sk_to_full_sk(struct sock *sk)
> >> > > static inline const struct sock *sk_const_to_full_sk(const 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 = ((const struct request_sock *)sk)->rsk_listener;
> >> > > + if (sk && READ_ONCE(sk->sk_state) == TCP_TIME_WAIT)
> >> > > + sk = NULL;
> >> > > #endif
> >> > > return sk;
> >> > > }
> >> > > diff --git a/net/core/filter.c b/net/core/filter.c
> >> > > index bd0d08bf76bb8de39ca2ca89cda99a97c9b0a034..202c1d386e19599e9fc6e0a0d4a95986ba6d0ea8 100644
> >> > > --- a/net/core/filter.c
> >> > > +++ b/net/core/filter.c
> >> > > @@ -6778,8 +6778,6 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >> > > /* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk
> >> > > * sock refcnt is decremented to prevent a request_sock leak.
> >> > > */
> >> > > - if (!sk_fullsock(sk2))
> >> > > - sk2 = NULL;
> >> >
> >> > IIUC, we still want the condition above since sk_to_full_sk can return
> >> > the request socket in which case the helper should return NULL, so we
> >> > still need the refcnt decrement?
> >> >
> >> > > if (sk2 != sk) {
> >> > > sock_gen_put(sk);
> >>
> >> Note that we call sock_gen_put(sk) here, not sock_gen_put(sk2);
> >>
> >>
> >> sk is not NULL here, so if sk2 is NULL, we will take this branch.
> >
> >
> > IIUC __bpf_sk_lookup calls __bpf_skc_lookup which can return a request listener socket and takes a refcnt, but __bpf_sk_lookup should only return full_sk (no request nor time_wait).
> >
> > That's why the function tries to detect whether req or time_wait was retrieved by __bpf_skc_lookup and if so, we invalidate the return: sk = NULL, and decrement the refcnt. This is done by having sk2 and then comparing vs sk, and if sk2 is invalid because time_wait or listener, then we decrement sk (the original return from __bpf_skc_lookup, which took a refcnt)
> >
> > I agree that after the change to sk_to_full_sk, for time_wait it will return NULL, hence the condition is repetitive.
> >
> > if (!sk_fullsock(sk2))
> > sk2 = NULL;
> >
> > but sk_to_full_sk can still retrieve the listener: sk = inet_reqsk(sk)->rsk_listener; in which case we would like to still use
> > if (!sk_fullsock(sk2))
> > sk2 = NULL;
> >
> > to invalidate the request socket, decrement the refcount and sk = sk2 ; // which makes sk == NULL?
> >
> > I think removing that condition allows __bpf_sk_lookup to return the req socket, which wasn't possible before?
>
> It was not possible before, and not possible after :
>
> static inline struct sock *sk_to_full_sk(struct sock *sk)
> {
> #ifdef CONFIG_INET
> 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) // NEW CODE
> sk = NULL; // NEW CODE
> #endif
> return sk;
> }
>
> if sk was a request socket, sk2 would be the listener.
This is the part that I missed, I got misled by the comment above the dead code.
Thanks for clarifying!
>
> sk2 being a listener means that sk_fullsock(sk2) is true.
>
> if (!sk_fullsock(sk2))
> sk2 = NULL;
>
> So really this check was only meant for TIME_WAIT, and it is now done
> directly from sk_to_full_sk()
>
> Therefore we can delete this dead code.
Reviewed-by: Brian Vazquez <brianvv@...gle.com>
Powered by blists - more mailing lists