[PATCH bpf-next 1/2] Revert "bpf: Check for BPF_F_ADJ_ROOM_FIXED_GSO when bpf_skb_change_proto"

[PATCH bpf-next 1/2] Revert "bpf: Check for BPF_F_ADJ_ROOM_FIXED_GSO when bpf_skb_change_proto"

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

This reverts commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e.

See the followup commit for the reasoning why I believe the appropriate
approach is to simply make this change without a flag, but it can basically
be summarized as using this helper without the flag is bug-prone or outright
buggy, and thus the default should be this new behaviour.

As this commit has only made it into net-next/master, but not into
any real release, such a backwards incompatible change is still ok.

Cc: Dongseok Yi <dseok.yi@samsung.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/core/filter.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index caa88955562e..04848de3e058 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3235,7 +3235,7 @@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
 	return ret;
 }
 
-static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags)
+static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 {
 	const u32 len_diff = sizeof(struct ipv6hdr) - sizeof(struct iphdr);
 	u32 off = skb_mac_header_len(skb);
@@ -3264,9 +3264,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags)
 		}
 
 		/* Due to IPv6 header, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
+		skb_decrease_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
@@ -3278,7 +3276,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags)
 	return 0;
 }
 
-static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags)
+static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
 {
 	const u32 len_diff = sizeof(struct ipv6hdr) - sizeof(struct iphdr);
 	u32 off = skb_mac_header_len(skb);
@@ -3307,9 +3305,7 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags)
 		}
 
 		/* Due to IPv4 header, MSS can be upgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_increase_gso_size(shinfo, len_diff);
-
+		skb_increase_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
@@ -3321,17 +3317,17 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags)
 	return 0;
 }
 
-static int bpf_skb_proto_xlat(struct sk_buff *skb, __be16 to_proto, u64 flags)
+static int bpf_skb_proto_xlat(struct sk_buff *skb, __be16 to_proto)
 {
 	__be16 from_proto = skb->protocol;
 
 	if (from_proto == htons(ETH_P_IP) &&
 	      to_proto == htons(ETH_P_IPV6))
-		return bpf_skb_proto_4_to_6(skb, flags);
+		return bpf_skb_proto_4_to_6(skb);
 
 	if (from_proto == htons(ETH_P_IPV6) &&
 	      to_proto == htons(ETH_P_IP))
-		return bpf_skb_proto_6_to_4(skb, flags);
+		return bpf_skb_proto_6_to_4(skb);
 
 	return -ENOTSUPP;
 }
@@ -3341,7 +3337,7 @@ BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto,
 {
 	int ret;
 
-	if (unlikely(flags & ~(BPF_F_ADJ_ROOM_FIXED_GSO)))
+	if (unlikely(flags))
 		return -EINVAL;
 
 	/* General idea is that this helper does the basic groundwork
@@ -3361,7 +3357,7 @@ BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto,
 	 * that. For offloads, we mark packet as dodgy, so that headers
 	 * need to be verified first.
 	 */
-	ret = bpf_skb_proto_xlat(skb, proto, flags);
+	ret = bpf_skb_proto_xlat(skb, proto);
 	bpf_compute_data_pointers(skb);
 	return ret;
 }
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH bpf-next 2/2] bpf: do not change gso_size during bpf_skb_change_proto()

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

This is technically a backwards incompatible change in behaviour,
but I'm going to argue that it is very unlikely to break things,
and likely to fix *far* more then it breaks.

In no particular order, various reasons follow:

(a) I've long had a bug assigned to myself to debug a super rare kernel
crash on Android Pixel phones which can (per stacktrace) be traced back
to bpf clat ipv6 to ipv4 protocol conversion causing some sort of ugly
failure much later on during transmit deep in the GSO engine, AFAICT
precisely because of this change to gso_size, though I've never been able
to manually reproduce it.
I believe it may be related to the particular network offload support
of attached usb ethernet dongle being used for tethering off of an
IPv6-only cellular connection.  The reason might be we end up with more
segments than max permitted, or with a gso packet with only one segment...
(either way we break some assumption and hit a BUG_ON)

(b) There is no check that the gso_size is > 20 when reducing it by 20,
so we might end up with a negative (or underflowing) gso_size or
a gso_size of 0.  This can't possibly be good.
Indeed this is probably somehow exploitable (or at least can result
in a kernel crash) by delivering crafted packets and perhaps triggering
an infinite loop or a divide by zero...
As a reminder: gso_size (mss) is related to mtu, but not directly
derived from it: gso_size/mss may be significantly smaller then
one would get by deriving from local mtu.  And on some nics (which
do loose mtu checking on receive, it may even potentially be larger,
for example my work pc with 1500 mtu can receive 1520 byte frames
[and sometimes does due to bugs in a vendor plat46 implementation]).
Indeed even just going from 21 to 1 is potentially problematic because
it increases the number of segments by a factor of 21 (think DoS,
or some other crash due to too many segments).

(c) It's always safe to not increase the gso_size, because it doesn't
result in the max packet size increasing.  So the skb_increase_gso_size()
call was always unnecessary for correctness (and outright undesirable, see
later).  As such the only part which is potentially dangerous (ie. could
cause backwards compatibility issues) is the removal of the
skb_decrease_gso_size() call.

(d) If the packets are ultimately destined to the local device, then
there is absolutely no benefit to playing around with gso_size.
It only matters if the packets will egress the device.  ie. we're
either forwarding, or transmitting from the device.

(e) This logic only triggers for packets which are GSO.  It does not
trigger for skbs which are not GSO.  It will not convert a non-GSO mtu
sized packet into a GSO packet (and you don't even know what the mtu is,
so you can't even fix it).  As such your transmit path must *already* be
able to handle an mtu 20 bytes larger then your receive path (for ipv4
to ipv6 translation) - and indeed 28 bytes larger due to ipv4 fragments.
Thus removing the skb_decrease_gso_size() call doesn't actually increase
the size of the packets your transmit side must be able to handle.
ie. to handle non-GSO max-mtu packets, the ipv4/ipv6 device/route mtus
must already be set correctly.  Since for example with an ipv4 egress mtu
of 1500, ipv4 to ipv6 translation will already build 1520 byte ipv6 frames,
so you need a 1520 byte device mtu.  This means if your ipv6 device's
egress mtu is 1280, your ipv4 route must be 1260 (and actually 1252,
because of the need to handle fragments).  This is to handle normal non-GSO
packets.  Thus the reduction is simply not needed for GSO packets,
because when they're correctly built, they will already be the right size.

(f) TSO/GSO should be able to exactly undo GRO: the number of packets
(TCP segments) should not be modified, so that TCP's mss counting works
correctly (this matters for congestion control).
If protocol conversion changes the gso_size, then the number of TCP segments
may increase or decrease.  Packet loss after protocol conversion can result
in partial loss of mss segments that the sender sent.  How's the sending
TCP stack going to react to receiving ACKs/SACKs in the middle of the
segments it sent?

(g) skb_{decrease,increase}_gso_size() are already no-ops for GSO_BY_FRAGS
case (besides triggering WARN_ON_ONCE). This means you already cannot
guarantee that gso_size (and thus resulting packet mtu) is changed.
ie. you must assume it won't be changed.

(h) changing gso_size is outright buggy for UDP GSO packets, where framing
matters (I believe that's also the case for SCTP, but it's already excluded
by [g]).  So the only remaining case is TCP, which also doesn't want it
(see [f]).

(i) see also the reasoning on the previous attempt at fixing this
(commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e) which shows
that the current behaviour causes TCP packet loss:

  In the forwarding path GRO -> BPF 6 to 4 -> GSO for TCP traffic, the
  coalesced packet payload can be > MSS, but < MSS + 20.

  bpf_skb_proto_6_to_4() will upgrade the MSS and it can be > the payload
  length. After then tcp_gso_segment checks for the payload length if it
  is <= MSS. The condition is causing the packet to be dropped.

  tcp_gso_segment():
    [...]
    mss = skb_shinfo(skb)->gso_size;
    if (unlikely(skb->len <= mss)) goto out;
    [...]

Thus changing the gso_size is simply a very bad idea.
Increasing is unnecessary and buggy, and decreasing can go negative.

Cc: Dongseok Yi <dseok.yi@samsung.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Willem de Bruijn <willemb@google.com>
Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/core/filter.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 04848de3e058..953b6c31b803 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3263,8 +3263,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 			shinfo->gso_type |=  SKB_GSO_TCPV6;
 		}
 
