[PATCH v3 0/2] bnx2x: disable GSO on too-large packets

[PATCH v3 0/2] bnx2x: disable GSO on too-large packets

[Daniel Axtens]

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].

Ultimately we want a fix in the core, but that is very tricky to
backport. So for now, just stop the bnx2x driver from crashing.

When net-next re-opens I will send the fix to the core and a revert
for this.

Marcelo: I have left renaming skb_gso_validate_mtu() for the
next series.

Thanks,
Daniel
   
[0]: https://patchwork.ozlabs.org/patch/859410/

Cc: Manish.Chopra@cavium.com
Cc: Jason Wang <jasowang@redhat.com>
Cc: Pravin Shelar <pshelar@ovn.org>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Daniel Axtens (2):
  net: create skb_gso_validate_mac_len()
  bnx2x: disable GSO where gso_size is too big for hardware

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  9 ++++
 include/linux/skbuff.h                           | 16 ++++++
 net/core/skbuff.c                                | 65 +++++++++++++++++++-----
 net/sched/sch_tbf.c                              | 10 ----
 4 files changed, 76 insertions(+), 24 deletions(-)

-- 
2.14.1

[PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[Daniel Axtens]

If you take a GSO skb, and split it into packets, will the MAC
length (L2 + L3 + L4 headers + payload) of those packets be small
enough to fit within a given length?

Move skb_gso_mac_seglen() to skbuff.h with other related functions
like skb_gso_network_seglen() so we can use it, and then create
skb_gso_validate_mac_len to do the full calculation.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 include/linux/skbuff.h | 16 +++++++++++++
 net/core/skbuff.c      | 65 +++++++++++++++++++++++++++++++++++++++-----------
 net/sched/sch_tbf.c    | 10 --------
 3 files changed, 67 insertions(+), 24 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b8e0da6c27d6..242d6773c7c2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3287,6 +3287,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
 void skb_scrub_packet(struct sk_buff *skb, bool xnet);
 unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
 bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
+bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
 struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
 struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
 int skb_ensure_writable(struct sk_buff *skb, int write_len);
@@ -4120,6 +4121,21 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
 	return hdr_len + skb_gso_transport_seglen(skb);
 }
 
+/**
+ * skb_gso_mac_seglen - Return length of individual segments of a gso packet
+ *
+ * @skb: GSO skb
+ *
+ * skb_gso_mac_seglen is used to determine the real size of the
+ * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
+ * headers (TCP/UDP).
+ */
+static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
+{
+	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
+	return hdr_len + skb_gso_transport_seglen(skb);
+}
+
 /* Local Checksum Offload.
  * Compute outer checksum based on the assumption that the
  * inner checksum will be offloaded later.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 01e8285aea73..55d84ab7d093 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4914,36 +4914,73 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
 EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
 
 /**
- * skb_gso_validate_mtu - Return in case such skb fits a given MTU
+ * skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
  *
- * @skb: GSO skb
- * @mtu: MTU to validate against
+ * There are a couple of instances where we have a GSO skb, and we
+ * want to determine what size it would be after it is segmented.
  *
- * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
- * once split.
+ * We might want to check:
+ * -    L3+L4+payload size (e.g. IP forwarding)
+ * - L2+L3+L4+payload size (e.g. sanity check before passing to driver)
+ *
+ * This is a helper to do that correctly considering GSO_BY_FRAGS.
+ *
+ * @seg_len: The segmented length (from skb_gso_*_seglen). In the
+ *           GSO_BY_FRAGS case this will be [header sizes + GSO_BY_FRAGS].
+ *
+ * @max_len: The maximum permissible length.
+ *
+ * Returns true if the segmented length <= max length.
  */
-bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
-{
+static inline bool skb_gso_size_check(const struct sk_buff *skb,
+				      unsigned int seg_len,
+				      unsigned int max_len) {
 	const struct skb_shared_info *shinfo = skb_shinfo(skb);
 	const struct sk_buff *iter;
-	unsigned int hlen;
-
-	hlen = skb_gso_network_seglen(skb);
 
 	if (shinfo->gso_size != GSO_BY_FRAGS)
-		return hlen <= mtu;
+		return seg_len <= max_len;
 
 	/* Undo this so we can re-use header sizes */
-	hlen -= GSO_BY_FRAGS;
+	seg_len -= GSO_BY_FRAGS;
 
 	skb_walk_frags(skb, iter) {
-		if (hlen + skb_headlen(iter) > mtu)
+		if (seg_len + skb_headlen(iter) > max_len)
 			return false;
 	}
 
 	return true;
 }
-EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
+
+/**
+ * skb_gso_validate_mtu - Return in case such skb fits a given MTU
+ *
+ * @skb: GSO skb
+ * @mtu: MTU to validate against
+ *
+ * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
+ * once split.
+ */
+bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
+{
+	return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
+}
+EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
+
+/**
+ * skb_gso_validate_mac_len - Will a split GSO skb fit in a given length?
+ *
+ * @skb: GSO skb
+ * @len: length to validate against
+ *
+ * skb_gso_validate_mac_len validates if a given skb will fit a wanted
+ * length once split, including L2, L3 and L4 headers and the payload.
+ */
+bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len)
+{
+	return skb_gso_size_check(skb, skb_gso_mac_seglen(skb), len);
+}
+EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
 
 static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 {
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 83e76d046993..229172d509cc 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -142,16 +142,6 @@ static u64 psched_ns_t2l(const struct psched_ratecfg *r,
 	return len;
 }
 
-/*
- * Return length of individual segments of a gso packet,
- * including all headers (MAC, IP, TCP/UDP)
- */
-static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
-{
-	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
-	return hdr_len + skb_gso_transport_seglen(skb);
-}
-
 /* GSO packet is too big, segment it so that tbf can transmit
  * each segment in time
  */
-- 
2.14.1

[PATCH v3 2/2] bnx2x: disable GSO where gso_size is too big for hardware

[Daniel Axtens]

If a bnx2x card is passed a GSO packet with a gso_size larger than
~9700 bytes, it will cause a firmware error that will bring the card
down:

bnx2x: [bnx2x_attn_int_deasserted3:4323(enP24p1s0f0)]MC assert!
bnx2x: [bnx2x_mc_assert:720(enP24p1s0f0)]XSTORM_ASSERT_LIST_INDEX 0x2
bnx2x: [bnx2x_mc_assert:736(enP24p1s0f0)]XSTORM_ASSERT_INDEX 0x0 = 0x00000000 0x25e43e47 0x00463e01 0x00010052
bnx2x: [bnx2x_mc_assert:750(enP24p1s0f0)]Chip Revision: everest3, FW Version: 7_13_1
... (dump of values continues) ...

Detect when the mac length of a GSO packet is greater than the maximum
packet size (9700 bytes) and disable GSO.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---

v3: use skb_gso_validate_mac_len to get the actual size, to allow
    jumbo frames to work.

I don't have local access to a bnx2x card and sending this off to the
user who does would add about a month of round-trip time, so this
particular spin is build-tested only. (Earlier spins were tested on
real hardware.)

---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 7b08323e3f3d..fbb08e360625 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12934,6 +12934,15 @@ static netdev_features_t bnx2x_features_check(struct sk_buff *skb,
 					      struct net_device *dev,
 					      netdev_features_t features)
 {
+	/*
+	 * A skb with gso_size + header length > 9700 will cause a
+	 * firmware panic. Drop GSO support.
+	 *
+	 * Eventually the upper layer should not pass these packets down.
+	 */
+	if (unlikely(skb_is_gso(skb) && !skb_gso_validate_mac_len(skb, 9700)))
+		features &= ~NETIF_F_GSO_MASK;
+
 	features = vlan_features_check(skb, features);
 	return vxlan_features_check(skb, features);
 }
-- 
2.14.1

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[Eric Dumazet]

On Tue, 2018-01-30 at 12:14 +1100, Daniel Axtens wrote:
> If you take a GSO skb, and split it into packets, will the MAC
> length (L2 + L3 + L4 headers + payload) of those packets be small
> enough to fit within a given length?
> 
> Move skb_gso_mac_seglen() to skbuff.h with other related functions
> like skb_gso_network_seglen() so we can use it, and then create
> skb_gso_validate_mac_len to do the full calculation.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  include/linux/skbuff.h | 16 +++++++++++++
>  net/core/skbuff.c      | 65 +++++++++++++++++++++++++++++++++++++++-----------
>  net/sched/sch_tbf.c    | 10 --------
>  3 files changed, 67 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index b8e0da6c27d6..242d6773c7c2 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3287,6 +3287,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
>  void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>  bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
> +bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>  int skb_ensure_writable(struct sk_buff *skb, int write_len);
> @@ -4120,6 +4121,21 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
>  	return hdr_len + skb_gso_transport_seglen(skb);
>  }
>  
> +/**
> + * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> + *
> + * @skb: GSO skb
> + *
> + * skb_gso_mac_seglen is used to determine the real size of the
> + * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> + * headers (TCP/UDP).
> + */
> +static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> +{
> +	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> +	return hdr_len + skb_gso_transport_seglen(skb);
> +}

