From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH v2 0/4] Check size of packets before sending Date: Thu, 25 Jan 2018 04:40:36 -0800 Message-ID: <1516884036.3715.45.camel@gmail.com> References: <20180125043109.28332-1-dja@axtens.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Jason Wang , Pravin Shelar , Marcelo Ricardo Leitner , Manish.Chopra@cavium.com, dev@openvswitch.org To: Daniel Axtens , netdev@vger.kernel.org Return-path: Received: from mail-io0-f193.google.com ([209.85.223.193]:36715 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752071AbeAYMkk (ORCPT ); Thu, 25 Jan 2018 07:40:40 -0500 Received: by mail-io0-f193.google.com with SMTP id l17so8489470ioc.3 for ; Thu, 25 Jan 2018 04:40:39 -0800 (PST) In-Reply-To: <20180125043109.28332-1-dja@axtens.net> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 2018-01-25 at 15:31 +1100, Daniel Axtens wrote: > There are a few ways we can send packets that are too large to a > network driver. > > When non-GSO packets are forwarded, we validate their size, based on > the MTU of the destination device. However, when GSO packets are > forwarded, we do not validate their size. We implicitly assume that > when they are segmented, the resultant packets will be correctly > sized. > > This is not always the case. > > We observed a case where a packet received on an ibmveth device had a > GSO size of around 10kB. This was forwarded by Open vSwitch to a bnx2x > device, where it caused a firmware assert. This is described in detail > at [0] and was the genesis of this series. > > Rather than fixing this in the driver, this series fixes the > core path. It does it in 2 steps: > > 1) make is_skb_forwardable check GSO packets - this catches bridges > > 2) make validate_xmit_skb check the size of all packets, so as to > catch everything else (e.g. macvlan, tc mired, OVS) > > I am a bit nervous about how this series will interact with nested > VLANs, as the existing code only allows for one VLAN_HLEN. (Previously > these packets would sail past unchecked.) But I thought it would be > prudent to get more eyes on this sooner rather than later. > > Thanks, > Daniel > > v1: https://www.spinics.net/lists/netdev/msg478634.html > Changes in v2: > > - improve names, thanks Marcelo Ricardo Leitner > > - add check to xmit_validate_skb; thanks to everyone who participated > in the discussion. > > - drop extra check in Open vSwitch. Bad packets will be caught by > validate_xmit_skb for now and we can come back and add it later if > OVS people would like the extra logging. > > [0]: https://patchwork.ozlabs.org/patch/859410/ > > Cc: Jason Wang > Cc: Pravin Shelar > Cc: Marcelo Ricardo Leitner > Cc: Manish.Chopra@cavium.com > Cc: dev@openvswitch.org > > Daniel Axtens (4): > net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len > net: move skb_gso_mac_seglen to skbuff.h > net: is_skb_forwardable: check the size of GSO segments > net: check the size of a packet in validate_xmit_skb > > include/linux/skbuff.h | 18 ++++++++- > net/core/dev.c | 24 ++++++++---- > net/core/skbuff.c | 66 ++++++++++++++++++++++++++------- > net/ipv4/ip_forward.c | 2 +- > net/ipv4/ip_output.c | 2 +- > net/ipv4/netfilter/nf_flow_table_ipv4.c | 2 +- > net/ipv6/ip6_output.c | 2 +- > net/ipv6/netfilter/nf_flow_table_ipv6.c | 2 +- > net/mpls/af_mpls.c | 2 +- > net/sched/sch_tbf.c | 10 ----- > net/xfrm/xfrm_device.c | 2 +- > 11 files changed, 93 insertions(+), 39 deletions(-) > May I ask which tree are you targeting ? ( Documentation/networking/netdev-FAQ.txt ) Anything touching GSO is very risky and should target net-next, especially considering 4.15 is released this week end. Are we really willing to backport this intrusive series in stable trees, or do we have a smaller fix for bnx2x ? Thanks.