-		/* Due to IPv6 header, MSS needs to be downgraded. */
-		skb_decrease_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
@@ -3304,8 +3302,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
 			shinfo->gso_type |=  SKB_GSO_TCPV4;
 		}
 
-		/* Due to IPv4 header, MSS can be upgraded. */
-		skb_increase_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
-- 
2.32.0.rc1.229.g3e70b5a671-goog

Re: [PATCH bpf-next 2/2] bpf: do not change gso_size during bpf_skb_change_proto()

[Maciej Żenczykowski]

On Thu, Jun 3, 2021 at 6:52 PM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> From: Maciej Żenczykowski <maze@google.com>
>
> This is technically a backwards incompatible change in behaviour,
> but I'm going to argue that it is very unlikely to break things,
> and likely to fix *far* more then it breaks.
>
> In no particular order, various reasons follow:
>
> (a) I've long had a bug assigned to myself to debug a super rare kernel
> crash on Android Pixel phones which can (per stacktrace) be traced back
> to bpf clat ipv6 to ipv4 protocol conversion causing some sort of ugly
> failure much later on during transmit deep in the GSO engine, AFAICT
> precisely because of this change to gso_size, though I've never been able
> to manually reproduce it.
> I believe it may be related to the particular network offload support
> of attached usb ethernet dongle being used for tethering off of an
> IPv6-only cellular connection.  The reason might be we end up with more
> segments than max permitted, or with a gso packet with only one segment...
> (either way we break some assumption and hit a BUG_ON)
>
> (b) There is no check that the gso_size is > 20 when reducing it by 20,
> so we might end up with a negative (or underflowing) gso_size or
> a gso_size of 0.  This can't possibly be good.
> Indeed this is probably somehow exploitable (or at least can result
> in a kernel crash) by delivering crafted packets and perhaps triggering
> an infinite loop or a divide by zero...
> As a reminder: gso_size (mss) is related to mtu, but not directly
> derived from it: gso_size/mss may be significantly smaller then
> one would get by deriving from local mtu.  And on some nics (which
> do loose mtu checking on receive, it may even potentially be larger,
> for example my work pc with 1500 mtu can receive 1520 byte frames
> [and sometimes does due to bugs in a vendor plat46 implementation]).
> Indeed even just going from 21 to 1 is potentially problematic because
> it increases the number of segments by a factor of 21 (think DoS,
> or some other crash due to too many segments).
>
> (c) It's always safe to not increase the gso_size, because it doesn't
> result in the max packet size increasing.  So the skb_increase_gso_size()
> call was always unnecessary for correctness (and outright undesirable, see
> later).  As such the only part which is potentially dangerous (ie. could
> cause backwards compatibility issues) is the removal of the
> skb_decrease_gso_size() call.
>
> (d) If the packets are ultimately destined to the local device, then
> there is absolutely no benefit to playing around with gso_size.
> It only matters if the packets will egress the device.  ie. we're
> either forwarding, or transmitting from the device.
>
> (e) This logic only triggers for packets which are GSO.  It does not
> trigger for skbs which are not GSO.  It will not convert a non-GSO mtu
> sized packet into a GSO packet (and you don't even know what the mtu is,
> so you can't even fix it).  As such your transmit path must *already* be
> able to handle an mtu 20 bytes larger then your receive path (for ipv4
> to ipv6 translation) - and indeed 28 bytes larger due to ipv4 fragments.
> Thus removing the skb_decrease_gso_size() call doesn't actually increase
> the size of the packets your transmit side must be able to handle.
> ie. to handle non-GSO max-mtu packets, the ipv4/ipv6 device/route mtus
> must already be set correctly.  Since for example with an ipv4 egress mtu
> of 1500, ipv4 to ipv6 translation will already build 1520 byte ipv6 frames,
> so you need a 1520 byte device mtu.  This means if your ipv6 device's
> egress mtu is 1280, your ipv4 route must be 1260 (and actually 1252,
> because of the need to handle fragments).  This is to handle normal non-GSO
> packets.  Thus the reduction is simply not needed for GSO packets,
> because when they're correctly built, they will already be the right size.
>
> (f) TSO/GSO should be able to exactly undo GRO: the number of packets
> (TCP segments) should not be modified, so that TCP's mss counting works
> correctly (this matters for congestion control).
> If protocol conversion changes the gso_size, then the number of TCP segments
> may increase or decrease.  Packet loss after protocol conversion can result
> in partial loss of mss segments that the sender sent.  How's the sending
> TCP stack going to react to receiving ACKs/SACKs in the middle of the
> segments it sent?
>
> (g) skb_{decrease,increase}_gso_size() are already no-ops for GSO_BY_FRAGS
> case (besides triggering WARN_ON_ONCE). This means you already cannot
> guarantee that gso_size (and thus resulting packet mtu) is changed.
> ie. you must assume it won't be changed.
>
> (h) changing gso_size is outright buggy for UDP GSO packets, where framing
> matters (I believe that's also the case for SCTP, but it's already excluded
> by [g]).  So the only remaining case is TCP, which also doesn't want it
> (see [f]).
>
> (i) see also the reasoning on the previous attempt at fixing this
> (commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e) which shows
> that the current behaviour causes TCP packet loss:
>
>   In the forwarding path GRO -> BPF 6 to 4 -> GSO for TCP traffic, the
>   coalesced packet payload can be > MSS, but < MSS + 20.
>
>   bpf_skb_proto_6_to_4() will upgrade the MSS and it can be > the payload
>   length. After then tcp_gso_segment checks for the payload length if it
>   is <= MSS. The condition is causing the packet to be dropped.
>
>   tcp_gso_segment():
>     [...]
>     mss = skb_shinfo(skb)->gso_size;
>     if (unlikely(skb->len <= mss)) goto out;
>     [...]
>
> Thus changing the gso_size is simply a very bad idea.
> Increasing is unnecessary and buggy, and decreasing can go negative.
>
> Cc: Dongseok Yi <dseok.yi@samsung.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Willem de Bruijn <willemb@google.com>
> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>  net/core/filter.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 04848de3e058..953b6c31b803 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3263,8 +3263,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
>                         shinfo->gso_type |=  SKB_GSO_TCPV6;
>                 }
>
> -               /* Due to IPv6 header, MSS needs to be downgraded. */
> -               skb_decrease_gso_size(shinfo, len_diff);
>                 /* Header must be checked, and gso_segs recomputed. */
>                 shinfo->gso_type |= SKB_GSO_DODGY;
>                 shinfo->gso_segs = 0;
> @@ -3304,8 +3302,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
>                         shinfo->gso_type |=  SKB_GSO_TCPV4;
>                 }
>
> -               /* Due to IPv4 header, MSS can be upgraded. */
> -               skb_increase_gso_size(shinfo, len_diff);
>                 /* Header must be checked, and gso_segs recomputed. */
>                 shinfo->gso_type |= SKB_GSO_DODGY;
>                 shinfo->gso_segs = 0;
> --
> 2.32.0.rc1.229.g3e70b5a671-goog