skb_gso_transport_seglen(skb) is quite expensive (out of line)

It is unfortunate bnx2x seems to support 9600 MTU (
ETH_MAX_JUMBO_PACKET_SIZE ), because 100 bytes of headers can be too
small in some cases.

Presumably we could avoid calling the function for standard MTU <= 9000

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[Eric Dumazet]

On Mon, 2018-01-29 at 17:46 -0800, Eric Dumazet wrote:
> On Tue, 2018-01-30 at 12:14 +1100, Daniel Axtens wrote:
> > If you take a GSO skb, and split it into packets, will the MAC
> > length (L2 + L3 + L4 headers + payload) of those packets be small
> > enough to fit within a given length?
> > 
> > Move skb_gso_mac_seglen() to skbuff.h with other related functions
> > like skb_gso_network_seglen() so we can use it, and then create
> > skb_gso_validate_mac_len to do the full calculation.
> > 
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > ---
> >  include/linux/skbuff.h | 16 +++++++++++++
> >  net/core/skbuff.c      | 65 +++++++++++++++++++++++++++++++++++++++-----------
> >  net/sched/sch_tbf.c    | 10 --------
> >  3 files changed, 67 insertions(+), 24 deletions(-)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index b8e0da6c27d6..242d6773c7c2 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3287,6 +3287,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
> >  void skb_scrub_packet(struct sk_buff *skb, bool xnet);
> >  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
> >  bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
> > +bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
> >  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> >  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> >  int skb_ensure_writable(struct sk_buff *skb, int write_len);
> > @@ -4120,6 +4121,21 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
> >  	return hdr_len + skb_gso_transport_seglen(skb);
> >  }
> >  
> > +/**
> > + * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> > + *
> > + * @skb: GSO skb
> > + *
> > + * skb_gso_mac_seglen is used to determine the real size of the
> > + * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> > + * headers (TCP/UDP).
> > + */
> > +static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> > +{
> > +	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> > +	return hdr_len + skb_gso_transport_seglen(skb);
> > +}
> 
> skb_gso_transport_seglen(skb) is quite expensive (out of line)
> 
> It is unfortunate bnx2x seems to support 9600 MTU (
> ETH_MAX_JUMBO_PACKET_SIZE ), because 100 bytes of headers can be too
> small in some cases.
> 
> Presumably we could avoid calling the function for standard MTU <= 9000

Also are we sure about these bnx2x crashes being limited to TSO ?

Maybe it will crash the same after GSO has segmented the packet and we
provide a big (like 10,000 bytes) packet ? I do not believe upper stack
will prevent this.

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[Daniel Axtens]

Hi Eric,

>> skb_gso_transport_seglen(skb) is quite expensive (out of line)
>> 
>> It is unfortunate bnx2x seems to support 9600 MTU (
>> ETH_MAX_JUMBO_PACKET_SIZE ), because 100 bytes of headers can be too
>> small in some cases.
>> 
>> Presumably we could avoid calling the function for standard MTU <=
>> 9000

Yes, it is an expensive call. I will send another version where we only
call the expensive path in the jumbo frame case. I'll wait until
tomorrow in case there is any more feedback in the next 24 hours or so.

