[PATCH 0/3] Check gso_size of packets when forwarding

[PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

When regular packets are forwarded, we validate their size against the
MTU of the destination device. However, when GSO packets are
forwarded, we do not validate their size against the MTU. 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 it in
the driver, this series fixes the forwarding path.

To fix this:

 - Move a helper in patch 1.

 - Validate GSO segment lengths in is_skb_forwardable() in the GSO
   case, rather than assuming all will be well. This fixes bridges.
   This is patch 2.

 - Open vSwitch uses its own slightly specialised algorithm for
   checking lengths. Wire up checking for that in patch 3.

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

Cc: Manish.Chopra-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org

Daniel Axtens (3):
  net: move skb_gso_mac_seglen to skbuff.h
  net: is_skb_forwardable: validate length of GSO packet segments
  openvswitch: drop GSO packets that are too large

 include/linux/skbuff.h  | 16 ++++++++++++++++
 net/core/dev.c          |  7 ++++---
 net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
 net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
 net/sched/sch_tbf.c     | 10 ----------
 5 files changed, 84 insertions(+), 20 deletions(-)

-- 
2.14.1

[PATCH 1/3] net: move skb_gso_mac_seglen to skbuff.h

[Daniel Axtens]

We're about to use this elsewhere, so move it into the header with
the other related functions like skb_gso_network_seglen().

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

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b8e0da6c27d6..b8acafd73272 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4120,6 +4120,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/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 2/3] net: is_skb_forwardable: validate length of GSO packet segments

[Daniel Axtens]

is_skb_forwardable attempts to detect if a packet is too large to
be sent to the destination device. However, this test does not
consider GSO packets, and it is possible that a GSO packet, when
resegmented, will be larger than the device can transmit.

Add detection for packets which will be too large by creating a
skb_gso_validate_len() routine which is similar to
skb_gso_validate_mtu(), but which considers L2 headers, and wire
it up in is_skb_forwardable().

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 include/linux/skbuff.h |  1 +
 net/core/dev.c         |  7 ++++---
 net/core/skbuff.c      | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b8acafd73272..6a9e4b6f80a7 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_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);
diff --git a/net/core/dev.c b/net/core/dev.c
index 94435cd09072..5af04be128c1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1841,11 +1841,12 @@ bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
 	if (skb->len <= len)
 		return true;
 
-	/* if TSO is enabled, we don't care about the length as the packet
-	 * could be forwarded without being segmented before
+	/*
+	 * if TSO is enabled, we need to check the size of the
+	 * segmented packets
 	 */
 	if (skb_is_gso(skb))
-		return true;
+		return skb_gso_validate_len(skb, len);
 
 	return false;
 }
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 01e8285aea73..aefe049e4b0c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4945,6 +4945,40 @@ bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
 }
 EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
 