This patch series (ie. this patch and the previous revert) seems to
have gotten no response, and we're running out of time, since it
reverts the newly added api.

Opinions?

Re: [PATCH bpf-next 2/2] bpf: do not change gso_size during bpf_skb_change_proto()

[Daniel Borkmann]

On 6/15/21 9:35 AM, Maciej Żenczykowski wrote:
> On Thu, Jun 3, 2021 at 6:52 PM Maciej Żenczykowski
> <zenczykowski@gmail.com> wrote:
>> From: Maciej Żenczykowski <maze@google.com>
>>
>> This is technically a backwards incompatible change in behaviour,
>> but I'm going to argue that it is very unlikely to break things,
>> and likely to fix *far* more then it breaks.
>>
>> In no particular order, various reasons follow:
>>
>> (a) I've long had a bug assigned to myself to debug a super rare kernel
>> crash on Android Pixel phones which can (per stacktrace) be traced back
>> to bpf clat ipv6 to ipv4 protocol conversion causing some sort of ugly
>> failure much later on during transmit deep in the GSO engine, AFAICT
>> precisely because of this change to gso_size, though I've never been able
>> to manually reproduce it.
>> I believe it may be related to the particular network offload support
>> of attached usb ethernet dongle being used for tethering off of an
>> IPv6-only cellular connection.  The reason might be we end up with more
>> segments than max permitted, or with a gso packet with only one segment...
>> (either way we break some assumption and hit a BUG_ON)

Do you happen to have some more debug data from there, e.g. which bug_on
is hit? Do you have some pointers to the driver code where you suspect
this could cause an issue?

>> (b) There is no check that the gso_size is > 20 when reducing it by 20,
>> so we might end up with a negative (or underflowing) gso_size or
>> a gso_size of 0.  This can't possibly be good.
>> Indeed this is probably somehow exploitable (or at least can result
>> in a kernel crash) by delivering crafted packets and perhaps triggering
>> an infinite loop or a divide by zero...
>> As a reminder: gso_size (mss) is related to mtu, but not directly
>> derived from it: gso_size/mss may be significantly smaller then
>> one would get by deriving from local mtu.  And on some nics (which
>> do loose mtu checking on receive, it may even potentially be larger,
>> for example my work pc with 1500 mtu can receive 1520 byte frames
>> [and sometimes does due to bugs in a vendor plat46 implementation]).
>> Indeed even just going from 21 to 1 is potentially problematic because
>> it increases the number of segments by a factor of 21 (think DoS,
>> or some other crash due to too many segments).

Do you have a reproducer for creating such small gso_size from stack, is
this mainly from virtio_net side possible? If it's too small, perhaps the
gso attributes should just be cleared from the skb generally instead of
marking SKB_GSO_DODGY as we otherwise do?

>> (c) It's always safe to not increase the gso_size, because it doesn't
>> result in the max packet size increasing.  So the skb_increase_gso_size()
>> call was always unnecessary for correctness (and outright undesirable, see
>> later).  As such the only part which is potentially dangerous (ie. could
>> cause backwards compatibility issues) is the removal of the
>> skb_decrease_gso_size() call.

Right.

>> (d) If the packets are ultimately destined to the local device, then
>> there is absolutely no benefit to playing around with gso_size.
>> It only matters if the packets will egress the device.  ie. we're
>> either forwarding, or transmitting from the device.

Yep, the issue is that we don't know this at the time of the helper call.

