netdev - Re: [PATCH net] udp: fix busy polling

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: <CANn89i+x9qKCaQVbaf+TO8TJWMaLcW-efhAuNtatFuyrza-UaQ@mail.gmail.com>
Date: Thu, 18 Jan 2024 16:52:23 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Willem de Bruijn <willemb@...gle.com>, David Ahern <dsahern@...nel.org>, 
	netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net] udp: fix busy polling

On Thu, Jan 18, 2024 at 4:15 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Eric Dumazet wrote:
> > Generic sk_busy_loop_end() only looks at sk->sk_receive_queue
> > for presence of packets.
> >
> > Problem is that for UDP sockets after blamed commit, some packets
> > could be present in another queue: udp_sk(sk)->reader_queue
> >
> > In some cases, a busy poller could spin until timeout expiration,
> > even if some packets are available in udp_sk(sk)->reader_queue.
> >
> > Fixes: 2276f58ac589 ("udp: use a separate rx queue for packet reception")
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > ---
> >  include/linux/skmsg.h |  6 ------
> >  include/net/sock.h    |  5 +++++
> >  net/core/sock.c       | 10 +++++++++-
> >  3 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> > index 888a4b217829fd4d6baf52f784ce35e9ad6bd0ed..e65ec3fd27998a5b82fc2c4597c575125e653056 100644
> > --- a/include/linux/skmsg.h
> > +++ b/include/linux/skmsg.h
> > @@ -505,12 +505,6 @@ static inline bool sk_psock_strp_enabled(struct sk_psock *psock)
> >       return !!psock->saved_data_ready;
> >  }
> >
> > -static inline bool sk_is_udp(const struct sock *sk)
> > -{
> > -     return sk->sk_type == SOCK_DGRAM &&
> > -            sk->sk_protocol == IPPROTO_UDP;
> > -}
> > -
> >  #if IS_ENABLED(CONFIG_NET_SOCK_MSG)
> >
> >  #define BPF_F_STRPARSER      (1UL << 1)
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index a7f815c7cfdfdf1296be2967fd100efdb10cdd63..b1ceba8e179aa5cc4c90e98d353551b3a3e1ab86 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2770,6 +2770,11 @@ static inline bool sk_is_tcp(const struct sock *sk)
> >       return sk->sk_type == SOCK_STREAM && sk->sk_protocol == IPPROTO_TCP;
> >  }
> >
> > +static inline bool sk_is_udp(const struct sock *sk)
> > +{
> > +     return sk->sk_type == SOCK_DGRAM && sk->sk_protocol == IPPROTO_UDP;
> > +}
> > +
>
> Since busy polling code is protocol (family) independent, is it safe
> to assume sk->sk_family == PF_INET or PF_INET6 here?

Hmm. This is a valid point.

It seems the only current user of sk_is_udp(), bpf_sk_lookup_assign()
can only be used from inet sockets,

But if we use sk_is_udp() in a core function like sk_busy_loop_end(),
we need to be more careful...