Received: by 2002:ac0:bc90:0:0:0:0:0 with SMTP id a16csp497859img; Thu, 21 Mar 2019 02:47:04 -0700 (PDT) X-Google-Smtp-Source: APXvYqzMHuda2Y4xnXnapfuyEut1cX5WODrLXiTq87WHpRvdunph1Ad8TbmDsQLg2JG5afyzgn1Q X-Received: by 2002:a62:b508:: with SMTP id y8mr2438599pfe.140.1553161624731; Thu, 21 Mar 2019 02:47:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1553161624; cv=none; d=google.com; s=arc-20160816; b=VP7flUA7nhcZ0gm3MeQxTxjlPcwxpkszd2bwXjXipNP2lM6zFBv3HaA4mSkY/pB/4N 4YcqIQOyu5h3B0AXLfaWgwB0ILTjEmZHAXELe5jvdtKHfkFRclc1irWw4wNXQWmObkT6 dWgCgON03raylGHdUD7Se3A39sjosFnLGHRxx6kgpj9lJ9EnfyMVhuikP0PVFo1YG5j3 iHwz3f65EbP3h3Heo1IKBqs55zKBHVuepoe5GqTri7KPptzzQTWNArELlj2XCGaGLE8u tFT6efQScXcmk5o5Ntudd5KNvIbZyeUvGxLS9TtdsKlt3kzHyEWoDGeDP3d3kmajb2rU sYPQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:date:cc:to:from:subject :message-id; bh=XJrte+QIAKqvhC5dTGYUKH1Sv5NVPwYTt9o6s9ApKdw=; b=ge8GACJHZu0Us5e6QPrInUb9l9Z9yr3OnPu8ziyxZhhfKmHmQYsT9B0GsqdwWuiW99 uTESJEpvdTa44sR1Fhw/Eiivs3lROi9rI9JvVroSyfu8fhL4Wg78ttsHRbbwEG52/6O0 ISjEqx2lE5zpJtnDz03snDg6yyrUJo/T8rY75qJrLh4PE2EMaXcemSZeKaQt5X1IpQVb FzWhDGtF+ylIOcAB23vA1AW3oqMqHlctxHF+4JPSj4zE3Cd44o0/QU6M+44dOmpvHKr5 alxgT5CKoQQ5v1qIFC1Ba81e8V3oukN5ewA6DAp8sZK4upxa2Y327ksdQ8+BlIfMGiI/ zANA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id cb2si4325411plb.201.2019.03.21.02.46.46; Thu, 21 Mar 2019 02:47:04 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728213AbfCUJpp (ORCPT + 99 others); Thu, 21 Mar 2019 05:45:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50920 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727829AbfCUJpo (ORCPT ); Thu, 21 Mar 2019 05:45:44 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 015DA81F18; Thu, 21 Mar 2019 09:45:44 +0000 (UTC) Received: from localhost.localdomain (unknown [10.32.181.117]) by smtp.corp.redhat.com (Postfix) with ESMTP id 48CF660857; Thu, 21 Mar 2019 09:45:42 +0000 (UTC) Message-ID: Subject: Re: [net-next PATCH v3 4/8] net: Change return type of sk_busy_loop from bool to void From: Paolo Abeni To: Christoph Paasch , Alexander Duyck Cc: netdev , linux-kernel@vger.kernel.org, sridhar.samudrala@intel.com, Eric Dumazet , David Miller , linux-api@vger.kernel.org Date: Thu, 21 Mar 2019 10:45:41 +0100 In-Reply-To: References: <20170324164902.15226.48358.stgit@localhost.localdomain> <20170324170812.15226.97497.stgit@localhost.localdomain> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5 (3.30.5-1.fc29) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 21 Mar 2019 09:45:44 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > wrote: > > From: Alexander Duyck > > > > > 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 > > Acked-by: Eric Dumazet > > --- > > 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: commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac Author: samanthakumar 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. 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;