>> (e) This logic only triggers for packets which are GSO.  It does not
>> trigger for skbs which are not GSO.  It will not convert a non-GSO mtu
>> sized packet into a GSO packet (and you don't even know what the mtu is,
>> so you can't even fix it).  As such your transmit path must *already* be
>> able to handle an mtu 20 bytes larger then your receive path (for ipv4
>> to ipv6 translation) - and indeed 28 bytes larger due to ipv4 fragments.
>> Thus removing the skb_decrease_gso_size() call doesn't actually increase
>> the size of the packets your transmit side must be able to handle.
>> ie. to handle non-GSO max-mtu packets, the ipv4/ipv6 device/route mtus
>> must already be set correctly.  Since for example with an ipv4 egress mtu
>> of 1500, ipv4 to ipv6 translation will already build 1520 byte ipv6 frames,
>> so you need a 1520 byte device mtu.  This means if your ipv6 device's
>> egress mtu is 1280, your ipv4 route must be 1260 (and actually 1252,
>> because of the need to handle fragments).  This is to handle normal non-GSO
>> packets.  Thus the reduction is simply not needed for GSO packets,
>> because when they're correctly built, they will already be the right size.

Makes sense to me.

>> (f) TSO/GSO should be able to exactly undo GRO: the number of packets
>> (TCP segments) should not be modified, so that TCP's mss counting works
>> correctly (this matters for congestion control).
>> If protocol conversion changes the gso_size, then the number of TCP segments
>> may increase or decrease.  Packet loss after protocol conversion can result
>> in partial loss of mss segments that the sender sent.  How's the sending
>> TCP stack going to react to receiving ACKs/SACKs in the middle of the
>> segments it sent?
>>
>> (g) skb_{decrease,increase}_gso_size() are already no-ops for GSO_BY_FRAGS
>> case (besides triggering WARN_ON_ONCE). This means you already cannot
>> guarantee that gso_size (and thus resulting packet mtu) is changed.
>> ie. you must assume it won't be changed.
>>
>> (h) changing gso_size is outright buggy for UDP GSO packets, where framing
>> matters (I believe that's also the case for SCTP, but it's already excluded
>> by [g]).  So the only remaining case is TCP, which also doesn't want it
>> (see [f]).
>>
>> (i) see also the reasoning on the previous attempt at fixing this
>> (commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e) which shows
>> that the current behaviour causes TCP packet loss:
>>
>>    In the forwarding path GRO -> BPF 6 to 4 -> GSO for TCP traffic, the
>>    coalesced packet payload can be > MSS, but < MSS + 20.
>>
>>    bpf_skb_proto_6_to_4() will upgrade the MSS and it can be > the payload
>>    length. After then tcp_gso_segment checks for the payload length if it
>>    is <= MSS. The condition is causing the packet to be dropped.
>>
>>    tcp_gso_segment():
>>      [...]
>>      mss = skb_shinfo(skb)->gso_size;
>>      if (unlikely(skb->len <= mss)) goto out;
>>      [...]
>>
>> Thus changing the gso_size is simply a very bad idea.
>> Increasing is unnecessary and buggy, and decreasing can go negative.
>>
>> Cc: Dongseok Yi <dseok.yi@samsung.com>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
>> Signed-off-by: Maciej Żenczykowski <maze@google.com>
>> ---
>>   net/core/filter.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 04848de3e058..953b6c31b803 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3263,8 +3263,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
>>                          shinfo->gso_type |=  SKB_GSO_TCPV6;
>>                  }
>>
>> -               /* Due to IPv6 header, MSS needs to be downgraded. */
>> -               skb_decrease_gso_size(shinfo, len_diff);
>>                  /* Header must be checked, and gso_segs recomputed. */
>>                  shinfo->gso_type |= SKB_GSO_DODGY;
>>                  shinfo->gso_segs = 0;
>> @@ -3304,8 +3302,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
>>                          shinfo->gso_type |=  SKB_GSO_TCPV4;
>>                  }
>>
>> -               /* Due to IPv4 header, MSS can be upgraded. */
>> -               skb_increase_gso_size(shinfo, len_diff);
>>                  /* Header must be checked, and gso_segs recomputed. */
>>                  shinfo->gso_type |= SKB_GSO_DODGY;
>>                  shinfo->gso_segs = 0;
>> --
>> 2.32.0.rc1.229.g3e70b5a671-goog
> 
> This patch series (ie. this patch and the previous revert) seems to
> have gotten no response, and we're running out of time, since it
> reverts the newly added api.

The change you're reverting in patch 1/2 is only in net-next, but not in
Linus tree, so there still is a large enough time window for at least the
revert. That said, I presume what you mean here is to just revert the 1/2
in bpf-next and the 2/2 fix targeted for bpf tree, no?

Few follow-up questions:

1) Could we then also cover the case of skb_is_gso(skb) && !skb_is_gso_tcp(skb)
    that we currently reject with -ENOTSUPP, in other words all GSO cases?

2) Do we still need to mark SKB_GSO_DODGY and reset segs? I presume not anymore
    after this change?

3) skb_{decrease,increase}_gso_size() should probably just removed then.

Thanks,
Daniel

Re: [PATCH bpf-next 2/2] bpf: do not change gso_size during bpf_skb_change_proto()

[Maciej Żenczykowski]

> >> (a) I've long had a bug assigned to myself to debug a super rare kernel
> >> crash on Android Pixel phones which can (per stacktrace) be traced back
> >> to bpf clat ipv6 to ipv4 protocol conversion causing some sort of ugly
> >> failure much later on during transmit deep in the GSO engine, AFAICT
> >> precisely because of this change to gso_size, though I've never been able
> >> to manually reproduce it.
> >> I believe it may be related to the particular network offload support
> >> of attached usb ethernet dongle being used for tethering off of an
> >> IPv6-only cellular connection.  The reason might be we end up with more
> >> segments than max permitted, or with a gso packet with only one segment...
> >> (either way we break some assumption and hit a BUG_ON)
>
> Do you happen to have some more debug data from there, e.g. which bug_on
> is hit? Do you have some pointers to the driver code where you suspect
> this could cause an issue?

Yes, I found an old relevant stack trace in google bug 158835517.
This is from a blueline (Pixel 3 non-XL) running a 4.9.223 derived kernel.

[57742.623372] c0      0 ------------[ cut here ]------------
[57742.623451] c0      0 kernel BUG at net/core/skbuff.c:3290!
[57742.623473] c0      0 Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[57742.623500] Modules linked in: ftm5 heatmap videobuf2_vmalloc
videobuf2_memops snd_soc_sdm845 snd_soc_cs35l36 snd_soc_wcd_spi
snd_soc_wcd934x snd_soc_wcd9xxx wcd_dsp_glink wcd_core pinctrl_wcd
wlan(O)
[57742.623676] c0      0 CPU: 0 PID: 0 Comm: swapper/0 Tainted: G
     O    4.9.223-ga7ce4286ca2d-ab6546858 #0