+/**
+ * skb_gso_validate_len - Will a split GSO skb fit in a given length?
+ *
+ * @skb: GSO skb
+ * @len: length to validate against
+ *
+ * skb_gso_validate_len validates if a given skb will fit a wanted
+ * length once split, including L2, L3 and L4 headers.
+ *
+ * Similar to skb_gso_validate_mtu, but inclusive of L2 headers.
+ */
+bool skb_gso_validate_len(const struct sk_buff *skb, unsigned int len)
+{
+	const struct skb_shared_info *shinfo = skb_shinfo(skb);
+	const struct sk_buff *iter;
+	unsigned int hlen;
+
+	hlen = skb_gso_mac_seglen(skb);
+
+	if (shinfo->gso_size != GSO_BY_FRAGS)
+		return hlen <= len;
+
+	/* Undo this so we can re-use header sizes */
+	hlen -= GSO_BY_FRAGS;
+
+	skb_walk_frags(skb, iter) {
+		if (hlen + skb_headlen(iter) > len)
+			return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(skb_gso_validate_len);
+
 static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 {
 	if (skb_cow(skb, skb_headroom(skb)) < 0) {
-- 
2.14.1

[PATCH 3/3] openvswitch: drop GSO packets that are too large

[Daniel Axtens]

Open vSwitch attempts to detect if a packet is too large to be
sent to the destination device. However, this test does not
consider GSO packets, and it is possible that a GSO packet, when
resegmented, will be larger than the device can send.

Add detection for packets which are too large. We use
skb_gso_validate_len, reusing the length calculation in the existing
checks - see 738314a084aa
("openvswitch: use hard_header_len instead of hardcoded ETH_HLEN")
This is different from the is_skb_forwardable logic in that it only
allows for the length of a VLAN tag if one is actually present.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index b6c8524032a0..290eeaa82344 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -464,16 +464,16 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
 	return 0;
 }
 
-static unsigned int packet_length(const struct sk_buff *skb,
-				  struct net_device *dev)
+static unsigned int packet_length_offset(const struct sk_buff *skb,
+					 const struct net_device *dev)
 {
-	unsigned int length = skb->len - dev->hard_header_len;
+	unsigned int length = dev->hard_header_len;
 
 	if (!skb_vlan_tag_present(skb) &&
 	    eth_type_vlan(skb->protocol))
-		length -= VLAN_HLEN;
+		length += VLAN_HLEN;
 
-	/* Don't subtract for multiple VLAN tags. Most (all?) drivers allow
+	/* Don't adjust for multiple VLAN tags. Most (all?) drivers allow
 	 * (ETH_LEN + VLAN_HLEN) in addition to the mtu value, but almost none
 	 * account for 802.1ad. e.g. is_skb_forwardable().
 	 */
@@ -481,6 +481,21 @@ static unsigned int packet_length(const struct sk_buff *skb,
 	return length;
 }
 
+static inline unsigned int packet_length(const struct sk_buff *skb,
+					 const struct net_device *dev)
+{
+	return skb->len - packet_length_offset(skb, dev);
+}
+
+static inline bool vport_gso_validate_len(const struct sk_buff *skb,
+					  const struct net_device *dev,
+					  unsigned int mtu)
+{
+	unsigned int len = mtu + packet_length_offset(skb, dev);
+	
+	return skb_gso_validate_len(skb, len);
+}
+
 void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
 {
 	int mtu = vport->dev->mtu;
@@ -504,13 +519,21 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
 		goto drop;
 	}
 
-	if (unlikely(packet_length(skb, vport->dev) > mtu &&
-		     !skb_is_gso(skb))) {
+	if (!skb_is_gso(skb) &&
+	    unlikely(packet_length(skb, vport->dev) > mtu)) {
 		net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n",
 				     vport->dev->name,
 				     packet_length(skb, vport->dev), mtu);
 		vport->dev->stats.tx_errors++;
 		goto drop;
+	} else if (skb_is_gso(skb) &&
+		   unlikely(!vport_gso_validate_len(skb, vport->dev, mtu))) {
+		net_warn_ratelimited("%s: dropped over-mtu GSO packet: "
+				     "gso_size = %d, mtu = %d\n",
+				     vport->dev->name,
+				     skb_shinfo(skb)->gso_size, mtu);
+		vport->dev->stats.tx_errors++;
+		goto drop;
 	}
 
 	skb->dev = vport->dev;
-- 
2.14.1

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[David Miller]

From: Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org>
Date: Tue, 16 Jan 2018 13:09:17 +1100

> When regular packets are forwarded, we validate their size against the
> MTU of the destination device. However, when GSO packets are
> forwarded, we do not validate their size against the MTU. 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 it in
> the driver, this series fixes the forwarding path.
> 
> To fix this:
> 
>  - Move a helper in patch 1.
> 
>  - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>    case, rather than assuming all will be well. This fixes bridges.
>    This is patch 2.
> 
>  - Open vSwitch uses its own slightly specialised algorithm for
>    checking lengths. Wire up checking for that in patch 3.
> 
> [0]: https://patchwork.ozlabs.org/patch/859410/

This looks good to me, could the OVS folks please review this patch
series?

Thank you.

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Pravin Shelar]

On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
> When regular packets are forwarded, we validate their size against the
> MTU of the destination device. However, when GSO packets are
> forwarded, we do not validate their size against the MTU. 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 it in
> the driver, this series fixes the forwarding path.
>
Are there any other possible forwarding path in networking stack? TC
is one subsystem that could forward such a packet to the bnx2x device,
how is that handled ?

> To fix this:
>
>  - Move a helper in patch 1.
>
>  - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>    case, rather than assuming all will be well. This fixes bridges.
>    This is patch 2.
>
>  - Open vSwitch uses its own slightly specialised algorithm for
>    checking lengths. Wire up checking for that in patch 3.
>
> [0]: https://patchwork.ozlabs.org/patch/859410/
>
> Cc: Manish.Chopra@cavium.com
> Cc: dev@openvswitch.org
>
> Daniel Axtens (3):
>   net: move skb_gso_mac_seglen to skbuff.h
>   net: is_skb_forwardable: validate length of GSO packet segments
>   openvswitch: drop GSO packets that are too large
>
>  include/linux/skbuff.h  | 16 ++++++++++++++++
>  net/core/dev.c          |  7 ++++---
>  net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
>  net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
>  net/sched/sch_tbf.c     | 10 ----------
>  5 files changed, 84 insertions(+), 20 deletions(-)
>
> --
> 2.14.1
>

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Jason Wang]

On 2018年01月18日 16:28, Pravin Shelar wrote:
> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
>> When regular packets are forwarded, we validate their size against the
>> MTU of the destination device. However, when GSO packets are
>> forwarded, we do not validate their size against the MTU. 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 it in
>> the driver, this series fixes the forwarding path.
>>
> Are there any other possible forwarding path in networking stack? TC
> is one subsystem that could forward such a packet to the bnx2x device,
> how is that handled ?

Yes, so it looks to me we should do the check in e.g netif_needs_gso() 
before passing it to hardware. And bnx2x needs to set its gso_max_size 
correctly.

Btw, looks like this could be triggered from a guest which is a DOS. We 
need request a CVE for this.

Thanks

>
>> To fix this:
>>
>>   - Move a helper in patch 1.
>>
>>   - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>>     case, rather than assuming all will be well. This fixes bridges.
>>     This is patch 2.
>>
>>   - Open vSwitch uses its own slightly specialised algorithm for
>>     checking lengths. Wire up checking for that in patch 3.
>>
>> [0]: https://patchwork.ozlabs.org/patch/859410/
>>
>> Cc: Manish.Chopra@cavium.com
>> Cc: dev@openvswitch.org
>>
>> Daniel Axtens (3):
>>    net: move skb_gso_mac_seglen to skbuff.h
>>    net: is_skb_forwardable: validate length of GSO packet segments
>>    openvswitch: drop GSO packets that are too large
>>
>>   include/linux/skbuff.h  | 16 ++++++++++++++++
>>   net/core/dev.c          |  7 ++++---
>>   net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
>>   net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
>>   net/sched/sch_tbf.c     | 10 ----------
>>   5 files changed, 84 insertions(+), 20 deletions(-)
>>
>> --
>> 2.14.1
>>

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:

> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>> When regular packets are forwarded, we validate their size against the
>> MTU of the destination device. However, when GSO packets are
>> forwarded, we do not validate their size against the MTU. 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 it in
>> the driver, this series fixes the forwarding path.
>>
> Are there any other possible forwarding path in networking stack? TC
> is one subsystem that could forward such a packet to the bnx2x device,
> how is that handled ?

So far I have only looked at bridges, openvswitch and macvlan. In
general, if the code uses dev_forward_skb() it should automatically be
fine as that invokes is_skb_forwardable(), which we patch.

There is potentially a forwarding path in macvlan that doesn't pass
through dev_forward_skb() (which calls is_skb_forwardable). I did post a
patch to this earlier, but it got somewhat derailed by how you would get
such a packet in to a macvlan device and whether that should be fixed
instead. Once we have some consensus on Jason's questions and the
overall approach is solid, I'm happy to work on that again.

To answer your specific question, I'm not aware of how tc can cause a
packet to be forwarded - if you can point me to the right general area I
can have a look.

Regards,
Daniel

>
>> To fix this:
>>
>>  - Move a helper in patch 1.
>>
>>  - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>>    case, rather than assuming all will be well. This fixes bridges.
>>    This is patch 2.
>>
>>  - Open vSwitch uses its own slightly specialised algorithm for
>>    checking lengths. Wire up checking for that in patch 3.
>>
>> [0]: https://patchwork.ozlabs.org/patch/859410/
>>
>> Cc: Manish.Chopra-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org
>> Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org
>>
>> Daniel Axtens (3):
>>   net: move skb_gso_mac_seglen to skbuff.h
>>   net: is_skb_forwardable: validate length of GSO packet segments
>>   openvswitch: drop GSO packets that are too large
>>
>>  include/linux/skbuff.h  | 16 ++++++++++++++++
>>  net/core/dev.c          |  7 ++++---
>>  net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
>>  net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
>>  net/sched/sch_tbf.c     | 10 ----------
>>  5 files changed, 84 insertions(+), 20 deletions(-)
>>
>> --
>> 2.14.1
>>

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

Jason Wang <jasowang@redhat.com> writes:

> On 2018年01月18日 16:28, Pravin Shelar wrote:
>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
>>> When regular packets are forwarded, we validate their size against the
>>> MTU of the destination device. However, when GSO packets are
>>> forwarded, we do not validate their size against the MTU. 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 it in
>>> the driver, this series fixes the forwarding path.
>>>
>> Are there any other possible forwarding path in networking stack? TC
>> is one subsystem that could forward such a packet to the bnx2x device,
>> how is that handled ?
>
> Yes, so it looks to me we should do the check in e.g netif_needs_gso() 
> before passing it to hardware. And bnx2x needs to set its gso_max_size 
> correctly.

I don't think gso_max_size is quite the same. If I understand
net/ipv4/tcp.c correctly, gso_max_size is supposed to cap the total
length of the skb, not the length of each segmented packet. The problem
for the bnx2x card is the length of the segment, not the overall length.

>
> Btw, looks like this could be triggered from a guest which is a DOS. We 
> need request a CVE for this.
>

You are correct about how this can be triggered: in fact it came to my
attention because a network packet from one LPAR (PowerVM virtual
machine) brought down the card attached to a different LPAR. It didn't
occur to me that it was potentially a security issue. I am talking with
the security team at Canonical regarding this.

Regards,
Daniel

> Thanks
>
>>
>>> To fix this:
>>>
>>>   - Move a helper in patch 1.
>>>
>>>   - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>>>     case, rather than assuming all will be well. This fixes bridges.
>>>     This is patch 2.
>>>
>>>   - Open vSwitch uses its own slightly specialised algorithm for
>>>     checking lengths. Wire up checking for that in patch 3.
>>>
>>> [0]: https://patchwork.ozlabs.org/patch/859410/
>>>
>>> Cc: Manish.Chopra@cavium.com
>>> Cc: dev@openvswitch.org
>>>
>>> Daniel Axtens (3):
>>>    net: move skb_gso_mac_seglen to skbuff.h
>>>    net: is_skb_forwardable: validate length of GSO packet segments
>>>    openvswitch: drop GSO packets that are too large
>>>
>>>   include/linux/skbuff.h  | 16 ++++++++++++++++
>>>   net/core/dev.c          |  7 ++++---
>>>   net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
>>>   net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
>>>   net/sched/sch_tbf.c     | 10 ----------
>>>   5 files changed, 84 insertions(+), 20 deletions(-)
>>>
>>> --
>>> 2.14.1
>>>

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

Daniel Axtens <dja@axtens.net> writes:

> Jason Wang <jasowang@redhat.com> writes:
>
>> On 2018年01月18日 16:28, Pravin Shelar wrote:
>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
>>>> When regular packets are forwarded, we validate their size against the
>>>> MTU of the destination device. However, when GSO packets are
>>>> forwarded, we do not validate their size against the MTU. 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 it in
>>>> the driver, this series fixes the forwarding path.
>>>>
>>> Are there any other possible forwarding path in networking stack? TC
>>> is one subsystem that could forward such a packet to the bnx2x device,
>>> how is that handled ?
>>
>> Yes, so it looks to me we should do the check in e.g netif_needs_gso() 
>> before passing it to hardware. And bnx2x needs to set its gso_max_size 
>> correctly.
>
> I don't think gso_max_size is quite the same. If I understand
> net/ipv4/tcp.c correctly, gso_max_size is supposed to cap the total
> length of the skb, not the length of each segmented packet. The problem
> for the bnx2x card is the length of the segment, not the overall length.
>
>>
>> Btw, looks like this could be triggered from a guest which is a DOS. We 
>> need request a CVE for this.
>>
>
> You are correct about how this can be triggered: in fact it came to my
> attention because a network packet from one LPAR (PowerVM virtual
> machine) brought down the card attached to a different LPAR. It didn't
> occur to me that it was potentially a security issue. I am talking with
> the security team at Canonical regarding this.

I have requested a CVE from the Distributed Weakness Filing.

Regards,
Daniel

>
> Regards,
> Daniel
>
>> Thanks
>>
>>>
>>>> To fix this:
>>>>
>>>>   - Move a helper in patch 1.
>>>>
>>>>   - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>>>>     case, rather than assuming all will be well. This fixes bridges.
>>>>     This is patch 2.
>>>>
>>>>   - Open vSwitch uses its own slightly specialised algorithm for
>>>>     checking lengths. Wire up checking for that in patch 3.
>>>>
>>>> [0]: https://patchwork.ozlabs.org/patch/859410/
>>>>
>>>> Cc: Manish.Chopra@cavium.com
>>>> Cc: dev@openvswitch.org
>>>>
>>>> Daniel Axtens (3):
>>>>    net: move skb_gso_mac_seglen to skbuff.h
>>>>    net: is_skb_forwardable: validate length of GSO packet segments
>>>>    openvswitch: drop GSO packets that are too large
>>>>
>>>>   include/linux/skbuff.h  | 16 ++++++++++++++++
>>>>   net/core/dev.c          |  7 ++++---
>>>>   net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
>>>>   net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
>>>>   net/sched/sch_tbf.c     | 10 ----------
>>>>   5 files changed, 84 insertions(+), 20 deletions(-)
>>>>
>>>> --
>>>> 2.14.1
>>>>
_______________________________________________
dev mailing list
dev@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Pravin Shelar]

On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja@axtens.net> wrote:
> Pravin Shelar <pshelar@ovn.org> writes:
>
>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
>>> When regular packets are forwarded, we validate their size against the
>>> MTU of the destination device. However, when GSO packets are
>>> forwarded, we do not validate their size against the MTU. 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 it in
>>> the driver, this series fixes the forwarding path.
>>>
>> Are there any other possible forwarding path in networking stack? TC
>> is one subsystem that could forward such a packet to the bnx2x device,
>> how is that handled ?
>
> So far I have only looked at bridges, openvswitch and macvlan. In
> general, if the code uses dev_forward_skb() it should automatically be
> fine as that invokes is_skb_forwardable(), which we patch.
>
But there are other ways to forward packets, e.g tc-mirred or bpf
redirect. We need to handle all these cases rather than fixing one at
a time. As Jason suggested netif_needs_gso() looks like good function
to validate if a device is capable of handling given GSO packet.

Re: [PATCH 2/3] net: is_skb_forwardable: validate length of GSO packet segments

[Marcelo Ricardo Leitner]

On Tue, Jan 16, 2018 at 01:09:19PM +1100, Daniel Axtens wrote:
> is_skb_forwardable attempts to detect if a packet is too large to
> be sent to the destination device. However, this test does not
> consider GSO packets, and it is possible that a GSO packet, when
> resegmented, will be larger than the device can transmit.
> 
> Add detection for packets which will be too large by creating a
> skb_gso_validate_len() routine which is similar to
> skb_gso_validate_mtu(), but which considers L2 headers, and wire

Why not add a 3rd parameter to skb_gso_validate_mtu() instead, hlen?
After all it's the only difference between both.

And maybe rename _mtu if you think the term doesn't fit in the new
usecase or add a wrapper around it for each case.

As it is here, it gets very confusing on which function does what,
IMHO.

> it up in is_skb_forwardable().
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  include/linux/skbuff.h |  1 +
>  net/core/dev.c         |  7 ++++---
>  net/core/skbuff.c      | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index b8acafd73272..6a9e4b6f80a7 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_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);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 94435cd09072..5af04be128c1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1841,11 +1841,12 @@ bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
>  	if (skb->len <= len)
>  		return true;
>  
> -	/* if TSO is enabled, we don't care about the length as the packet
> -	 * could be forwarded without being segmented before
> +	/*
> +	 * if TSO is enabled, we need to check the size of the
> +	 * segmented packets
>  	 */
>  	if (skb_is_gso(skb))
> -		return true;
> +		return skb_gso_validate_len(skb, len);
>  
>  	return false;
>  }
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 01e8285aea73..aefe049e4b0c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4945,6 +4945,40 @@ bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
>  }
>  EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
>  
> +/**
> + * skb_gso_validate_len - Will a split GSO skb fit in a given length?
> + *
> + * @skb: GSO skb
> + * @len: length to validate against
> + *
> + * skb_gso_validate_len validates if a given skb will fit a wanted
> + * length once split, including L2, L3 and L4 headers.
> + *
> + * Similar to skb_gso_validate_mtu, but inclusive of L2 headers.
> + */
> +bool skb_gso_validate_len(const struct sk_buff *skb, unsigned int len)
> +{
> +	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> +	const struct sk_buff *iter;
> +	unsigned int hlen;
> +
> +	hlen = skb_gso_mac_seglen(skb);
> +
> +	if (shinfo->gso_size != GSO_BY_FRAGS)
> +		return hlen <= len;
> +
> +	/* Undo this so we can re-use header sizes */
> +	hlen -= GSO_BY_FRAGS;
> +
> +	skb_walk_frags(skb, iter) {
> +		if (hlen + skb_headlen(iter) > len)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(skb_gso_validate_len);
> +
>  static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>  {
>  	if (skb_cow(skb, skb_headroom(skb)) < 0) {
> -- 
> 2.14.1
> 

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:

> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>> Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:
>>
>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>>>> When regular packets are forwarded, we validate their size against the
>>>> MTU of the destination device. However, when GSO packets are
>>>> forwarded, we do not validate their size against the MTU. 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 it in
>>>> the driver, this series fixes the forwarding path.
>>>>
>>> Are there any other possible forwarding path in networking stack? TC
>>> is one subsystem that could forward such a packet to the bnx2x device,
>>> how is that handled ?
>>
>> So far I have only looked at bridges, openvswitch and macvlan. In
>> general, if the code uses dev_forward_skb() it should automatically be
>> fine as that invokes is_skb_forwardable(), which we patch.
>>
> But there are other ways to forward packets, e.g tc-mirred or bpf
> redirect. We need to handle all these cases rather than fixing one at
> a time. As Jason suggested netif_needs_gso() looks like good function
> to validate if a device is capable of handling given GSO packet.

I am not entirely sure this is a better solution.

The biggest reason I am uncomfortable with this is that if
netif_needs_gso() returns true, the skb will be segmented. The segment
sizes will be based on gso_size. Since gso_size is greater than the MTU,
the resulting segments will themselves be over-MTU. Those over-MTU
segments will then be passed to the network card. I think we should not
be creating over-MTU segments; we should instead be dropping the packet
and logging.

I do take the point that you and Jason are making: a more generic
fix would be good. I'm just not sure where to put it.


Regards,
Daniel

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> writes:

> Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:
>
>> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>>> Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:
>>>
>>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>>>>> When regular packets are forwarded, we validate their size against the
>>>>> MTU of the destination device. However, when GSO packets are
>>>>> forwarded, we do not validate their size against the MTU. 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 it in
>>>>> the driver, this series fixes the forwarding path.
>>>>>
>>>> Are there any other possible forwarding path in networking stack? TC
>>>> is one subsystem that could forward such a packet to the bnx2x device,
>>>> how is that handled ?
>>>
>>> So far I have only looked at bridges, openvswitch and macvlan. In
>>> general, if the code uses dev_forward_skb() it should automatically be
>>> fine as that invokes is_skb_forwardable(), which we patch.
>>>
>> But there are other ways to forward packets, e.g tc-mirred or bpf
>> redirect. We need to handle all these cases rather than fixing one at
>> a time. As Jason suggested netif_needs_gso() looks like good function
>> to validate if a device is capable of handling given GSO packet.
>
> I am not entirely sure this is a better solution.
>
> The biggest reason I am uncomfortable with this is that if
> netif_needs_gso() returns true, the skb will be segmented. The segment
> sizes will be based on gso_size. Since gso_size is greater than the MTU,
> the resulting segments will themselves be over-MTU. Those over-MTU
> segments will then be passed to the network card. I think we should not
> be creating over-MTU segments; we should instead be dropping the packet
> and logging.
>
> I do take the point that you and Jason are making: a more generic
> fix would  be good. I'm just not sure where to put it.

How about this? It puts the check in validate_xmit_skb(). (completely untested)

Ultimately I think we need to make a broader policy decision about who
is responsible for ensuring that packets are correctly sized - is it the
caller of dev_queue_xmit (as in bridges and openswitch) or is it lower
down the stack, such as validate_xmit_skb()? Making the caller check is
more efficient - packets created on the system are always going to fit
due to the checks in the protocol code, so rechecking is inefficient and
unnecessary. But checking later is more reliable as we don't have to
identify all forwarding paths.

Regards,
Daniel

diff --git a/net/core/dev.c b/net/core/dev.c
index 5af04be128c1..b8c3f33ece19 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1830,13 +1830,11 @@ static inline void net_timestamp_set(struct sk_buff *skb)
                        __net_timestamp(SKB);           \
        }                                               \
 
-bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
+static inline bool does_skb_fit_mtu(const struct net_device *dev,
+                                   const struct sk_buff *skb)
 {
        unsigned int len;
 
-       if (!(dev->flags & IFF_UP))
-               return false;
-
        len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
        if (skb->len <= len)
                return true;
@@ -1850,6 +1848,14 @@ bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
 
        return false;
 }
+
+bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
+{
+       if (!(dev->flags & IFF_UP))
+               return false;
+
+       return does_skb_fit_mtu(dev, skb);
+}
 EXPORT_SYMBOL_GPL(is_skb_forwardable);
 
 int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
@@ -3081,6 +3087,9 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
        if (unlikely(!skb))
                goto out_null;
 
+       if (unlikely(!does_skb_fit_mtu(dev, skb)))
+               goto out_kfree_skb;
+
        if (netif_needs_gso(skb, features)) {
                struct sk_buff *segs;
 



>
>
> Regards,
> Daniel

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Pravin Shelar]

On Thu, Jan 18, 2018 at 5:28 PM, Daniel Axtens <dja@axtens.net> wrote:
> Pravin Shelar <pshelar@ovn.org> writes:
>
>> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja@axtens.net> wrote:
>>> Pravin Shelar <pshelar@ovn.org> writes:
>>>
>>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
>>>>> When regular packets are forwarded, we validate their size against the
>>>>> MTU of the destination device. However, when GSO packets are
>>>>> forwarded, we do not validate their size against the MTU. 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 it in
>>>>> the driver, this series fixes the forwarding path.
>>>>>
>>>> Are there any other possible forwarding path in networking stack? TC
>>>> is one subsystem that could forward such a packet to the bnx2x device,
>>>> how is that handled ?
>>>
>>> So far I have only looked at bridges, openvswitch and macvlan. In
>>> general, if the code uses dev_forward_skb() it should automatically be
>>> fine as that invokes is_skb_forwardable(), which we patch.
>>>
>> But there are other ways to forward packets, e.g tc-mirred or bpf
>> redirect. We need to handle all these cases rather than fixing one at
>> a time. As Jason suggested netif_needs_gso() looks like good function
>> to validate if a device is capable of handling given GSO packet.
>
> I am not entirely sure this is a better solution.
>
> The biggest reason I am uncomfortable with this is that if
> netif_needs_gso() returns true, the skb will be segmented. The segment
> sizes will be based on gso_size. Since gso_size is greater than the MTU,
> the resulting segments will themselves be over-MTU. Those over-MTU
> segments will then be passed to the network card. I think we should not
> be creating over-MTU segments; we should instead be dropping the packet
> and logging.
>

Would this oversized segment cause the firmware assert?
If this solves the assert issue then I do not see much value in adding
checks in fast-path just for logging.

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Daniel Axtens]

Pravin Shelar <pshelar@ovn.org> writes:

> On Thu, Jan 18, 2018 at 5:28 PM, Daniel Axtens <dja@axtens.net> wrote:
>> Pravin Shelar <pshelar@ovn.org> writes:
>>
>>> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja@axtens.net> wrote:
>>>> Pravin Shelar <pshelar@ovn.org> writes:
>>>>
>>>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@axtens.net> wrote:
>>>>>> When regular packets are forwarded, we validate their size against the
>>>>>> MTU of the destination device. However, when GSO packets are
>>>>>> forwarded, we do not validate their size against the MTU. 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 it in
>>>>>> the driver, this series fixes the forwarding path.
>>>>>>
>>>>> Are there any other possible forwarding path in networking stack? TC
>>>>> is one subsystem that could forward such a packet to the bnx2x device,
>>>>> how is that handled ?
>>>>
>>>> So far I have only looked at bridges, openvswitch and macvlan. In
>>>> general, if the code uses dev_forward_skb() it should automatically be
>>>> fine as that invokes is_skb_forwardable(), which we patch.
>>>>
>>> But there are other ways to forward packets, e.g tc-mirred or bpf
>>> redirect. We need to handle all these cases rather than fixing one at
>>> a time. As Jason suggested netif_needs_gso() looks like good function
>>> to validate if a device is capable of handling given GSO packet.
>>
>> I am not entirely sure this is a better solution.
>>
>> The biggest reason I am uncomfortable with this is that if
>> netif_needs_gso() returns true, the skb will be segmented. The segment
>> sizes will be based on gso_size. Since gso_size is greater than the MTU,
>> the resulting segments will themselves be over-MTU. Those over-MTU
>> segments will then be passed to the network card. I think we should not
>> be creating over-MTU segments; we should instead be dropping the packet
>> and logging.
>>
>
> Would this oversized segment cause the firmware assert?
> If this solves the assert issue then I do not see much value in adding
> checks in fast-path just for logging.

No - I tested this (or rather, as I don't have direct access to a bnx2x
card, this was tested on my behalf): as long as the packet is not a GSO
packet, it doesn't cause the crash. So we *could* segment them, I just
think that knowingly creating invalid segments is not particularly
pleasant.

Do you think my approach in a later email which checks and drops without
logging is sufficient? It is the same GSO check, and also checks non-GSO
packets against the MTU.

Regards,
Daniel

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Pravin Shelar]

On Fri, Jan 19, 2018 at 3:58 AM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
> Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:
>
>> On Thu, Jan 18, 2018 at 5:28 PM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>>> Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:
>>>
>>>> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>>>>> Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org> writes:
>>>>>
>>>>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja-Yfaxwxk/+vWsTnJN9+BGXg@public.gmane.org> wrote:
>>>>>>> When regular packets are forwarded, we validate their size against the
>>>>>>> MTU of the destination device. However, when GSO packets are
>>>>>>> forwarded, we do not validate their size against the MTU. 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 it in
>>>>>>> the driver, this series fixes the forwarding path.
>>>>>>>
>>>>>> Are there any other possible forwarding path in networking stack? TC
>>>>>> is one subsystem that could forward such a packet to the bnx2x device,
>>>>>> how is that handled ?
>>>>>
>>>>> So far I have only looked at bridges, openvswitch and macvlan. In
>>>>> general, if the code uses dev_forward_skb() it should automatically be
>>>>> fine as that invokes is_skb_forwardable(), which we patch.
>>>>>
>>>> But there are other ways to forward packets, e.g tc-mirred or bpf
>>>> redirect. We need to handle all these cases rather than fixing one at
>>>> a time. As Jason suggested netif_needs_gso() looks like good function
>>>> to validate if a device is capable of handling given GSO packet.
>>>
>>> I am not entirely sure this is a better solution.
>>>
>>> The biggest reason I am uncomfortable with this is that if
>>> netif_needs_gso() returns true, the skb will be segmented. The segment
>>> sizes will be based on gso_size. Since gso_size is greater than the MTU,
>>> the resulting segments will themselves be over-MTU. Those over-MTU
>>> segments will then be passed to the network card. I think we should not
>>> be creating over-MTU segments; we should instead be dropping the packet
>>> and logging.
>>>
>>
>> Would this oversized segment cause the firmware assert?
>> If this solves the assert issue then I do not see much value in adding
>> checks in fast-path just for logging.
>
> No - I tested this (or rather, as I don't have direct access to a bnx2x
> card, this was tested on my behalf): as long as the packet is not a GSO
> packet, it doesn't cause the crash. So we *could* segment them, I just
> think that knowingly creating invalid segments is not particularly
> pleasant.
>
I agree it is not perfect. But the other proposed patch does not fix
the connectivity issue. It only adds log msg in such cases at cost of
extra checks/code. Therefore I prefer the easier fix for the issue
which also fixes for all cases of packet forwarding rather than just
OVS and Bridge.

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[David Miller]

From: Pravin Shelar <pshelar@ovn.org>
Date: Fri, 19 Jan 2018 13:54:15 -0800

> I agree it is not perfect. But the other proposed patch does not fix
> the connectivity issue. It only adds log msg in such cases at cost
> of extra checks/code. Therefore I prefer the easier fix for the
> issue which also fixes for all cases of packet forwarding rather
> than just OVS and Bridge.

I really think that something needs to guarantee that device drivers
will never be given either over-MTU or over-max-GSO-seg-size SKBs.

Otherwise drivers need to add completely stupid checks like making
sure that SKB lengths do not exceed the maxmimu value that can be
encoded into descriptors.

What's probably happening often now in such situations is that the
driver ends up masking the length blindly and ends up sending out a
truncated packet.

Which frankly is quite bad too.

It doesn't scale to add these checks into every driver, or trying to
"figure out" which drivers will behave adversely and only add checks
to those.

The kernel shouldn't pass objects with out-of-range attributes
to the driver, period.

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Stephen Hemminger]

