From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Christoph Paasch <christoph.paasch@gmail.com>,
Alexander Duyck <alexander.duyck@gmail.com>,
netdev <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
"Samudrala, Sridhar" <sridhar.samudrala@intel.com>,
Eric Dumazet <edumazet@google.com>,
David Miller <davem@davemloft.net>,
Linux API <linux-api@vger.kernel.org>,
Alexei Starovoitov <ast@kernel.org>
Subject: Re: [net-next PATCH v3 4/8] net: Change return type of sk_busy_loop from bool to void
Date: Thu, 21 Mar 2019 10:28:45 -0400 [thread overview]
Message-ID: <CAF=yD-JwetDu2NjHi=HX=2UHHFKE5i9arCOP7bcEFN-fxvR58Q@mail.gmail.com> (raw)
In-Reply-To: <e532fdb97e142eca2484e5abbeca4c30d11fbad6.camel@redhat.com>
On Thu, Mar 21, 2019 at 5:46 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi,
>
> On Wed, 2019-03-20 at 11:35 -0700, Christoph Paasch wrote:
> > Hello,
> >
> > On Fri, Mar 24, 2017 at 3:23 PM Alexander Duyck
> > <alexander.duyck@gmail.com> wrote:
> > > From: Alexander Duyck <alexander.h.duyck@intel.com>
> > >
> > > > From what I can tell there is only a couple spots where we are actually
> > > checking the return value of sk_busy_loop. As there are only a few
> > > consumers of that data, and the data being checked for can be replaced
> > > with a check for !skb_queue_empty() we might as well just pull the code
> > > out of sk_busy_loop and place it in the spots that actually need it.
> > >
> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> > > Acked-by: Eric Dumazet <edumazet@google.com>
> > > ---
> > > include/net/busy_poll.h | 5 ++---
> > > net/core/datagram.c | 8 ++++++--
> > > net/core/dev.c | 25 +++++++++++--------------
> > > net/sctp/socket.c | 9 ++++++---
> > > 4 files changed, 25 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
> > > index b82d6ba70a14..c55760f4820f 100644
> > > --- a/include/net/busy_poll.h
> > > +++ b/include/net/busy_poll.h
> > > @@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
> > > return time_after(now, end_time);
> > > }
> > >
> > > -bool sk_busy_loop(struct sock *sk, int nonblock);
> > > +void sk_busy_loop(struct sock *sk, int nonblock);
> > >
> > > #else /* CONFIG_NET_RX_BUSY_POLL */
> > > static inline unsigned long net_busy_loop_on(void)
> > > @@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
> > > return true;
> > > }
> > >
> > > -static inline bool sk_busy_loop(struct sock *sk, int nonblock)
> > > +static inline void sk_busy_loop(struct sock *sk, int nonblock)
> > > {
> > > - return false;
> > > }
> > >
> > > #endif /* CONFIG_NET_RX_BUSY_POLL */
> > > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > > index ea633342ab0d..4608aa245410 100644
> > > --- a/net/core/datagram.c
> > > +++ b/net/core/datagram.c
> > > @@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct
> > > sock *sk, unsigned int flags,
> > > }
> > >
> > > spin_unlock_irqrestore(&queue->lock, cpu_flags);
> > > - } while (sk_can_busy_loop(sk) &&
> > > - sk_busy_loop(sk, flags & MSG_DONTWAIT));
> > > +
> > > + if (!sk_can_busy_loop(sk))
> > > + break;
> > > +
> > > + sk_busy_loop(sk, flags & MSG_DONTWAIT);
> > > + } while (!skb_queue_empty(&sk->sk_receive_queue));
> >
> > since this change I am hitting stalls where it's looping in this
> > while-loop with syzkaller.
> >
> > It worked prior to this change because sk->sk_napi_id was not set thus
> > sk_busy_loop would make us get out of the loop.
> >
> > Now, it keeps on looping because there is an skb in the queue with
> > skb->len == 0 and we are peeking with an offset, thus
> > __skb_try_recv_from_queue will return NULL and thus we have no way of
> > getting out of the loop.
> >
> > I'm not sure what would be the best way to fix it. I don't see why we
> > end up with an skb in the list with skb->len == 0. So, shooting a
> > quick e-mail, maybe somebody has an idea :-)
> > I have the syzkaller-reproducer if needed.
>
> IIRC we can have 0 len UDP packet sitting on sk_receive_queue since:
Yes, as of header before enqueue pulling zero byte datagrams may be
queued. And these need to be delivered, among other reason for their
cmsg metadata.
> commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
> Author: samanthakumar <samanthakumar@google.com>
> Date: Tue Apr 5 12:41:15 2016 -0400
>
> udp: remove headers from UDP packets before queueing
>
> Both __skb_try_recv_datagram() and napi_busy_loop() assume that we
> received some packets if the queue is not empty. When peeking such
> assumption is not true, we should check if the last packet is changed,
> as __skb_recv_datagram() already does.
Good catch. The condition in sk_busy_loop_end is not easy to address.
Since busy poll is an optimization and poll at offset rare, one way
out may be to amend the __sk_can_busy_loop test in __skb_recv_udp to
disallow busy polling together with peek at offset.
The difference in behavior betwee __skb_try_recv_datagram and
__skb_recv_datagram also reminds of Alexei's earlier report (without
busy polling, seemingly with a list corruption introduced elsewhere)
in
[net-next,1/3] net/sock: factor out dequeue/peek with offset code
https://patchwork.ozlabs.org/patch/762327/
> So I *think* the root cause of
> this issue is older than Alex's patch.
>
> The following - completely untested - should avoid the unbounded loop,
> but it's not a complete fix, I *think* we should also change
> sk_busy_loop_end() in a similar way, but that is a little more complex
> due to the additional indirections.
>
> Could you please test it?
>
> Any feedback welcome!
>
>
> Could you please test it?
>
> Paolo
> ---
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index b2651bb6d2a3..e657289db4ac 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -279,7 +279,7 @@ struct sk_buff *__skb_try_recv_datagram(struct sock
> *sk, unsigned int flags,
> break;
>
> sk_busy_loop(sk, flags & MSG_DONTWAIT);
> - } while (!skb_queue_empty(&sk->sk_receive_queue));
> + } while (sk->sk_receive_queue.prev != *last);
>
> error = -EAGAIN;
>
>
next prev parent reply other threads:[~2019-03-21 14:28 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-24 17:07 [net-next PATCH v3 0/8] Add busy poll support for epoll Alexander Duyck
2017-03-24 17:08 ` [net-next PATCH v3 2/8] tcp: Record Rx hash and NAPI ID in tcp_child_process Alexander Duyck
2017-03-24 17:08 ` [net-next PATCH v3 3/8] net: Only define skb_mark_napi_id in one spot instead of two Alexander Duyck
[not found] ` <20170324164902.15226.48358.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2017-03-24 17:07 ` [net-next PATCH v3 1/8] net: Busy polling should ignore sender CPUs Alexander Duyck
2017-03-24 17:08 ` [net-next PATCH v3 4/8] net: Change return type of sk_busy_loop from bool to void Alexander Duyck
2019-03-20 18:35 ` Christoph Paasch
2019-03-20 19:40 ` David Miller
2019-03-21 9:45 ` Paolo Abeni
2019-03-21 14:28 ` Willem de Bruijn [this message]
2019-03-21 16:43 ` Alexander Duyck
2019-03-22 3:05 ` Christoph Paasch
2019-03-22 10:33 ` Paolo Abeni
2019-03-22 12:59 ` Eric Dumazet
2019-03-22 13:35 ` Paolo Abeni
2019-03-22 19:25 ` Christoph Paasch
2017-03-24 17:08 ` [net-next PATCH v3 5/8] net: Track start of busy loop instead of when it should end Alexander Duyck
[not found] ` <20170324170818.15226.51326.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2017-03-25 3:34 ` Eric Dumazet
2017-03-25 2:23 ` [net-next PATCH v3 0/8] Add busy poll support for epoll David Miller
2017-03-25 3:49 ` David Miller
2017-03-24 17:08 ` [net-next PATCH v3 6/8] net: Commonize busy polling code to focus on napi_id instead of socket Alexander Duyck
2017-03-24 17:08 ` [net-next PATCH v3 7/8] epoll: Add busy poll support to epoll with socket fds Alexander Duyck
[not found] ` <20170324170830.15226.9932.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2017-03-25 3:33 ` Eric Dumazet
2017-03-24 17:08 ` [net-next PATCH v3 8/8] net: Introduce SO_INCOMING_NAPI_ID Alexander Duyck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAF=yD-JwetDu2NjHi=HX=2UHHFKE5i9arCOP7bcEFN-fxvR58Q@mail.gmail.com' \
--to=willemdebruijn.kernel@gmail.com \
--cc=alexander.duyck@gmail.com \
--cc=ast@kernel.org \
--cc=christoph.paasch@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sridhar.samudrala@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).