[57742.623696] c0      0 Hardware name: Google Inc. MSM sdm845 B1 DVT1.1 (DT)
[57742.623727] c0      0 task: 0000000097327d34 task.stack: 00000000533bef44
[57742.623770] c0      0 PC is at skb_segment+0xeb8/0xf1c
[57742.623786] c0      0 LR is at skb_segment+0x998/0xf1c
...
[57742.625301] c0      0 1720: 0000000000000011 ffffffc13b8b1850
ffffff898aa2b530 ffffffc13b8b1770
[57742.625321] c0      0 [<000000008025a24e>] skb_segment+0xeb8/0xf1c
[57742.625348] c0      0 [<00000000f0dbe6cc>] tcp_gso_segment+0xdc/0x428
[57742.625376] c0      0 [<000000009ff681a5>] tcp6_gso_segment+0x60/0x17c
[57742.625391] c0      0 [<00000000ae27ff84>] ipv6_gso_segment+0x208/0x3d0
[57742.625413] c0      0 [<000000009ff20266>] skb_mac_gso_segment+0xcc/0x198
[57742.625430] c0      0 [<000000005f3e189b>] __skb_gso_segment+0xe0/0x198
[57742.625448] c0      0 [<000000002756bf90>] validate_xmit_skb+0x214/0x3ac
[57742.625473] c0      0 [<00000000d536666c>] sch_direct_xmit+0x8c/0x37c
[57742.625492] c0      0 [<000000006ae4cbff>] __qdisc_run+0x3e4/0x5d4
[57742.625512] c0      0 [<0000000040112c03>] __dev_queue_xmit+0x4d8/0xc9c
[57742.625538] c0      0 [<000000002adaadc6>] __bpf_redirect+0x148/0x2f8
[57742.625557] c0      0 [<00000000766573a7>] __dev_queue_xmit+0x830/0xc9c
[57742.625578] c0      0 [<000000006df3822d>] neigh_direct_output+0x1c/0x28
[57742.625607] c0      0 [<00000000bacb35cc>] ip_finish_output2+0x3ac/0x6cc
[57742.625628] c0      0 [<00000000e7f62131>] ip_finish_output+0x2e4/0x360
[57742.625643] c0      0 [<0000000079e638f1>] ip_output+0x19c/0x274
[57742.625666] c0      0 [<0000000092e42b8c>] NF_HOOK_THRESH+0x150/0x1cc
[57742.625685] c0      0 [<00000000b4e9c6f8>] ip_forward+0x468/0x510
[57742.625704] c0      0 [<00000000779cadd9>] ip_rcv_finish+0x228/0x3c0
[57742.625723] c0      0 [<000000006eee429d>] ip_rcv+0x3b8/0x53c
[57742.625740] c0      0 [<00000000eabf9034>]
__netif_receive_skb_core+0xb10/0xe68
[57742.625761] c0      0 [<000000009368ee55>]
netif_receive_skb_internal+0x1b4/0x26c
[57742.625775] c0      0 [<00000000afac19f3>] napi_gro_complete+0x5c/0x180
[57742.625795] c0      0 [<000000001a7429fd>] napi_complete_done+0x70/0x14c
[57742.625824] c0      0 [<000000000876f6ad>] r8152_poll+0x1138/0x14c8
[57742.625846] c0      0 [<000000006a1f8e3a>] napi_poll+0x8c/0x2f0
[57742.625868] c0      0 [<00000000128c5761>] net_rx_action+0xa8/0x2e8
[57742.625892] c0      0 [<00000000684eda45>] __do_softirq+0x23c/0x568
[57742.625921] c0      0 [<000000008094d781>] irq_exit+0x130/0x144
[57742.625949] c0      0 [<00000000c3de88cc>] __handle_domain_irq+0x108/0x16c
[57742.625963] c0      0 [<000000005c213a95>] gic_handle_irq.21048+0x124/0x19c
[57742.625979] c0      0 Exception stack(0xffffff898c003c20 to
0xffffff898c003d50)
[57742.625999] c0      0 3c20: ffffcb7ad1c65ff9 0000000000000000
0000000000000000 0000000000000001
[57742.626014] c0      0 3c40: 000034852e1f1c83 0000000000000000
0000000000300000 0000000000000000
[57742.626033] c0      0 3c60: 0000000000000000 00000000000039d0
0000000000000018 003529018eb90a43
[57742.626052] c0      0 3c80: 00000000341555ac 00000000019b7894
00000000c800ffff 0000000000000000
[57742.626070] c0      0 3ca0: 00000000000003fc 00000000ffffffff
ffffff898c004030 0000000000000002
[57742.626089] c0      0 3cc0: ffffffc12c4d2858 ffffffc12c4d2018
ffffff898c00f7f0 ffffff898c11f990
[57742.626107] c0      0 3ce0: 0000000000000001 00003484f804e64b
ffffff898adce668 ffffff898c460108
[57742.626126] c0      0 3d00: ffffff898c460000 ffffff898c003d90
ffffff898a547e1c ffffff898c003d50
[57742.626145] c0      0 3d20: ffffff898a547e58 00000000a0c00145
ffffff898c00f7f0 ffffffc12c4d2018
[57742.626159] c0      0 3d40: ffffffffffffffff 0000000000000002
[57742.626179] c0      0 [<00000000179ba2a9>] el1_irq+0xc4/0x13c
[57742.626205] c0      0 [<0000000052ad168a>] lpm_cpuidle_enter+0x5c0/0x670
[57742.626233] c0      0 [<00000000b8e10462>] cpuidle_enter_state+0x200/0x400
[57742.626260] c0      0 [<0000000048578bb9>] cpu_idle_loop+0x294/0x440
[57742.626276] c0      0 [<00000000fbf7777c>] cpu_idle_loop+0x0/0x440
[57742.626304] c0      0 [<0000000012a6efc3>] kernel_init+0x0/0x2a8
[57742.626333] c0      0 [<0000000034e25c8d>] start_kernel+0xe04/0xe0c
[57742.626350] c0      0 [<000000000861285f>] __primary_switched+0x6c/0x98
[57742.626375] c0      0 Code: a94f6ffc f85f8e5e 910503ff d65f03c0 (d4210000)
[57742.626419] c0      0 ---[ end trace 5245603348170006 ]---
[57742.727830] c0      0 Kernel panic - not syncing: Fatal exception
in interrupt

The commit is 'a7ce4286ca2d LTS: Merge android-4.9-q (4.9.223) into
android-msm-pixel-4.9' and it is crashing at
https://android.googlesource.com/kernel/msm/+/a7ce4286ca2d640068055973938f69e3a069e67a/net/core/skbuff.c#3290