> Also are we sure about these bnx2x crashes being limited to TSO ?
>
> Maybe it will crash the same after GSO has segmented the packet and we
> provide a big (like 10,000 bytes) packet ? I do not believe upper stack
> will prevent this.

We did test with TSO off and that was sufficient to prevent the crash.

You are right that the upper stack sends down the 10k packet, so I don't
know why it prevents the crash. But we did definitely test it! I don't
have access to the firmware details but I imagine it's an issue with the
offloaded segmentation.

Thanks for your patience with the many versions of this patch set.

Regards,
Daniel

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[Marcelo Ricardo Leitner]

Hi,

On Tue, Jan 30, 2018 at 12:14:46PM +1100, Daniel Axtens wrote:
> If you take a GSO skb, and split it into packets, will the MAC
> length (L2 + L3 + L4 headers + payload) of those packets be small
> enough to fit within a given length?
> 
> Move skb_gso_mac_seglen() to skbuff.h with other related functions
> like skb_gso_network_seglen() so we can use it, and then create
> skb_gso_validate_mac_len to do the full calculation.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  include/linux/skbuff.h | 16 +++++++++++++
>  net/core/skbuff.c      | 65 +++++++++++++++++++++++++++++++++++++++-----------
>  net/sched/sch_tbf.c    | 10 --------
>  3 files changed, 67 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index b8e0da6c27d6..242d6773c7c2 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3287,6 +3287,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
>  void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>  bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
> +bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>  int skb_ensure_writable(struct sk_buff *skb, int write_len);
> @@ -4120,6 +4121,21 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
>  	return hdr_len + skb_gso_transport_seglen(skb);
>  }
>  
> +/**
> + * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> + *
> + * @skb: GSO skb
> + *
> + * skb_gso_mac_seglen is used to determine the real size of the
> + * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> + * headers (TCP/UDP).
> + */
> +static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> +{
> +	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> +	return hdr_len + skb_gso_transport_seglen(skb);
> +}
> +
>  /* Local Checksum Offload.
>   * Compute outer checksum based on the assumption that the
>   * inner checksum will be offloaded later.
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 01e8285aea73..55d84ab7d093 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4914,36 +4914,73 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
>  EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
>  
>  /**
> - * skb_gso_validate_mtu - Return in case such skb fits a given MTU
> + * skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
>   *
> - * @skb: GSO skb
> - * @mtu: MTU to validate against
> + * There are a couple of instances where we have a GSO skb, and we
> + * want to determine what size it would be after it is segmented.
>   *
> - * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
> - * once split.
> + * We might want to check:
> + * -    L3+L4+payload size (e.g. IP forwarding)
> + * - L2+L3+L4+payload size (e.g. sanity check before passing to driver)
> + *
> + * This is a helper to do that correctly considering GSO_BY_FRAGS.
> + *
> + * @seg_len: The segmented length (from skb_gso_*_seglen). In the
> + *           GSO_BY_FRAGS case this will be [header sizes + GSO_BY_FRAGS].
> + *
> + * @max_len: The maximum permissible length.
> + *
> + * Returns true if the segmented length <= max length.
>   */
> -bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
> -{
> +static inline bool skb_gso_size_check(const struct sk_buff *skb,
> +				      unsigned int seg_len,
> +				      unsigned int max_len) {
>  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
>  	const struct sk_buff *iter;
> -	unsigned int hlen;
> -
> -	hlen = skb_gso_network_seglen(skb);
>  
>  	if (shinfo->gso_size != GSO_BY_FRAGS)
> -		return hlen <= mtu;
> +		return seg_len <= max_len;
>  
>  	/* Undo this so we can re-use header sizes */
> -	hlen -= GSO_BY_FRAGS;
> +	seg_len -= GSO_BY_FRAGS;
>  
>  	skb_walk_frags(skb, iter) {
> -		if (hlen + skb_headlen(iter) > mtu)
> +		if (seg_len + skb_headlen(iter) > max_len)
>  			return false;
>  	}
>  
>  	return true;
>  }
> -EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
> +
> +/**
> + * skb_gso_validate_mtu - Return in case such skb fits a given MTU
> + *
> + * @skb: GSO skb
> + * @mtu: MTU to validate against
> + *
> + * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
> + * once split.
> + */
> +bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
> +{
> +	return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
> +}
> +EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);

