[PATCH v3 net-next 0/3] ipv6: avoid atomic fragment on GSO output

[PATCH v3 net-next 0/3] ipv6: avoid atomic fragment on GSO output

[Yan Zhai]

When the ipv6 stack output a GSO packet, if its gso_size is larger than
dst MTU, then all segments would be fragmented. However, it is possible
for a GSO packet to have a trailing segment with smaller actual size
than both gso_size as well as the MTU, which leads to an "atomic
fragment". Atomic fragments are considered harmful in RFC-8021. An
Existing report from APNIC also shows that atomic fragments are more
likely to be dropped even it is equivalent to a no-op [1].

The series contains following changes on IPv6 output:
* drop dst_allfrag check, which is always false now
* refactor __ip6_finish_output code to separate GSO and non-GSO packet processing,
  mirroring IPv4 side logic
* avoid generating atomic fragment on GSO packets

Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1]

change log:
V2 -> V3: split the changes to separate commits as Willem de Bruijn suggested
V1 is incorrect and omitted

V2: https://lore.kernel.org/netdev/ZS1%2Fqtr0dZJ35VII@debian.debian/

Yan Zhai (3):
  ipv6: remove dst_allfrag test on ipv6 output
  ipv6: refactor ip6_finish_output for GSO handling
  ipv6: avoid atomic fragment on GSO packets

 net/ipv6/ip6_output.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

-- 
2.30.2

[PATCH v3 net-next 1/3] ipv6: remove dst_allfrag test on ipv6 output

[Yan Zhai]

dst_allfrag was added before the first git commit:

https://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg03399.html

The feature would send packets to the fragmentation path if a box
receives a PMTU value with less than 1280 byte. However, since commit
9d289715eb5c ("ipv6: stop sending PTB packets for MTU < 1280"), such
message would be simply discarded. The feature flag is neither supported
in iproute2 utility. In theory one can still manipulate it with direct
netlink message, but it is not ideal because it was based on obsoleted
guidance of RFC-2460 (replaced by RFC-8200).

The feature test would always return false at the moment, so remove it
from the output path.