struct sk_buff *skb_segment(struct sk_buff *head_skb,
netdev_features_t features) {
  ...
  while (pos < offset + len) {
    if (i >= nfrags) {
      BUG_ON(skb_headlen(list_skb));  <-- crash

My bare bones initial analysis:

This looks like an ipv4/tcp upload from rx csum/gso capable ethernet
(realtek 8152 100mbps or 8153 gigabit) usb dongle hitting ipv4 nat +
forwarding to v4 tun clat device, and then tc egress bpf on that v4
tun device doing clat ipv4-to-ipv6 translation and bpf_redirect to v6
only cell interface (ethernet header less), then triggering a bug in
the kernel's software segmentation engine on transmit through the
cellular uplink (which is a T-Mobile ipv6 only cell network).  This is
on a blueline 4.9 kernel.

User confirmed - to paraphrase:

Home internet was down. I thought I would try using ethernet tethering
to power my whole home's internet. I plugged my Pixel 3 into the
included USB-A to USB-C adapter, into an Insignia NS-PU98635 [note:
https://www.insigniaproducts.com/pdp/NS-PU98635/3510527 ] into my Nest
Wifi router [note: presumably via an ethernet cable] then turned on
ethernet tethering. It seemed to work for a while, but then the phone
rebooted.

Carrier was Google Fi, presumably 'roaming' on T-Mobile, so ipv6-only
cellular network]

Internally, I already asked Willem and Eric about this, I'll quote
from the initial investigation in the bug.

Eric said (trimmed):
  skb_segment() is not yet able to segment something that GRO has
aggregated with a frag_list, if gso_size has to be changed.
  skb_segment() is not generic enough.
  I tried to fix this in the past:
https://lore.kernel.org/netdev/94cd6f4d-09d4-11c0-64f4-bdc544bb3dcb@gmail.com/
  This was the patch Herbert came up 7 years ago.
  https://marc.info/?l=linux-netdev&m=138419594024851&w=2
  I believe my proposal was :
https://www.spinics.net/lists/netdev/msg255549.html

Willem said (slightly trimmed):
  I wonder if the fix as a result of that upstream discussion would
help here, too.
  Basically, it falls back onto non-sg copy based segmentation for some packets.
  I think so. Can you check whether your kernel has this 4.19 stable backport:

  net: gso: Fix skb_segment splat when splitting gso_size mangled skb
having linear-headed frag_list
  https://lore.kernel.org/patchwork/patch/1128590/

  The real solution is to not play gso_size games in
bpf_skb_proto_[46]_to_[64], of course.
  I should have added support for BPF_F_ADJ_ROOM_FIXED_GSO to those
functions when I introduced that.
  As you pointed out offline, even if we add it now, a missing flag causes
  runtime, not build time, failure and requires a workaround...

(back story: writing bpf code that works on all manner of kernels that
may or may not have all fixes/backports,
and may be as old as 4.9 is challenging.  Some things rely on multiple
versions of the bpf code, and using
the first one that successfully loads, other things depend on runtime
fallbacks, etc.  Here with the new flag,
we'd have to do something like bpf_skb_change_proto(FIXED_GSO), and if
that fails [presumably due to lack
of kernel patch] fall back to bpf_skb_change_proto(0).  With the
revert + this patch, it'll 'just' work.)

  One more thing, maybe you can trigger linearization of these skbs when
  they traverse an intermediary device, notably tun, by disabling with
  ethtool -K tx-scatter-gather-fraglist

  Then in validate_xmit_skb on transmit from that device it should hit
  the skb_need_linearize (?)  This is wild speculation..

  Oh 4.9. Also, the bpf_redirect means that that egress path is never
hit on the tun device of course.

  This is the 4.9 stable backport
  https://lore.kernel.org/stable/20190919214802.102848604@linuxfoundation.org/

To which my response was:
  commit 162a5a8c3aff15c449e6b38355cdf80ab4f77a5a
  net: gso: Fix skb_segment splat when splitting gso_size mangled skb
having linear-headed frag_list
git describe 162a5a8c3aff15c449e6b38355cdf80ab4f77a5a
v4.9.193-6-g162a5a8c3aff
So theoretically it's already in the tree...

And the discussion petered out roughly a year ago.
I've never found a way to trigger this on demand (via veth or otherwise)

> >> (b) There is no check that the gso_size is > 20 when reducing it by 20,
> >> so we might end up with a negative (or underflowing) gso_size or
> >> a gso_size of 0.  This can't possibly be good.
> >> Indeed this is probably somehow exploitable (or at least can result
> >> in a kernel crash) by delivering crafted packets and perhaps triggering
> >> an infinite loop or a divide by zero...
> >> As a reminder: gso_size (mss) is related to mtu, but not directly
> >> derived from it: gso_size/mss may be significantly smaller then
> >> one would get by deriving from local mtu.  And on some nics (which
> >> do loose mtu checking on receive, it may even potentially be larger,
> >> for example my work pc with 1500 mtu can receive 1520 byte frames
> >> [and sometimes does due to bugs in a vendor plat46 implementation]).
> >> Indeed even just going from 21 to 1 is potentially problematic because
> >> it increases the number of segments by a factor of 21 (think DoS,
> >> or some other crash due to too many segments).
>
> Do you have a reproducer for creating such small gso_size from stack, is
> this mainly from virtio_net side possible? If it's too small, perhaps the
> gso attributes should just be cleared from the skb generally instead of
> marking SKB_GSO_DODGY as we otherwise do?

I don't.  I tend to think it's not possible with the linux tcp stack,
but it should be possible - if annoying to do - with a raw socket.

You don't even need an established tcp connection, since gro will
happily aggregate packets for non existing flows.

So you simply need to send two consecutive ipv4/tcp segments with 20
byte tcp payload (ie. gso_size),
and gro should merge them, and then ipv4-to-ipv6 conversion should
result in a ipv6/tcp headers + 40 byte payload, yet 0-byte gso_size
packet, and it should crash.

I've checked the code and we later DIV_ROUND_UP by gso_size, which
should thus div by zero.

But I haven't ever actually tried to trigger it.

> The change you're reverting in patch 1/2 is only in net-next, but not in
> Linus tree, so there still is a large enough time window for at least the
> revert. That said, I presume what you mean here is to just revert the 1/2
> in bpf-next and the 2/2 fix targeted for bpf tree, no?

That would be ideal, yes.

> Few follow-up questions:
>
> 1) Could we then also cover the case of skb_is_gso(skb) && !skb_is_gso_tcp(skb)
>     that we currently reject with -ENOTSUPP, in other words all GSO cases?

I think so.  Somehow I missed this condition even being there.
If this patch series is accepted I'll follow up to remove that
(presumably in bpf-next though?)

> 2) Do we still need to mark SKB_GSO_DODGY and reset segs? I presume not anymore
>     after this change?

I'm uncertain.  It certainly seems safer to leave it?

One could argue that any gso packet modified by bpf write operations
should technically be SKB_GSO_DODGY...

After all, the user may fail to even put in a valid
ethernet/ipv4/ipv6(/tcp) header...

That said, that seems to be user error, so I think I'd be willing to
remove it, but I think that would belong in bpf-next rather than bpf.

> 3) skb_{decrease,increase}_gso_size() should probably just removed then.

Oh, interesting, are there no other users?  But again, that would seem
to be a bpf-next candidate I think?
But in general I would agree playing games with gso_size is just a bad
idea no matter what.

- Maciej

[PATCH bpf-next v2 1/4] Revert "bpf: Check for BPF_F_ADJ_ROOM_FIXED_GSO when bpf_skb_change_proto"

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

This reverts commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e.

See the followup commit for the reasoning why I believe the appropriate
approach is to simply make this change without a flag, but it can basically
be summarized as using this helper without the flag is bug-prone or outright
buggy, and thus the default should be this new behaviour.

As this commit has only made it into net-next/master, but not into
any real release, such a backwards incompatible change is still ok.

Cc: Dongseok Yi <dseok.yi@samsung.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/core/filter.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 239de1306de9..65ab4e21c087 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3235,7 +3235,7 @@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
 	return ret;
 }
 
-static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags)
+static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 {
 	const u32 len_diff = sizeof(struct ipv6hdr) - sizeof(struct iphdr);
 	u32 off = skb_mac_header_len(skb);
@@ -3264,9 +3264,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags)
 		}
 
 		/* Due to IPv6 header, MSS needs to be downgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_decrease_gso_size(shinfo, len_diff);
-
+		skb_decrease_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
@@ -3278,7 +3276,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags)
 	return 0;
 }
 
-static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags)
+static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
 {
 	const u32 len_diff = sizeof(struct ipv6hdr) - sizeof(struct iphdr);
 	u32 off = skb_mac_header_len(skb);
@@ -3307,9 +3305,7 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags)
 		}
 
 		/* Due to IPv4 header, MSS can be upgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_increase_gso_size(shinfo, len_diff);
-
+		skb_increase_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
@@ -3321,17 +3317,17 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags)
 	return 0;
 }
 
-static int bpf_skb_proto_xlat(struct sk_buff *skb, __be16 to_proto, u64 flags)
+static int bpf_skb_proto_xlat(struct sk_buff *skb, __be16 to_proto)
 {
 	__be16 from_proto = skb->protocol;
 
 	if (from_proto == htons(ETH_P_IP) &&
 	      to_proto == htons(ETH_P_IPV6))
-		return bpf_skb_proto_4_to_6(skb, flags);
+		return bpf_skb_proto_4_to_6(skb);
 
 	if (from_proto == htons(ETH_P_IPV6) &&
 	      to_proto == htons(ETH_P_IP))
-		return bpf_skb_proto_6_to_4(skb, flags);
+		return bpf_skb_proto_6_to_4(skb);
 
 	return -ENOTSUPP;
 }
@@ -3341,7 +3337,7 @@ BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto,
 {
 	int ret;
 
-	if (unlikely(flags & ~(BPF_F_ADJ_ROOM_FIXED_GSO)))
+	if (unlikely(flags))
 		return -EINVAL;
 
 	/* General idea is that this helper does the basic groundwork
@@ -3361,7 +3357,7 @@ BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto,
 	 * that. For offloads, we mark packet as dodgy, so that headers
 	 * need to be verified first.
 	 */