This export is not matching the function name.

Re: [PATCH v3 0/2] bnx2x: disable GSO on too-large packets

[Marcelo Ricardo Leitner]

On Tue, Jan 30, 2018 at 12:14:45PM +1100, Daniel Axtens wrote:
> 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].
> 
> Ultimately we want a fix in the core, but that is very tricky to
> backport. So for now, just stop the bnx2x driver from crashing.
> 
> When net-next re-opens I will send the fix to the core and a revert
> for this.
> 
> Marcelo: I have left renaming skb_gso_validate_mtu() for the
> next series.

Alright! Just need to sync the EXPORT_ in there.
(I have no further comments, LGTM)

> 
> Thanks,
> Daniel
>    
> [0]: https://patchwork.ozlabs.org/patch/859410/
> 
> Cc: Manish.Chopra@cavium.com
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Pravin Shelar <pshelar@ovn.org>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> Daniel Axtens (2):
>   net: create skb_gso_validate_mac_len()
>   bnx2x: disable GSO where gso_size is too big for hardware
> 
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  9 ++++
>  include/linux/skbuff.h                           | 16 ++++++
>  net/core/skbuff.c                                | 65 +++++++++++++++++++-----
>  net/sched/sch_tbf.c                              | 10 ----
>  4 files changed, 76 insertions(+), 24 deletions(-)
> 
> -- 
> 2.14.1
> 

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[Daniel Axtens]

Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> writes:

> Hi,
>
> On Tue, Jan 30, 2018 at 12:14:46PM +1100, Daniel Axtens wrote:
>> If you take a GSO skb, and split it into packets, will the MAC
>> length (L2 + L3 + L4 headers + payload) of those packets be small
>> enough to fit within a given length?
>> 
>> Move skb_gso_mac_seglen() to skbuff.h with other related functions
>> like skb_gso_network_seglen() so we can use it, and then create
>> skb_gso_validate_mac_len to do the full calculation.
>> 
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> ---
>>  include/linux/skbuff.h | 16 +++++++++++++
>>  net/core/skbuff.c      | 65 +++++++++++++++++++++++++++++++++++++++-----------
>>  net/sched/sch_tbf.c    | 10 --------
>>  3 files changed, 67 insertions(+), 24 deletions(-)
>> 
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index b8e0da6c27d6..242d6773c7c2 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -3287,6 +3287,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
>>  void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>>  bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
>> +bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
>>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>>  int skb_ensure_writable(struct sk_buff *skb, int write_len);
>> @@ -4120,6 +4121,21 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
>>  	return hdr_len + skb_gso_transport_seglen(skb);
>>  }
>>  
>> +/**
>> + * skb_gso_mac_seglen - Return length of individual segments of a gso packet
>> + *
>> + * @skb: GSO skb
>> + *
>> + * skb_gso_mac_seglen is used to determine the real size of the
>> + * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
>> + * headers (TCP/UDP).
>> + */
>> +static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
>> +{
>> +	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
>> +	return hdr_len + skb_gso_transport_seglen(skb);
>> +}
>> +
>>  /* Local Checksum Offload.
>>   * Compute outer checksum based on the assumption that the
>>   * inner checksum will be offloaded later.
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 01e8285aea73..55d84ab7d093 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -4914,36 +4914,73 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
>>  EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
>>  
>>  /**
>> - * skb_gso_validate_mtu - Return in case such skb fits a given MTU
>> + * skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
>>   *
>> - * @skb: GSO skb
>> - * @mtu: MTU to validate against
>> + * There are a couple of instances where we have a GSO skb, and we
>> + * want to determine what size it would be after it is segmented.
>>   *
>> - * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
>> - * once split.
>> + * We might want to check:
>> + * -    L3+L4+payload size (e.g. IP forwarding)
>> + * - L2+L3+L4+payload size (e.g. sanity check before passing to driver)
>> + *
>> + * This is a helper to do that correctly considering GSO_BY_FRAGS.
>> + *
>> + * @seg_len: The segmented length (from skb_gso_*_seglen). In the
>> + *           GSO_BY_FRAGS case this will be [header sizes + GSO_BY_FRAGS].
>> + *
>> + * @max_len: The maximum permissible length.
>> + *
>> + * Returns true if the segmented length <= max length.
>>   */
>> -bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
>> -{
>> +static inline bool skb_gso_size_check(const struct sk_buff *skb,
>> +				      unsigned int seg_len,
>> +				      unsigned int max_len) {
>>  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
>>  	const struct sk_buff *iter;
>> -	unsigned int hlen;
>> -
>> -	hlen = skb_gso_network_seglen(skb);
>>  
>>  	if (shinfo->gso_size != GSO_BY_FRAGS)
>> -		return hlen <= mtu;
>> +		return seg_len <= max_len;
>>  
>>  	/* Undo this so we can re-use header sizes */
>> -	hlen -= GSO_BY_FRAGS;
>> +	seg_len -= GSO_BY_FRAGS;
>>  
>>  	skb_walk_frags(skb, iter) {
>> -		if (hlen + skb_headlen(iter) > mtu)
>> +		if (seg_len + skb_headlen(iter) > max_len)
>>  			return false;
>>  	}
>>  
>>  	return true;
>>  }
>> -EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
>> +
>> +/**
>> + * skb_gso_validate_mtu - Return in case such skb fits a given MTU
>> + *
>> + * @skb: GSO skb
>> + * @mtu: MTU to validate against
>> + *
>> + * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
>> + * once split.
>> + */
>> +bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
>> +{
>> +	return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
>> +}
>> +EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
>
> This export is not matching the function name.
Oh darn it! I fixed this locally and then forgot to run
format-patch. Will fix it in the next spin.

Thanks!

Regards,
Daniel

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

Hi Daniel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.15 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Axtens/bnx2x-disable-GSO-on-too-large-packets/20180131-000934
config: i386-randconfig-a1-201804 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from include/linux/linkage.h:7:0,
                    from include/linux/kernel.h:7,
                    from include/linux/list.h:9,
                    from include/linux/module.h:9,
                    from net/core/skbuff.c:41:
>> net/core/skbuff.c:4968:19: error: 'skb_gso_validate_network_len' undeclared here (not in a function)
    EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
                      ^
   include/linux/export.h:65:16: note: in definition of macro '___EXPORT_SYMBOL'
     extern typeof(sym) sym;      \
                   ^
   net/core/skbuff.c:4968:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
    ^

vim +/skb_gso_validate_network_len +4968 net/core/skbuff.c

  4954	
  4955	/**
  4956	 * skb_gso_validate_mtu - Return in case such skb fits a given MTU
  4957	 *
  4958	 * @skb: GSO skb
  4959	 * @mtu: MTU to validate against
  4960	 *
  4961	 * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
  4962	 * once split.
  4963	 */
  4964	bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
  4965	{
  4966		return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
  4967	}
> 4968	EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
  4969	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29908 bytes --]

Re: [PATCH v3 1/2] net: create skb_gso_validate_mac_len()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

Hi Daniel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.15 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Axtens/bnx2x-disable-GSO-on-too-large-packets/20180131-000934
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   In file included from include/linux/linkage.h:7:0,
                    from include/linux/kernel.h:7,
                    from include/linux/list.h:9,
                    from include/linux/module.h:9,
                    from net//core/skbuff.c:41:
>> net//core/skbuff.c:4968:19: error: 'skb_gso_validate_network_len' undeclared here (not in a function); did you mean 'skb_gso_validate_mac_len'?
    EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
                      ^
   include/linux/export.h:65:16: note: in definition of macro '___EXPORT_SYMBOL'
     extern typeof(sym) sym;      \
                   ^~~
   net//core/skbuff.c:4968:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
    ^~~~~~~~~~~~~~~~~

vim +4968 net//core/skbuff.c

  4954	
  4955	/**
  4956	 * skb_gso_validate_mtu - Return in case such skb fits a given MTU
  4957	 *
  4958	 * @skb: GSO skb
  4959	 * @mtu: MTU to validate against
  4960	 *
  4961	 * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
  4962	 * once split.
  4963	 */
  4964	bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
  4965	{
  4966		return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
  4967	}
> 4968	EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
  4969	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49818 bytes --]