| Message ID | ZS1/qtr0dZJ35VII@debian.debian (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] ipv6: avoid atomic fragment on GSO packets | expand |
On Mon, Oct 16, 2023 at 1:23 PM Yan Zhai <yan@cloudflare.com> wrote: > > GSO packets can contain a trailing segment that is smaller than > gso_size. When examining the dst MTU for such packet, if its gso_size is > too large, then all segments would be fragmented. However, there is a > good chance the trailing segment has smaller actual size than both > gso_size as well as the MTU, which leads to an "atomic fragment". It is > 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]. > > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > processing. It mirrors __ip_finish_output logic now. Add an extra check > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > sending PTB packets for MTU < 1280"). > > Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1] > Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing") > Suggested-by: Florian Westphal <fw@strlen.de> > Reported-by: David Wragg <dwragg@cloudflare.com> > Signed-off-by: Yan Zhai <yan@cloudflare.com> > --- Forgot to add v1 thread: https://lore.kernel.org/lkml/20231002171146.GB9274@breakpoint.cc/. It was wrongly implemented though without considering max_frag_size for non-GSO packets though, so not really useful in fact. > net/ipv6/ip6_output.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index a471c7e91761..1de6f3c11655 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -162,7 +162,14 @@ 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 might be smaller than actual MTU. Adding > + * a fragment header to it would produce an "atomic fragment", > + * which is considered harmful (RFC-8021) > + */ > + err = segs->len > mtu ? > + ip6_fragment(net, sk, segs, ip6_finish_output2) : > + ip6_finish_output2(net, sk, segs); > + > if (err && ret == 0) > ret = err; > } > @@ -170,10 +177,19 @@ 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; > - > #if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM) > /* Policy lookup after SNAT yielded a new policy */ > if (skb_dst(skb)->xfrm) { > @@ -183,17 +199,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)) || > - dst_allfrag(skb_dst(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 >
On Mon, 2023-10-16 at 11:23 -0700, Yan Zhai wrote: > GSO packets can contain a trailing segment that is smaller than > gso_size. When examining the dst MTU for such packet, if its gso_size is > too large, then all segments would be fragmented. However, there is a > good chance the trailing segment has smaller actual size than both > gso_size as well as the MTU, which leads to an "atomic fragment". It is > 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]. > > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > processing. It mirrors __ip_finish_output logic now. Add an extra check > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > sending PTB packets for MTU < 1280"). > > Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1] > Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing") > Suggested-by: Florian Westphal <fw@strlen.de> > Reported-by: David Wragg <dwragg@cloudflare.com> > Signed-off-by: Yan Zhai <yan@cloudflare.com> > --- > net/ipv6/ip6_output.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index a471c7e91761..1de6f3c11655 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -162,7 +162,14 @@ 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 might be smaller than actual MTU. Adding > + * a fragment header to it would produce an "atomic fragment", > + * which is considered harmful (RFC-8021) > + */ > + err = segs->len > mtu ? > + ip6_fragment(net, sk, segs, ip6_finish_output2) : > + ip6_finish_output2(net, sk, segs); > + > if (err && ret == 0) > ret = err; > } > @@ -170,10 +177,19 @@ 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); If we are sending fakejumbo or have a frame that doesn't pass the muster it is just going immediately to ip6_finish_output. I think the checks that you removed are needed to keep the socket from getting stuck sending frames that will probably be discarded. > + > + 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; > - This blank line can probably be left there to separate variable declarations from code. > #if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM) > /* Policy lookup after SNAT yielded a new policy */ > if (skb_dst(skb)->xfrm) { > @@ -183,17 +199,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)) || > - dst_allfrag(skb_dst(skb)) || > + if (skb->len > mtu || This change looks a bit too aggressive to me. Basically if the frame is gso you now bypass the ip6_fragment entirely and are ignoring the dst_allfrag and frag_max_size case below. See the fail_toobig code in ip6_fragment. > (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)
On Mon, Oct 16, 2023 at 4:00 PM Alexander H Duyck <alexander.duyck@gmail.com> wrote: > > On Mon, 2023-10-16 at 11:23 -0700, Yan Zhai wrote: > > GSO packets can contain a trailing segment that is smaller than > > gso_size. When examining the dst MTU for such packet, if its gso_size is > > too large, then all segments would be fragmented. However, there is a > > good chance the trailing segment has smaller actual size than both > > gso_size as well as the MTU, which leads to an "atomic fragment". It is > > 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]. > > > > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > > processing. It mirrors __ip_finish_output logic now. Add an extra check > > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > > sending PTB packets for MTU < 1280"). > > > > Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1] > > Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing") > > Suggested-by: Florian Westphal <fw@strlen.de> > > Reported-by: David Wragg <dwragg@cloudflare.com> > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > --- > > net/ipv6/ip6_output.c | 33 +++++++++++++++++++++++---------- > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > > index a471c7e91761..1de6f3c11655 100644 > > --- a/net/ipv6/ip6_output.c > > +++ b/net/ipv6/ip6_output.c > > @@ -162,7 +162,14 @@ 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 might be smaller than actual MTU. Adding > > + * a fragment header to it would produce an "atomic fragment", > > + * which is considered harmful (RFC-8021) > > + */ > > + err = segs->len > mtu ? > > + ip6_fragment(net, sk, segs, ip6_finish_output2) : > > + ip6_finish_output2(net, sk, segs); > > + > > if (err && ret == 0) > > ret = err; > > } > > @@ -170,10 +177,19 @@ 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); > > If we are sending fakejumbo or have a frame that doesn't pass the > muster it is just going immediately to ip6_finish_output. I think the > checks that you removed are needed to keep the socket from getting > stuck sending frames that will probably be discarded. > Hi Alexander, Thanks for the feedback! But I am not sure I follow the situation you mentioned here. If it is a fake jumbo but non GSO packet, it won't enter ip6_finish_output_gso. What I am really skipping are the dst_allfrag and frag_max_size checks on GSO packets, and dst_allfrag on non-GSO packets. As to dst_allfrag, I looked back at the case when this was added: https://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg03399.html The actual feature was set only when a PMTU message carries a value smaller than 1280 byte. But the main line kernel just drops such messages now since the commit I pointed to in the change log (which makes sense because the feature was set based on old RFC-2460 guidelines, and those have been deprecated in RFC-8200). Iproute2 also doesn't expose this option as well. Is there any case that I am not aware of here that still relies on it? For frag_max_size, I might be wrong but to my best knowledge it only applies when netfilter defrags packets. However, when dealing with fragments, both local output and GRO code won't produce GSO packets in the first place. Similarly, if we look at IPv4 implementation, it also does not consider frag_max_size in GSO handling. So I intentionally skip this for GSO packets in the change. WDYT? > > + > > + 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; > > - > > This blank line can probably be left there to separate variable > declarations from code. > my bad, should not have included it. I'll revise this. thanks Yan > > #if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM) > > /* Policy lookup after SNAT yielded a new policy */ > > if (skb_dst(skb)->xfrm) { > > @@ -183,17 +199,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)) || > > - dst_allfrag(skb_dst(skb)) || > > + if (skb->len > mtu || > > This change looks a bit too aggressive to me. Basically if the frame is > gso you now bypass the ip6_fragment entirely and are ignoring the > dst_allfrag and frag_max_size case below. See the fail_toobig code in > ip6_fragment. > > > (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) >
On Mon, Oct 16, 2023 at 2:51 PM Yan Zhai <yan@cloudflare.com> wrote: > > On Mon, Oct 16, 2023 at 4:00 PM Alexander H Duyck > <alexander.duyck@gmail.com> wrote: > > > > On Mon, 2023-10-16 at 11:23 -0700, Yan Zhai wrote: > > > GSO packets can contain a trailing segment that is smaller than > > > gso_size. When examining the dst MTU for such packet, if its gso_size is > > > too large, then all segments would be fragmented. However, there is a > > > good chance the trailing segment has smaller actual size than both > > > gso_size as well as the MTU, which leads to an "atomic fragment". It is > > > 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]. > > > > > > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > > > processing. It mirrors __ip_finish_output logic now. Add an extra check > > > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > > > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > > > sending PTB packets for MTU < 1280"). > > > > > > Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1] > > > Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing") > > > Suggested-by: Florian Westphal <fw@strlen.de> > > > Reported-by: David Wragg <dwragg@cloudflare.com> > > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > > --- > > > net/ipv6/ip6_output.c | 33 +++++++++++++++++++++++---------- > > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > > > index a471c7e91761..1de6f3c11655 100644 > > > --- a/net/ipv6/ip6_output.c > > > +++ b/net/ipv6/ip6_output.c > > > @@ -162,7 +162,14 @@ 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 might be smaller than actual MTU. Adding > > > + * a fragment header to it would produce an "atomic fragment", > > > + * which is considered harmful (RFC-8021) > > > + */ > > > + err = segs->len > mtu ? > > > + ip6_fragment(net, sk, segs, ip6_finish_output2) : > > > + ip6_finish_output2(net, sk, segs); > > > + > > > if (err && ret == 0) > > > ret = err; > > > } > > > @@ -170,10 +177,19 @@ 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); > > > > If we are sending fakejumbo or have a frame that doesn't pass the > > muster it is just going immediately to ip6_finish_output. I think the > > checks that you removed are needed to keep the socket from getting > > stuck sending frames that will probably be discarded. > > > > Hi Alexander, > > Thanks for the feedback! But I am not sure I follow the situation you > mentioned here. If it is a fake jumbo but non GSO packet, it won't > enter ip6_finish_output_gso. What I am really skipping are the > dst_allfrag and frag_max_size checks on GSO packets, and dst_allfrag > on non-GSO packets. > > As to dst_allfrag, I looked back at the case when this was added: > > https://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg03399.html > > The actual feature was set only when a PMTU message carries a value > smaller than 1280 byte. But the main line kernel just drops such > messages now since the commit I pointed to in the change log (which > makes sense because the feature was set based on old RFC-2460 > guidelines, and those have been deprecated in RFC-8200). Iproute2 also > doesn't expose this option as well. Is there any case that I am not > aware of here that still relies on it? > > For frag_max_size, I might be wrong but to my best knowledge it only > applies when netfilter defrags packets. However, when dealing with > fragments, both local output and GRO code won't produce GSO packets in > the first place. Similarly, if we look at IPv4 implementation, it also > does not consider frag_max_size in GSO handling. So I intentionally > skip this for GSO packets in the change. WDYT? I am not certain. Just looking at the code it seems like there were a number of corner cases handled that this is getting rid of the code for. Specifically my main concern is GSO being enabled for a path where the MTU is incorrect due to something such as a tunnel being between the system and the endpoint. In such a case it would normally send back an ICMP message triggering a path MTU update which would then have to ripple through. I'm not an IPv6 expert though so perhaps I will leave that for somebody else to provide feedback on.
Yan Zhai <yan@cloudflare.com> wrote: > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > processing. It mirrors __ip_finish_output logic now. Add an extra check > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > sending PTB packets for MTU < 1280"). > - if ((skb->len > mtu && !skb_is_gso(skb)) || > - dst_allfrag(skb_dst(skb)) || My preference is to first remove dst_allfrag, i.e. do this in a separate change.
On Tue, Oct 17, 2023 at 3:02 PM Florian Westphal <fw@strlen.de> wrote: > > Yan Zhai <yan@cloudflare.com> wrote: > > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > > processing. It mirrors __ip_finish_output logic now. Add an extra check > > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > > sending PTB packets for MTU < 1280"). > > > > - if ((skb->len > mtu && !skb_is_gso(skb)) || > > - dst_allfrag(skb_dst(skb)) || > > My preference is to first remove dst_allfrag, i.e. do this in > a separate change. You mean completely removing all dst_allfrag references and related stuff such like IP cork flags/socket flags? I was debating, it might be cleaner that way but it does not fit so well with the subject of this patch. I can open a new patchset to clean that up separately. For this one, I guess I can keep dst_allfrag for now and come back with a V3. Does that sound good to you? Yan
On Tue, Oct 17, 2023 at 9:42 PM Yan Zhai <yan@cloudflare.com> wrote: > > On Tue, Oct 17, 2023 at 3:02 PM Florian Westphal <fw@strlen.de> wrote: > > > > Yan Zhai <yan@cloudflare.com> wrote: > > > Refactor __ip6_finish_output code to separate GSO and non-GSO packet > > > processing. It mirrors __ip_finish_output logic now. Add an extra check > > > in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag > > > check, which is no longer true since commit 9d289715eb5c ("ipv6: stop > > > sending PTB packets for MTU < 1280"). > > > > > > > - if ((skb->len > mtu && !skb_is_gso(skb)) || > > > - dst_allfrag(skb_dst(skb)) || > > > > My preference is to first remove dst_allfrag, i.e. do this in > > a separate change. > > You mean completely removing all dst_allfrag references and related > stuff such like IP cork flags/socket flags? I was debating, it might > be cleaner that way but it does not fit so well with the subject of > this patch. I can open a new patchset to clean that up separately. For > this one, I guess I can keep dst_allfrag for now and come back with a > V3. Does that sound good to you? The second paragraph in the commit message really makes clear that this combines three changes in one patch. Of which the largest one in terms of code churn is supposed to be a NOOP. Separating into three patches will make all three more clear. They can be pushed as one series, conceivably.
On Tue, Oct 17, 2023 at 8:58 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > The second paragraph in the commit message really makes > clear that this combines three changes in one patch. Of which > the largest one in terms of code churn is supposed to be a > NOOP. > > Separating into three patches will make all three more clear. > They can be pushed as one series, conceivably. Thanks for clarifying. In that case I am just gonna remove dst_allfrag in ip6_finish_output rather than everywhere for the series. Remaining cleanup can come later then. In fact there were some past considerations already on this: https://lkml.kernel.org/netdev/1335348157.3274.30.camel@edumazet-glaptop/ Could be a good base to work on later. Yan
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index a471c7e91761..1de6f3c11655 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -162,7 +162,14 @@ 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 might be smaller than actual MTU. Adding + * a fragment header to it would produce an "atomic fragment", + * which is considered harmful (RFC-8021) + */ + err = segs->len > mtu ? + ip6_fragment(net, sk, segs, ip6_finish_output2) : + ip6_finish_output2(net, sk, segs); + if (err && ret == 0) ret = err; } @@ -170,10 +177,19 @@ 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; - #if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM) /* Policy lookup after SNAT yielded a new policy */ if (skb_dst(skb)->xfrm) { @@ -183,17 +199,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)) || - dst_allfrag(skb_dst(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)
GSO packets can contain a trailing segment that is smaller than gso_size. When examining the dst MTU for such packet, if its gso_size is too large, then all segments would be fragmented. However, there is a good chance the trailing segment has smaller actual size than both gso_size as well as the MTU, which leads to an "atomic fragment". It is 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]. Refactor __ip6_finish_output code to separate GSO and non-GSO packet processing. It mirrors __ip_finish_output logic now. Add an extra check in GSO handling to avoid atomic fragments. Lastly, drop dst_allfrag check, which is no longer true since commit 9d289715eb5c ("ipv6: stop sending PTB packets for MTU < 1280"). Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1] Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing") Suggested-by: Florian Westphal <fw@strlen.de> Reported-by: David Wragg <dwragg@cloudflare.com> Signed-off-by: Yan Zhai <yan@cloudflare.com> --- net/ipv6/ip6_output.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)