-	ret = bpf_skb_proto_xlat(skb, proto, flags);
+	ret = bpf_skb_proto_xlat(skb, proto);
 	bpf_compute_data_pointers(skb);
 	return ret;
 }
-- 
2.32.0.272.g935e593368-goog

[PATCH bpf-next v2 2/4] bpf: do not change gso_size during bpf_skb_change_proto()

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

This is technically a backwards incompatible change in behaviour,
but I'm going to argue that it is very unlikely to break things,
and likely to fix *far* more then it breaks.

In no particular order, various reasons follow:

(a) I've long had a bug assigned to myself to debug a super rare kernel
crash on Android Pixel phones which can (per stacktrace) be traced back
to bpf clat ipv6 to ipv4 protocol conversion causing some sort of ugly
failure much later on during transmit deep in the GSO engine, AFAICT
precisely because of this change to gso_size, though I've never been able
to manually reproduce it.
I believe it may be related to the particular network offload support
of attached usb ethernet dongle being used for tethering off of an
IPv6-only cellular connection.  The reason might be we end up with more
segments than max permitted, or with a gso packet with only one segment...
(either way we break some assumption and hit a BUG_ON)

(b) There is no check that the gso_size is > 20 when reducing it by 20,
so we might end up with a negative (or underflowing) gso_size or
a gso_size of 0.  This can't possibly be good.
Indeed this is probably somehow exploitable (or at least can result
in a kernel crash) by delivering crafted packets and perhaps triggering
an infinite loop or a divide by zero...
As a reminder: gso_size (mss) is related to mtu, but not directly
derived from it: gso_size/mss may be significantly smaller then
one would get by deriving from local mtu.  And on some nics (which
do loose mtu checking on receive, it may even potentially be larger,
for example my work pc with 1500 mtu can receive 1520 byte frames
[and sometimes does due to bugs in a vendor plat46 implementation]).
Indeed even just going from 21 to 1 is potentially problematic because
it increases the number of segments by a factor of 21 (think DoS,
or some other crash due to too many segments).

(c) It's always safe to not increase the gso_size, because it doesn't
result in the max packet size increasing.  So the skb_increase_gso_size()
call was always unnecessary for correctness (and outright undesirable, see
later).  As such the only part which is potentially dangerous (ie. could
cause backwards compatibility issues) is the removal of the
skb_decrease_gso_size() call.

(d) If the packets are ultimately destined to the local device, then
there is absolutely no benefit to playing around with gso_size.
It only matters if the packets will egress the device.  ie. we're
either forwarding, or transmitting from the device.

(e) This logic only triggers for packets which are GSO.  It does not
trigger for skbs which are not GSO.  It will not convert a non-GSO mtu
sized packet into a GSO packet (and you don't even know what the mtu is,
so you can't even fix it).  As such your transmit path must *already* be
able to handle an mtu 20 bytes larger then your receive path (for ipv4
to ipv6 translation) - and indeed 28 bytes larger due to ipv4 fragments.
Thus removing the skb_decrease_gso_size() call doesn't actually increase
the size of the packets your transmit side must be able to handle.
ie. to handle non-GSO max-mtu packets, the ipv4/ipv6 device/route mtus
must already be set correctly.  Since for example with an ipv4 egress mtu
of 1500, ipv4 to ipv6 translation will already build 1520 byte ipv6 frames,
so you need a 1520 byte device mtu.  This means if your ipv6 device's
egress mtu is 1280, your ipv4 route must be 1260 (and actually 1252,
because of the need to handle fragments).  This is to handle normal non-GSO
packets.  Thus the reduction is simply not needed for GSO packets,
because when they're correctly built, they will already be the right size.

(f) TSO/GSO should be able to exactly undo GRO: the number of packets
(TCP segments) should not be modified, so that TCP's mss counting works
correctly (this matters for congestion control).
If protocol conversion changes the gso_size, then the number of TCP segments
may increase or decrease.  Packet loss after protocol conversion can result
in partial loss of mss segments that the sender sent.  How's the sending
TCP stack going to react to receiving ACKs/SACKs in the middle of the
segments it sent?

(g) skb_{decrease,increase}_gso_size() are already no-ops for GSO_BY_FRAGS
case (besides triggering WARN_ON_ONCE). This means you already cannot
guarantee that gso_size (and thus resulting packet mtu) is changed.
ie. you must assume it won't be changed.

(h) changing gso_size is outright buggy for UDP GSO packets, where framing
matters (I believe that's also the case for SCTP, but it's already excluded
by [g]).  So the only remaining case is TCP, which also doesn't want it
(see [f]).

(i) see also the reasoning on the previous attempt at fixing this
(commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e) which shows
that the current behaviour causes TCP packet loss:

  In the forwarding path GRO -> BPF 6 to 4 -> GSO for TCP traffic, the
  coalesced packet payload can be > MSS, but < MSS + 20.

  bpf_skb_proto_6_to_4() will upgrade the MSS and it can be > the payload
  length. After then tcp_gso_segment checks for the payload length if it
  is <= MSS. The condition is causing the packet to be dropped.

  tcp_gso_segment():
    [...]
    mss = skb_shinfo(skb)->gso_size;
    if (unlikely(skb->len <= mss)) goto out;
    [...]

Thus changing the gso_size is simply a very bad idea.
Increasing is unnecessary and buggy, and decreasing can go negative.

Cc: Dongseok Yi <dseok.yi@samsung.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Willem de Bruijn <willemb@google.com>
Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/core/filter.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 65ab4e21c087..6541358a770b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3263,8 +3263,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 			shinfo->gso_type |=  SKB_GSO_TCPV6;
 		}
 
-		/* Due to IPv6 header, MSS needs to be downgraded. */
-		skb_decrease_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
@@ -3304,8 +3302,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
 			shinfo->gso_type |=  SKB_GSO_TCPV4;
 		}
 
-		/* Due to IPv4 header, MSS can be upgraded. */
-		skb_increase_gso_size(shinfo, len_diff);
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
 		shinfo->gso_segs = 0;
-- 
2.32.0.272.g935e593368-goog

[PATCH bpf-next v2 3/4] bpf: support all gso types in bpf_skb_change_proto()

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Since we no longer modify gso_size, it is now theoretically
safe to not set SKB_GSO_DODGY and reset gso_segs to zero.

