Date:   Thu, 1 Mar 2018 12:41:44 -0800
From:   Shannon Nelson <shannon.nelson@...cle.com>
To:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Daniel Axtens <dja@...ens.net>
Subject: Re: [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements

> From: Daniel Axtens <dja@...ens.net>
> 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
> 
> 
> 

Powered by blists - more mailing lists