On Mon, 22 Jan 2018 15:14:53 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Pravin Shelar <pshelar@ovn.org>
> Date: Fri, 19 Jan 2018 13:54:15 -0800
> 
> > I agree it is not perfect. But the other proposed patch does not fix
> > the connectivity issue. It only adds log msg in such cases at cost
> > of extra checks/code. Therefore I prefer the easier fix for the
> > issue which also fixes for all cases of packet forwarding rather
> > than just OVS and Bridge.  
> 
> I really think that something needs to guarantee that device drivers
> will never be given either over-MTU or over-max-GSO-seg-size SKBs.
> 
> Otherwise drivers need to add completely stupid checks like making
> sure that SKB lengths do not exceed the maxmimu value that can be
> encoded into descriptors.
> 
> What's probably happening often now in such situations is that the
> driver ends up masking the length blindly and ends up sending out a
> truncated packet.
> 
> Which frankly is quite bad too.
> 
> It doesn't scale to add these checks into every driver, or trying to
> "figure out" which drivers will behave adversely and only add checks
> to those.
> 
> The kernel shouldn't pass objects with out-of-range attributes
> to the driver, period.

Agreed. We should make it easier to write non-buggy drivers.

Re: [PATCH 0/3] Check gso_size of packets when forwarding

[Pravin Shelar]