This also means the skb_is_gso_tcp() check should no
longer be necessary.

Unfortunately we cannot remove the skb_{decrease,increase}_gso_size()
helpers, as they are still used elsewhere:
  bpf_skb_net_grow() without BPF_F_ADJ_ROOM_FIXED_GSO
  bpf_skb_net_shrink() without BPF_F_ADJ_ROOM_FIXED_GSO
and
  net/core/lwt_bpf.c's handle_gso_type()

Cc: Dongseok Yi <dseok.yi@samsung.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/core/filter.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 6541358a770b..8f05498f497e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3241,9 +3241,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 	u32 off = skb_mac_header_len(skb);
 	int ret;
 
-	if (skb_is_gso(skb) && !skb_is_gso_tcp(skb))
-		return -ENOTSUPP;
-
 	ret = skb_cow(skb, len_diff);
 	if (unlikely(ret < 0))
 		return ret;
@@ -3255,17 +3252,11 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* SKB_GSO_TCPV4 needs to be changed into
-		 * SKB_GSO_TCPV6.
-		 */
+		/* SKB_GSO_TCPV4 needs to be changed into SKB_GSO_TCPV6. */
 		if (shinfo->gso_type & SKB_GSO_TCPV4) {
 			shinfo->gso_type &= ~SKB_GSO_TCPV4;
 			shinfo->gso_type |=  SKB_GSO_TCPV6;
 		}
-
-		/* Header must be checked, and gso_segs recomputed. */
-		shinfo->gso_type |= SKB_GSO_DODGY;
-		shinfo->gso_segs = 0;
 	}
 
 	skb->protocol = htons(ETH_P_IPV6);
@@ -3280,9 +3271,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
 	u32 off = skb_mac_header_len(skb);
 	int ret;
 
-	if (skb_is_gso(skb) && !skb_is_gso_tcp(skb))
-		return -ENOTSUPP;
-
 	ret = skb_unclone(skb, GFP_ATOMIC);
 	if (unlikely(ret < 0))
 		return ret;
@@ -3294,17 +3282,11 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
-		/* SKB_GSO_TCPV6 needs to be changed into
-		 * SKB_GSO_TCPV4.
-		 */
+		/* SKB_GSO_TCPV6 needs to be changed into SKB_GSO_TCPV4. */
 		if (shinfo->gso_type & SKB_GSO_TCPV6) {
 			shinfo->gso_type &= ~SKB_GSO_TCPV6;
 			shinfo->gso_type |=  SKB_GSO_TCPV4;
 		}
-
-		/* Header must be checked, and gso_segs recomputed. */
-		shinfo->gso_type |= SKB_GSO_DODGY;
-		shinfo->gso_segs = 0;
 	}
 
 	skb->protocol = htons(ETH_P_IP);
-- 
2.32.0.272.g935e593368-goog

[PATCH bpf-next v2 4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

This is to more closely match behaviour of bpf_skb_change_proto()
which now does not adjust gso_size, and thus thoretically supports
all gso types, and does not need to set SKB_GSO_DODGY nor reset
gso_segs to zero.

Something similar should probably be done with bpf_skb_net_grow(),
but that code scares me.

Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/core/filter.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 8f05498f497e..faf2bae0309b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3506,11 +3506,10 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff,
 			       BPF_F_ADJ_ROOM_NO_CSUM_RESET)))
 		return -EINVAL;
 
-	if (skb_is_gso(skb) && !skb_is_gso_tcp(skb)) {
-		/* udp gso_size delineates datagrams, only allow if fixed */
-		if (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) ||
-		    !(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			return -ENOTSUPP;
+	if (skb_is_gso(skb) &&
+	    !skb_is_gso_tcp(skb) &&
+	    !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+		return -ENOTSUPP;
 	}
 
 	ret = skb_unclone(skb, GFP_ATOMIC);
@@ -3521,12 +3520,11 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff,
 	if (unlikely(ret < 0))
 		return ret;
 
-	if (skb_is_gso(skb)) {
+	if (skb_is_gso(skb) && !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 
 		/* Due to header shrink, MSS can be upgraded. */
-		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
-			skb_increase_gso_size(shinfo, len_diff);
+		skb_increase_gso_size(shinfo, len_diff);
 
 		/* Header must be checked, and gso_segs recomputed. */
 		shinfo->gso_type |= SKB_GSO_DODGY;
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH bpf-next v2 4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO

[Daniel Borkmann]

On 6/17/21 2:09 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> This is to more closely match behaviour of bpf_skb_change_proto()
> which now does not adjust gso_size, and thus thoretically supports
> all gso types, and does not need to set SKB_GSO_DODGY nor reset
> gso_segs to zero.
> 
> Something similar should probably be done with bpf_skb_net_grow(),
> but that code scares me.

Took in all except this one, would be good to have a complete solution for
both bpf_skb_net_{shrink,grow}(). If you don't have the cycles, I'll look
into it.

Thanks,
Daniel

Re: [PATCH bpf-next v2 4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO

[Maciej Żenczykowski]

On Thu, Jun 24, 2021 at 7:05 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 6/17/21 2:09 AM, Maciej Żenczykowski wrote:
> > From: Maciej Żenczykowski <maze@google.com>
> >
> > This is to more closely match behaviour of bpf_skb_change_proto()
> > which now does not adjust gso_size, and thus thoretically supports
> > all gso types, and does not need to set SKB_GSO_DODGY nor reset
> > gso_segs to zero.
> >
> > Something similar should probably be done with bpf_skb_net_grow(),
> > but that code scares me.
>
> Took in all except this one, would be good to have a complete solution for
> both bpf_skb_net_{shrink,grow}(). If you don't have the cycles, I'll look
> into it.
>
> Thanks,
> Daniel

I very much don't understand all the complexities of all the different
encap/tunneling
stuff that is handled in ..._grow().  In principle I think changing
the gso_size is
probably a bad idea in general, but I'm not at all sure that's a
change we can make now,
without breaking backward compatibility with some userspace somewhere
(not Android
though, we don't currently use either of these helpers yet) or causing
other trouble.

I'd love it if there was some truly good documentation of how all the
fields/offloads
in an skb interact, as I find myself constantly having to figure this
out via code examination,
and never feel like I really truly understand things (or perhaps some
helper function that would
'validate' an skb as well formed, ideally in debug mode we could call
it both before and after
a bpf program mucks with things and check it still passes).
I'm not sure who would be the expert here... you? Willem? Tom? someone else?
As such I'll leave this up to one of you.

I sent the patch for ..._shrink() because that one seemed simple enough.
(I don't really understand why shrink is so much simpler than grow...)

What I will try to send you is an extension to 4<->6 protocol
conversion to deal with the extra
8 bytes of overhead in an ipv6 fragment (48 instead of 40 byte header
converted to/from 20 byte ipv4 frag header).
Though this isn't something I even have ready atm, it's just on a todo
list as a relatively unimportant thing.

- Maciej