Signed-off-by: Yan Zhai <yan@cloudflare.com>
---
 net/ipv6/ip6_output.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a471c7e91761..ae87a3817d4a 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -189,7 +189,6 @@ static int __ip6_finish_output(struct net *net, struct sock *sk, struct sk_buff
 		return ip6_finish_output_gso_slowpath_drop(net, sk, skb, mtu);
 
 	if ((skb->len > mtu && !skb_is_gso(skb)) ||
-	    dst_allfrag(skb_dst(skb)) ||
 	    (IP6CB(skb)->frag_max_size && skb->len > IP6CB(skb)->frag_max_size))
 		return ip6_fragment(net, sk, skb, ip6_finish_output2);
 	else
-- 
2.30.2

Re: [PATCH v3 net-next 1/3] ipv6: remove dst_allfrag test on ipv6 output

[Eric Dumazet]

On Fri, Oct 20, 2023 at 7:32 AM Yan Zhai <yan@cloudflare.com> wrote:
>
> dst_allfrag was added before the first git commit:
>
> https://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg03399.html
>
> The feature would send packets to the fragmentation path if a box
> receives a PMTU value with less than 1280 byte. However, since commit
> 9d289715eb5c ("ipv6: stop sending PTB packets for MTU < 1280"), such
> message would be simply discarded. The feature flag is neither supported
> in iproute2 utility. In theory one can still manipulate it with direct
> netlink message, but it is not ideal because it was based on obsoleted
> guidance of RFC-2460 (replaced by RFC-8200).
>
> The feature test would always return false at the moment, so remove it
> from the output path.

What about other callers of dst_allfrag() ?

This feature seems broken atm.

Re: [PATCH v3 net-next 1/3] ipv6: remove dst_allfrag test on ipv6 output

[Yan Zhai]

On Fri, Oct 20, 2023 at 1:06 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Oct 20, 2023 at 7:32 AM Yan Zhai <yan@cloudflare.com> wrote:
> >
> > dst_allfrag was added before the first git commit:
> >
> > https://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg03399.html
> >
> > The feature would send packets to the fragmentation path if a box
> > receives a PMTU value with less than 1280 byte. However, since commit
> > 9d289715eb5c ("ipv6: stop sending PTB packets for MTU < 1280"), such
> > message would be simply discarded. The feature flag is neither supported
> > in iproute2 utility. In theory one can still manipulate it with direct
> > netlink message, but it is not ideal because it was based on obsoleted
> > guidance of RFC-2460 (replaced by RFC-8200).
> >
> > The feature test would always return false at the moment, so remove it
> > from the output path.
>
> What about other callers of dst_allfrag() ?
>
> This feature seems broken atm.

It is broken as far as I can tell. The reason I removed just one
caller here is to keep the code simpler and consistent. If I don't do
so, I ought to test it for both GSO fast path and slow path to be
logically consistent. Seems an overkill to me. For the removal of the
rest, I'd hope it could come in as a standalone patch(set) because it
is not just callers but also those unnecessary flags and tests on IP
corks and sockets, not quite aligned with this patch's intention. I
noted you have drafted something like this in the past:

https://lkml.kernel.org/netdev/1335348157.3274.30.camel@edumazet-glaptop/

I guess it might be a good base point to work on as a new patch(set)?
What's your call on this?

Yan

Re: [PATCH v3 net-next 1/3] ipv6: remove dst_allfrag test on ipv6 output

[Eric Dumazet]

On Fri, Oct 20, 2023 at 8:39 AM Yan Zhai <yan@cloudflare.com> wrote:
>
> On Fri, Oct 20, 2023 at 1:06 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Oct 20, 2023 at 7:32 AM Yan Zhai <yan@cloudflare.com> wrote:
> > >
> > > dst_allfrag was added before the first git commit:
> > >
> > > https://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg03399.html
> > >
> > > The feature would send packets to the fragmentation path if a box
> > > receives a PMTU value with less than 1280 byte. However, since commit
> > > 9d289715eb5c ("ipv6: stop sending PTB packets for MTU < 1280"), such
> > > message would be simply discarded. The feature flag is neither supported
> > > in iproute2 utility. In theory one can still manipulate it with direct
> > > netlink message, but it is not ideal because it was based on obsoleted
> > > guidance of RFC-2460 (replaced by RFC-8200).
> > >
> > > The feature test would always return false at the moment, so remove it
> > > from the output path.
> >
> > What about other callers of dst_allfrag() ?
> >
> > This feature seems broken atm.
>
> It is broken as far as I can tell. The reason I removed just one
> caller here is to keep the code simpler and consistent. If I don't do
> so, I ought to test it for both GSO fast path and slow path to be
> logically consistent. Seems an overkill to me. For the removal of the
> rest, I'd hope it could come in as a standalone patch(set) because it
> is not just callers but also those unnecessary flags and tests on IP
> corks and sockets, not quite aligned with this patch's intention. I
> noted you have drafted something like this in the past:
>
> https://lkml.kernel.org/netdev/1335348157.3274.30.camel@edumazet-glaptop/
>
> I guess it might be a good base point to work on as a new patch(set)?
> What's your call on this?
>

I am about to send a TCP patch series to enable usec TSval (instead of ms),
and was planning to use a new RTAX_FEATURE_TCP_USEC_TS.

I also noticed that iproute2 was not supporting RTAX_FEATURE_ALLFRAG,
so we might kill it completely ?

commit b258c87639f77d772c077a4e45dad602c62c9f1c
Author: Eric Dumazet <edumazet@google.com>
Date:   Wed Oct 18 09:33:38 2023 +0000

    tcp: add RTAX_FEATURE_TCP_USEC_TS

    This new dst feature flag will be used to allow TCP to use usec
    based timestamps instead of msec ones.

    ip route .... feature tcp_usec_ts

    Also document that RTAX_FEATURE_SACK and RTAX_FEATURE_TIMESTAMP
    are unused.

    Note that iproute2 does yet support RTAX_FEATURE_ALLFRAG ?

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

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 51c13cf9c5aee4a2d1ab33c1a89043383d67b9cf..aa2482a0614aa685590fcc73819cbe1baac63d66
100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -502,13 +502,17 @@ enum {

 #define RTAX_MAX (__RTAX_MAX - 1)

-#define RTAX_FEATURE_ECN       (1 << 0)
-#define RTAX_FEATURE_SACK      (1 << 1)
-#define RTAX_FEATURE_TIMESTAMP (1 << 2)
-#define RTAX_FEATURE_ALLFRAG   (1 << 3)
-
-#define RTAX_FEATURE_MASK      (RTAX_FEATURE_ECN | RTAX_FEATURE_SACK | \
-                                RTAX_FEATURE_TIMESTAMP | RTAX_FEATURE_ALLFRAG)
+#define RTAX_FEATURE_ECN               (1 << 0)
+#define RTAX_FEATURE_SACK              (1 << 1) /* unused */
+#define RTAX_FEATURE_TIMESTAMP         (1 << 2) /* unused */
+#define RTAX_FEATURE_ALLFRAG           (1 << 3)
+#define RTAX_FEATURE_TCP_USEC_TS       (1 << 4)
+
+#define RTAX_FEATURE_MASK      (RTAX_FEATURE_ECN |             \
+                                RTAX_FEATURE_SACK |            \
+                                RTAX_FEATURE_TIMESTAMP |       \
+                                RTAX_FEATURE_ALLFRAG |         \
+                                RTAX_FEATURE_TCP_USEC_TS)

 struct rta_session {
        __u8    proto;

Re: [PATCH v3 net-next 1/3] ipv6: remove dst_allfrag test on ipv6 output

[Florian Westphal]

Eric Dumazet <edumazet@google.com> wrote:
> I also noticed that iproute2 was not supporting RTAX_FEATURE_ALLFRAG,
> so we might kill it completely ?

Yes, I intentionally did not expose it in iproute2.

Lets remove ALLFRAG.

Re: [PATCH v3 net-next 1/3] ipv6: remove dst_allfrag test on ipv6 output

[Yan Zhai]

On Fri, Oct 20, 2023 at 5:01 AM Florian Westphal <fw@strlen.de> wrote:
>
> Eric Dumazet <edumazet@google.com> wrote:
> > I also noticed that iproute2 was not supporting RTAX_FEATURE_ALLFRAG,
> > so we might kill it completely ?
>
> Yes, I intentionally did not expose it in iproute2.
>
> Lets remove ALLFRAG.

I will do that in V4 later today to completely clean it up then.
Always cheerful to delete code!

Yan

[PATCH v3 net-next 2/3] ipv6: refactor ip6_finish_output for GSO handling

[Yan Zhai]

Separate GSO and non-GSO packets handling to make the logic cleaner. For
GSO packets, frag_max_size check can be omitted because it is only
useful for packets defragmented by netfilter hooks. Both local output
and GRO logic won't produce GSO packets when defragment is needed. This
also mirrors what IPv4 side code is doing.

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Yan Zhai <yan@cloudflare.com>
---
 net/ipv6/ip6_output.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index ae87a3817d4a..3270d56b5c37 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -170,6 +170,16 @@ ip6_finish_output_gso_slowpath_drop(struct net *net, struct sock *sk,
 	return ret;
 }
 
+static int ip6_finish_output_gso(struct net *net, struct sock *sk,
+				 struct sk_buff *skb, unsigned int mtu)
+{
+	if (!(IP6CB(skb)->flags & IP6SKB_FAKEJUMBO) &&
+	    !skb_gso_validate_network_len(skb, mtu))
+		return ip6_finish_output_gso_slowpath_drop(net, sk, skb, mtu);
+
+	return ip6_finish_output2(net, sk, skb);
+}
+
 static int __ip6_finish_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
 	unsigned int mtu;
@@ -183,16 +193,14 @@ static int __ip6_finish_output(struct net *net, struct sock *sk, struct sk_buff
 #endif
 
 	mtu = ip6_skb_dst_mtu(skb);
-	if (skb_is_gso(skb) &&
-	    !(IP6CB(skb)->flags & IP6SKB_FAKEJUMBO) &&
-	    !skb_gso_validate_network_len(skb, mtu))
-		return ip6_finish_output_gso_slowpath_drop(net, sk, skb, mtu);
+	if (skb_is_gso(skb))
+		return ip6_finish_output_gso(net, sk, skb, mtu);
 
-	if ((skb->len > mtu && !skb_is_gso(skb)) ||
+	if (skb->len > mtu ||
 	    (IP6CB(skb)->frag_max_size && skb->len > IP6CB(skb)->frag_max_size))
 		return ip6_fragment(net, sk, skb, ip6_finish_output2);
-	else
-		return ip6_finish_output2(net, sk, skb);
+
+	return ip6_finish_output2(net, sk, skb);
 }
 
 static int ip6_finish_output(struct net *net, struct sock *sk, struct sk_buff *skb)
-- 
2.30.2

Re: [PATCH v3 net-next 2/3] ipv6: refactor ip6_finish_output for GSO handling

[Willem de Bruijn]

Yan Zhai wrote:
> Separate GSO and non-GSO packets handling to make the logic cleaner. For
> GSO packets, frag_max_size check can be omitted because it is only
> useful for packets defragmented by netfilter hooks. Both local output
> and GRO logic won't produce GSO packets when defragment is needed. This
> also mirrors what IPv4 side code is doing.
> 
> Suggested-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Yan Zhai <yan@cloudflare.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks for splitting up the series. This change alone is subtle enough
that it benefits from standing alone. I thought it was intended to be
a NOOP, but you indeed call out the frag_max_size special case.

[PATCH v3 net-next 3/3] ipv6: avoid atomic fragment on GSO packets

[Yan Zhai]

When the ipv6 stack output a GSO packet, if its gso_size is larger than
dst MTU, then all segments would be fragmented. However, it is possible
for a GSO packet to have a trailing segment with smaller actual size
than both gso_size as well as the MTU, which leads to an "atomic
fragment". Atomic fragments are considered harmful in RFC-8021. An
Existing report from APNIC also shows that atomic fragments are more
likely to be dropped even it is equivalent to a no-op [1].

Add an extra check in the GSO slow output path. For each segment from
the original over-sized packet, if it fits with the path MTU, then avoid
generating an atomic fragment.

Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1]
Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing")
Reported-by: David Wragg <dwragg@cloudflare.com>
Signed-off-by: Yan Zhai <yan@cloudflare.com>
---
 net/ipv6/ip6_output.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 3270d56b5c37..3d4e8edaa10b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -162,7 +162,13 @@ ip6_finish_output_gso_slowpath_drop(struct net *net, struct sock *sk,
 		int err;
 
 		skb_mark_not_on_list(segs);
-		err = ip6_fragment(net, sk, segs, ip6_finish_output2);
+		/* Last GSO segment can be smaller than gso_size (and MTU).
+		 * Adding a fragment header would produce an "atomic fragment",
+		 * which is considered harmful (RFC-8021). Avoid that.
+		 */
+		err = segs->len > mtu ?
+			ip6_fragment(net, sk, segs, ip6_finish_output2) :
+			ip6_finish_output2(net, sk, segs);
 		if (err && ret == 0)
 			ret = err;
 	}
-- 
2.30.2