On Thu, Mar 21, 2019 at 5:46 AM Paolo Abeni [off-list ref] wrote:

Hi,

On Wed, 2019-03-20 at 11:35 -0700, Christoph Paasch wrote:

quoted

Hello,

On Fri, Mar 24, 2017 at 3:23 PM Alexander Duyck
[off-list ref] wrote:

quoted

From: Alexander Duyck <redacted>

quoted

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 <redacted>
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 [off-list ref]
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/