From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp2120.oracle.com ([141.146.126.78]:37692 "EHLO aserp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161629AbeCAUl6 (ORCPT ); Thu, 1 Mar 2018 15:41:58 -0500 Subject: Re: [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements To: "netdev@vger.kernel.org" , Daniel Axtens References: <20180301061340.15464-1-dja@axtens.net> From: Shannon Nelson Message-ID: <439489cc-8fa7-9216-0247-7d9616796854@oracle.com> Date: Thu, 1 Mar 2018 12:41:44 -0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: > From: Daniel Axtens > Date: Wed, Feb 28, 2018 at 10:13 PM > > As requested [1], I went through and had a look at users of gso_size to > see if there were things that need to be fixed to consider > GSO_BY_FRAGS, and I have tried to improve our helper functions to deal > with this case. > > I found a few. This fixes bugs relating to the use of > skb_gso_*_seglen() where GSO_BY_FRAGS is not considered. > > Patch 1 renames skb_gso_validate_mtu to skb_gso_validate_network_len. > This is follow-up to my earlier patch 2b16f048729b ("net: create > skb_gso_validate_mac_len()"), and just makes everything a bit clearer. > > Patches 2 and 3 replace the final users of skb_gso_network_seglen() - > which doesn't consider GSO_BY_FRAGS - with > skb_gso_validate_network_len(), which does. This allows me to make the > skb_gso_*_seglen functions private in patch 4 - now future users won't > accidentally do the wrong comparison. > > Two things remain. One is qdisc_pkt_len_init, which is discussed at > [2] - it's caught up in the GSO_DODGY mess. I don't have any expertise > in GSO_DODGY, and it looks like a good clean fix will involve > unpicking the whole validation mess, so I have left it for now. > > Secondly, there are 3 eBPF opcodes that change the gso_size of an SKB > and don't consider GSO_BY_FRAGS. This is going through the bpf tree. > > Regards, > Daniel > > [1] https://patchwork.ozlabs.org/comment/1852414/ > [2] https://www.spinics.net/lists/netdev/msg482397.html > > PS: This is all in the core networking stack. For a driver to be > affected by this it would need to support NETIF_F_GSO_SCTP / > NETIF_F_GSO_SOFTWARE and then either use gso_size or not be a purely > virtual device. (Many drivers look at gso_size, but do not support > SCTP segmentation, so the core network will segment an SCTP gso before > it hits them.) Based on that, the only driver that may be affected is > sunvnet, but I have no way of testing it, so I haven't looked at it. I took a quick peek into sunvnet to check on this, and it looks like the code in sunvnet_common.c::vnet_handle_offloads() is already mis-handling SCTP gso packets, so I don't think you're adding any more breakage. I suspect we should change sunvnet's use of NETIF_F_GSO_SOFTWARE to NETIF_F_ALL_TSO... sln > > v2: split out bpf stuff > fix review comments from Dave Miller > > Daniel Axtens (4): > net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len > net: sched: tbf: handle GSO_BY_FRAGS case in enqueue > net: xfrm: use skb_gso_validate_network_len() to check gso sizes > net: make skb_gso_*_seglen functions private > > include/linux/skbuff.h | 35 +----------------------- > net/core/skbuff.c | 48 ++++++++++++++++++++++++++++----- > net/ipv4/ip_forward.c | 2 +- > net/ipv4/ip_output.c | 2 +- > net/ipv4/netfilter/nf_flow_table_ipv4.c | 2 +- > net/ipv4/xfrm4_output.c | 3 ++- > net/ipv6/ip6_output.c | 2 +- > net/ipv6/netfilter/nf_flow_table_ipv6.c | 2 +- > net/ipv6/xfrm6_output.c | 2 +- > net/mpls/af_mpls.c | 2 +- > net/sched/sch_tbf.c | 3 ++- > net/xfrm/xfrm_device.c | 2 +- > 12 files changed, 54 insertions(+), 51 deletions(-) > > -- > 2.14.1 > > >