Messages in this thread | | | Subject | Re: [PATCH v2 net-next] ipv6: avoid atomic fragment on GSO packets | From | Alexander H Duyck <> | Date | Mon, 16 Oct 2023 14:00:28 -0700 |
| |
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)
| |