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: Tue, 15 May 2018 19:57:28 -0400 Message-ID: References: <20180514230747.118875-1-willemdebruijn.kernel@gmail.com> <20180514230747.118875-4-willemdebruijn.kernel@gmail.com> <7557fc96-eb5a-56cb-28b5-a49abe8dae7c@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-vk0-f66.google.com ([209.85.213.66]:35415 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751402AbeEOX6J (ORCPT ); Tue, 15 May 2018 19:58:09 -0400 Received: by mail-vk0-f66.google.com with SMTP id g72-v6so1293131vke.2 for ; Tue, 15 May 2018 16:58:09 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, May 15, 2018 at 4:04 PM, Willem de Bruijn wrote: > On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn > wrote: >> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet wrote: >>> >>> >>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote: >>> >>>> 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. >>> >>> Given that the device attribute could change under us, we need to not >>> crash, even if initially we thought NETIF_F_SG was available. >>> >>> Unless you want to hold RTNL in UDP xmit :) >>> >>> Ideally, GSO should be always on, as we did for TCP. >>> >>> Otherwise, I can guarantee syzkaller will hit again. >> >> Ah, right. Thanks, Eric! >> >> I'll read that feature bit only once. > > This issue is actually deeper and not specific to gso. > With corking it is trivial to turn off sg in between calls. > > I'll need to send a separate fix for that. This would do it. The extra branch is unfortunate, but I see no easy way around it for the corking case. It will obviously not build a linear skb, but validate_xmit_skb will clean that up for such edge cases. diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 66340ab750e6..e7daec7c7421 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -1040,7 +1040,8 @@ static int __ip_append_data(struct sock *sk, if (copy > length) copy = length; - if (!(rt->dst.dev->features&NETIF_F_SG)) { + if (!(rt->dst.dev->features&NETIF_F_SG) && + skb_tailroom(skb) >= copy) { unsigned int off;