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

[PATCH v4 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.

v4 changes:
  - fix compilation error with EXPORTs (patch 1)
  - only do slow test if gso_size is greater than 9000 bytes (patch 2)

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

Cc: Eric Dumazet <eric.dumazet@gmail.com>
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 | 18 +++++++
 include/linux/skbuff.h                           | 16 ++++++
 net/core/skbuff.c                                | 63 +++++++++++++++++++-----
 net/sched/sch_tbf.c                              | 10 ----
 4 files changed, 84 insertions(+), 23 deletions(-)

-- 
2.14.1

[PATCH v4 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>

---

v4: fix mistake in EXPORT name
---
 include/linux/skbuff.h | 16 +++++++++++++
 net/core/skbuff.c      | 63 +++++++++++++++++++++++++++++++++++++++-----------
 net/sched/sch_tbf.c    | 10 --------
 3 files changed, 66 insertions(+), 23 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..8c61c27c1b28 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4914,37 +4914,74 @@ 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;
 }
+
+/**
+ * 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_mtu);
 
+/**
+ * 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)
 {
 	if (skb_cow(skb, skb_headroom(skb)) < 0) {
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 v4 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>

---

v4: Only call the slow check if the gso_size is large.
    Eric - I think this is what you had in mind?
    Manish - do you think this is an acceptable performance trade-off?
             GSO will work for any packet size, and only jumbo frames will
	     have to do the slower test.

Again, only build-tested.
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 7b08323e3f3d..74fc9af4aadb 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12934,6 +12934,24 @@ 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.
+	 *
+	 * For speed, if the gso_size is <= 9000, assume there will
+	 * not be 700 bytes of headers and pass it through. Only do a
+	 * full (slow) validation if the gso_size is > 9000.
+	 *
+	 * (Due to the way SKB_BY_FRAGS works this will also do a full
+	 * validation in that case.)
+	 */
+	if (unlikely(skb_is_gso(skb) &&
+		     (skb_shinfo(skb)->gso_size > 9000) &&
+		     !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 v4 2/2] bnx2x: disable GSO where gso_size is too big for hardware

[Chopra, Manish]

> -----Original Message-----
> From: Daniel Axtens [mailto:dja@axtens.net]
> Sent: Wednesday, January 31, 2018 8:46 AM
> To: netdev@vger.kernel.org
> Cc: Daniel Axtens <dja@axtens.net>; Eric Dumazet <eric.dumazet@gmail.com>;
> Chopra, Manish <Manish.Chopra@cavium.com>; Jason Wang
> <jasowang@redhat.com>; Pravin Shelar <pshelar@ovn.org>; Marcelo Ricardo
> Leitner <marcelo.leitner@gmail.com>
> Subject: [PATCH v4 2/2] bnx2x: disable GSO where gso_size is too big for
> hardware
> 
> 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>
> 
> ---
> 
> v4: Only call the slow check if the gso_size is large.
>     Eric - I think this is what you had in mind?
>     Manish - do you think this is an acceptable performance trade-off?
>              GSO will work for any packet size, and only jumbo frames will
> 	     have to do the slower test.
> 
> Again, only build-tested.
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 18
> ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 7b08323e3f3d..74fc9af4aadb 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12934,6 +12934,24 @@ 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.
> +	 *
> +	 * For speed, if the gso_size is <= 9000, assume there will
> +	 * not be 700 bytes of headers and pass it through. Only do a
> +	 * full (slow) validation if the gso_size is > 9000.
> +	 *
> +	 * (Due to the way SKB_BY_FRAGS works this will also do a full
> +	 * validation in that case.)
> +	 */
> +	if (unlikely(skb_is_gso(skb) &&
> +		     (skb_shinfo(skb)->gso_size > 9000) &&
> +		     !skb_gso_validate_mac_len(skb, 9700)))
> +		features &= ~NETIF_F_GSO_MASK;

Hi Daniel,

Obviously, it could be bad from performance perspective since every gso packet has to do these check.
When running iperf/netperf performance benchmark, where GSO is likely to occur.

Why do you have to put two checks for skb_is_gso() and gso_size ? Isn't gso_size > anything means GSO skb ?
I assume it won't cause disabling the offload if "headers [L2 + L3 + L4] + gso_size" is still <= 9700. ?

Thanks,
Manish

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

[Daniel Axtens]

"Chopra, Manish" <Manish.Chopra@cavium.com> writes:

>> -----Original Message-----
>> From: Daniel Axtens [mailto:dja@axtens.net]
>> Sent: Wednesday, January 31, 2018 8:46 AM
>> To: netdev@vger.kernel.org
>> Cc: Daniel Axtens <dja@axtens.net>; Eric Dumazet <eric.dumazet@gmail.com>;
>> Chopra, Manish <Manish.Chopra@cavium.com>; Jason Wang
>> <jasowang@redhat.com>; Pravin Shelar <pshelar@ovn.org>; Marcelo Ricardo
>> Leitner <marcelo.leitner@gmail.com>
>> Subject: [PATCH v4 2/2] bnx2x: disable GSO where gso_size is too big for
>> hardware
>> 
>> 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>
>> 
>> ---
>> 
>> v4: Only call the slow check if the gso_size is large.
>>     Eric - I think this is what you had in mind?
>>     Manish - do you think this is an acceptable performance trade-off?
>>              GSO will work for any packet size, and only jumbo frames will
>> 	     have to do the slower test.
>> 
>> Again, only build-tested.
>> ---
>>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 18
>> ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> index 7b08323e3f3d..74fc9af4aadb 100644
>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> @@ -12934,6 +12934,24 @@ 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.
>> +	 *
>> +	 * For speed, if the gso_size is <= 9000, assume there will
>> +	 * not be 700 bytes of headers and pass it through. Only do a
>> +	 * full (slow) validation if the gso_size is > 9000.
>> +	 *
>> +	 * (Due to the way SKB_BY_FRAGS works this will also do a full
>> +	 * validation in that case.)
>> +	 */
>> +	if (unlikely(skb_is_gso(skb) &&
>> +		     (skb_shinfo(skb)->gso_size > 9000) &&
>> +		     !skb_gso_validate_mac_len(skb, 9700)))
>> +		features &= ~NETIF_F_GSO_MASK;
>
> Hi Daniel,
>
> Obviously, it could be bad from performance perspective since every gso packet has to do these check.
> When running iperf/netperf performance benchmark, where GSO is likely to occur.
>
> Why do you have to put two checks for skb_is_gso() and gso_size ? Isn't gso_size > anything means GSO skb ?

Yes, the check is redundant. I do it to make my logic clearer.

The compiler will optimise it into one check. I compiled with gcc-7.2
and gcc-4.8, both targeting amd64, and in both cases I could only find
one relevant cmp:

skb_is_gso():
/home/dja/dev/linux/linux/include/linux/skbuff.h:4032
    3686:       48 8b 8f d0 00 00 00    mov    0xd0(%rdi),%rcx
bnx2x_features_check():
/home/dja/dev/linux/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12950
    368d:       66 81 7c 01 04 28 23    cmpw   $0x2328,0x4(%rcx,%rax,1)
    3694:       0f 87 ea 01 00 00       ja     3884 <bnx2x_features_check+0x214>

0x2328 is decimal 9000.

> I assume it won't cause disabling the offload if "headers [L2 + L3 + L4] + gso_size" is still <= 9700. ?

That is correct. The flow is:

If the gso_size is less than or equal to 9000, offload is enabled with
no further checks. This is the fast path.

If the gso_size is greater than 9000, then the "headers [L2 + L3 + L4] +
gso_size" is checked. This is an out-of-line check done once per GSO skb.

If headers + gso_size is <= 9700, offload is enabled.

If headers + gso_size > 9700, offload is disabled.

Regards,
Daniel
>
> Thanks,
> Manish

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

[Eric Dumazet]

On Wed, 2018-01-31 at 14:15 +1100, Daniel Axtens wrote:
> 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>
> 
> ---
> 
> v4: Only call the slow check if the gso_size is large.
>     Eric - I think this is what you had in mind?
>     Manish - do you think this is an acceptable performance trade-off?
>              GSO will work for any packet size, and only jumbo frames will
> 	     have to do the slower test.

Yes this looks good to me, thanks !

Reviewed-by: Eric Dumazet <edumazet@google.com>

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

[David Miller]

From: Daniel Axtens <dja@axtens.net>
Date: Wed, 31 Jan 2018 14:15:32 +1100

> 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.
> 
> v4 changes:
>   - fix compilation error with EXPORTs (patch 1)
>   - only do slow test if gso_size is greater than 9000 bytes (patch 2)
 ...
> [0]: https://patchwork.ozlabs.org/patch/859410/

Series applied, thanks Daniel.