On Mon, Jan 22, 2018 at 12:14 PM, David Miller <davem@davemloft.net> wrote:
> From: Pravin Shelar <pshelar@ovn.org>
> Date: Fri, 19 Jan 2018 13:54:15 -0800
>
>> I agree it is not perfect. But the other proposed patch does not fix
>> the connectivity issue. It only adds log msg in such cases at cost
>> of extra checks/code. Therefore I prefer the easier fix for the
>> issue which also fixes for all cases of packet forwarding rather
>> than just OVS and Bridge.
>
> I really think that something needs to guarantee that device drivers
> will never be given either over-MTU or over-max-GSO-seg-size SKBs.
>
> Otherwise drivers need to add completely stupid checks like making
> sure that SKB lengths do not exceed the maxmimu value that can be
> encoded into descriptors.
>
> What's probably happening often now in such situations is that the
> driver ends up masking the length blindly and ends up sending out a
> truncated packet.
>
> Which frankly is quite bad too.
>
> It doesn't scale to add these checks into every driver, or trying to
> "figure out" which drivers will behave adversely and only add checks
> to those.
>
> The kernel shouldn't pass objects with out-of-range attributes
> to the driver, period.

OK, in that case we could add a check in validate_xmit_skb() as done
by Daniel in a patch posted earlier.