netdev - Re: [PATCH net-next] sctp: 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]

Date: Fri, 22 Dec 2023 13:34:09 -0500
From: Xin Long <lucien.xin@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	Jacob Moroni <jmoroni@...gle.com>, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net-next] sctp: fix busy polling

On Fri, Dec 22, 2023 at 12:05 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Dec 22, 2023 at 5:08 PM Xin Long <lucien.xin@...il.com> wrote:
> >
> > On Tue, Dec 19, 2023 at 12:00 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > Busy polling while holding the socket lock makes litle sense,
> > > because incoming packets wont reach our receive queue.
> > >
> > > Fixes: 8465a5fcd1ce ("sctp: add support for busy polling to sctp protocol")
> > > Reported-by: Jacob Moroni <jmoroni@...gle.com>
> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> > > Cc: Xin Long <lucien.xin@...il.com>
> > > ---
> > >  net/sctp/socket.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index 5fb02bbb4b349ef9ab9c2790cccb30fb4c4e897c..6b9fcdb0952a0fe599ae5d1d1cc6fa9557a3a3bc 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -2102,6 +2102,10 @@ static int sctp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> > >         if (unlikely(flags & MSG_ERRQUEUE))
> > >                 return inet_recv_error(sk, msg, len, addr_len);
> > >
> > > +       if (sk_can_busy_loop(sk) &&
> > > +           skb_queue_empty_lockless(&sk->sk_receive_queue))
> > > +               sk_busy_loop(sk, flags & MSG_DONTWAIT);
> > > +
> > Here is no any sk_state check, if the SCTP socket(TCP type) has been
> > already closed by peer, will sctp_recvmsg() block here?
>
> Busy polling is only polling the NIC queue, hoping to feed this socket
> for incoming packets.
OK, will it block if there's no incoming packets on the NIC queue?

If yes, when sysctl net.core.busy_read=1, my concern is:

     client                server
     -------------------------------
                           listen()
     connect()
                           accept()
     close()
                           recvmsg() <----

recvmsg() is supposed to return right away as the connection is
already close(). With this patch, will recvmsg() be able to do
that if no more incoming packets in the NIC after close()?

Thanks.

>
> Using more than a lockless read of sk->sk_receive_queue is not really necessary,
> and racy anyway.
>
> Eliezer Tamir added a check against sk_state for no good reason in
> TCP, my plan is to remove it.
>
> There are other states where it still makes sense to allow busy polling.
>
>
> >
> > Maybe here it needs a `!(sk->sk_shutdown & RCV_SHUTDOWN)` check,
> > which is set when it's closed by the peer.
>
> See above. Keep this as simple as possible...
>
>
> >
> > Thanks
> >
> > >         lock_sock(sk);
> > >
> > >         if (sctp_style(sk, TCP) && !sctp_sstate(sk, ESTABLISHED) &&
> > > @@ -9046,12 +9050,6 @@ struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags, int *err)
> > >                 if (sk->sk_shutdown & RCV_SHUTDOWN)
> > >                         break;
> > >
> > > -               if (sk_can_busy_loop(sk)) {
> > > -                       sk_busy_loop(sk, flags & MSG_DONTWAIT);
> > > -
> > > -                       if (!skb_queue_empty_lockless(&sk->sk_receive_queue))
> > > -                               continue;
> > > -               }
> > >
> > >                 /* User doesn't want to wait.  */
> > >                 error = -EAGAIN;
> > > --
> > > 2.43.0.472.g3155946c3a-goog
> > >