From mboxrd@z Thu Jan 1 00:00:00 1970 From: Willem de Bruijn Subject: Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather Date: Mon, 14 May 2018 19:30:04 -0400 Message-ID: References: <20180514230747.118875-1-willemdebruijn.kernel@gmail.com> <20180514230747.118875-4-willemdebruijn.kernel@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Network Development , David Miller , Willem de Bruijn To: Eric Dumazet Return-path: Received: from mail-ua0-f196.google.com ([209.85.217.196]:36716 "EHLO mail-ua0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752165AbeENXap (ORCPT ); Mon, 14 May 2018 19:30:45 -0400 Received: by mail-ua0-f196.google.com with SMTP id b25-v6so9587703uak.3 for ; Mon, 14 May 2018 16:30:45 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, May 14, 2018 at 7:12 PM, Eric Dumazet wrote: > > > On 05/14/2018 04:07 PM, Willem de Bruijn wrote: >> From: Willem de Bruijn >> >> Paged allocation stores most payload in skb frags. This helps udp gso >> by avoiding copying from the gso skb to segment skb in skb_segment. >> >> But without scatter-gather, data must be linear, so do not use paged >> mode unless NETIF_F_SG. >> >> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso") >> Reported-by: Sean Tranchetti >> Signed-off-by: Willem de Bruijn >> --- >> net/ipv4/ip_output.c | 2 +- >> net/ipv6/ip6_output.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >> index b5e21eb198d8..b38731d8a44f 100644 >> --- a/net/ipv4/ip_output.c >> +++ b/net/ipv4/ip_output.c >> @@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk, >> >> exthdrlen = !skb ? rt->dst.header_len : 0; >> mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize; >> - paged = !!cork->gso_size; >> + paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG); >> >> if (cork->tx_flags & SKBTX_ANY_SW_TSTAMP && >> sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID) >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c >> index 7f4493080df6..35a940b9f208 100644 >> --- a/net/ipv6/ip6_output.c >> +++ b/net/ipv6/ip6_output.c >> @@ -1262,7 +1262,7 @@ static int __ip6_append_data(struct sock *sk, >> dst_exthdrlen = rt->dst.header_len - rt->rt6i_nfheader_len; >> } >> >> - paged = !!cork->gso_size; >> + paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG); >> mtu = cork->gso_size ? IP6_MAX_MTU : cork->fragsize; >> orig_mtu = mtu; >> >> > > As I said, this wont help for stacked device > > bonding might advertise NETIF_F_SG, but one slave might not. I don't quite follow. The reported crash happens in the protocol layer, because of this check. With pagedlen we have not allocated sufficient space for the skb_put. if (!(rt->dst.dev->features&NETIF_F_SG)) { unsigned int off; off = skb->len; if (getfrag(from, skb_put(skb, copy), offset, copy, off, skb) < 0) { __skb_trim(skb, off); err = -EFAULT; goto error; } } else { int i = skb_shinfo(skb)->nr_frags; Are you referring to a separate potential issue in the gso layer? If a bonding device advertises SG, but a slave does not, then skb_segment on the slave should build linear segs? I have not tested that.