| Message ID | ZRcOXJ0pkuph6fko@debian.debian (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv6: avoid atomic fragment on GSO packets | expand |
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". > RFC-8021 explicitly recommend to deprecate such use case. An Existing > report from APNIC also shows that atomic fragments can be dropped > unexpectedly along the path [1]. > > Add an extra check in ip6_fragment to catch all possible generation of > atomic fragments. Skip atomic header if it is called on a packet no > larger than MTU. > > 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 951ba8089b5b..42f5f68a6e24 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -854,6 +854,13 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > __be32 frag_id; > u8 *prevhdr, nexthdr = 0; > > + /* RFC-8021 recommended atomic fragments to be deprecated. Double check > + * the actual packet size before fragment it. > + */ > + mtu = ip6_skb_dst_mtu(skb); > + if (unlikely(skb->len <= mtu)) > + return output(net, sk, skb); > + This helper is also called for skbs where IP6CB(skb)->frag_max_size exceeds the MTU, so this check looks wrong to me. Same remark for dst_allfrag() check in __ip6_finish_output(), after this patch, it would be ignored. I think you should consider to first refactor __ip6_finish_output to make the existing checks more readable (e.g. handle gso vs. non-gso in separate branches) and then add the check to last seg in ip6_finish_output_gso_slowpath_drop(). Alternatively you might be able to pass more info down to ip6_fragment and move decisions there. In any case we should make same frag-or-no-frag decisions, regardless of this being the orig skb or a segmented one,
On Sat, Sep 30, 2023 at 1:09 PM Florian Westphal <fw@strlen.de> wrote: > > 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". > > RFC-8021 explicitly recommend to deprecate such use case. An Existing > > report from APNIC also shows that atomic fragments can be dropped > > unexpectedly along the path [1]. > > > > Add an extra check in ip6_fragment to catch all possible generation of > > atomic fragments. Skip atomic header if it is called on a packet no > > larger than MTU. > > > > 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 951ba8089b5b..42f5f68a6e24 100644 > > --- a/net/ipv6/ip6_output.c > > +++ b/net/ipv6/ip6_output.c > > @@ -854,6 +854,13 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > __be32 frag_id; > > u8 *prevhdr, nexthdr = 0; > > > > + /* RFC-8021 recommended atomic fragments to be deprecated. Double check > > + * the actual packet size before fragment it. > > + */ > > + mtu = ip6_skb_dst_mtu(skb); > > + if (unlikely(skb->len <= mtu)) > > + return output(net, sk, skb); > > + > > This helper is also called for skbs where IP6CB(skb)->frag_max_size > exceeds the MTU, so this check looks wrong to me. > > Same remark for dst_allfrag() check in __ip6_finish_output(), > after this patch, it would be ignored. > > I think you should consider to first refactor __ip6_finish_output to make > the existing checks more readable (e.g. handle gso vs. non-gso in separate > branches) and then add the check to last seg in > ip6_finish_output_gso_slowpath_drop(). > > Alternatively you might be able to pass more info down to > ip6_fragment and move decisions there. > > In any case we should make same frag-or-no-frag decisions, > regardless of this being the orig skb or a segmented one, To add to that: if this is a suggestion to update the algorithm to match RFC 8021, not a fix for a bug in the current implementation, then I think this should target net-next. That will also make it easier to include the kind of refactoring that Florian suggests.
On Sat, Sep 30, 2023 at 6:09 AM Florian Westphal <fw@strlen.de> wrote: > > 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". > > RFC-8021 explicitly recommend to deprecate such use case. An Existing > > report from APNIC also shows that atomic fragments can be dropped > > unexpectedly along the path [1]. > > > > Add an extra check in ip6_fragment to catch all possible generation of > > atomic fragments. Skip atomic header if it is called on a packet no > > larger than MTU. > > > > 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 951ba8089b5b..42f5f68a6e24 100644 > > --- a/net/ipv6/ip6_output.c > > +++ b/net/ipv6/ip6_output.c > > @@ -854,6 +854,13 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, > > __be32 frag_id; > > u8 *prevhdr, nexthdr = 0; > > > > + /* RFC-8021 recommended atomic fragments to be deprecated. Double check > > + * the actual packet size before fragment it. > > + */ > > + mtu = ip6_skb_dst_mtu(skb); > > + if (unlikely(skb->len <= mtu)) > > + return output(net, sk, skb); > > + > > This helper is also called for skbs where IP6CB(skb)->frag_max_size > exceeds the MTU, so this check looks wrong to me. > > Same remark for dst_allfrag() check in __ip6_finish_output(), > after this patch, it would be ignored. > Thanks for covering my carelessness. I was just considering the GSO case so frag_max_size was overlooked. dst_allfrag is indeed a case based on the code logic. But just out of curiosity, do we still see any use of this feature? From commit messages it is set when PMTU values signals smaller than min IPv6 MTU. But such PMTU values are just dropped in __ip6_rt_update_pmtu now. Iproute2 code also does not provide this route feature anymore. So it might be actually a dead check? > I think you should consider to first refactor __ip6_finish_output to make > the existing checks more readable (e.g. handle gso vs. non-gso in separate > branches) and then add the check to last seg in > ip6_finish_output_gso_slowpath_drop(). > Agree with refactoring to mirror what IPv4 code is doing. It might not hurt if we check every segments in this case, since it is already the slowpath and it will make code more compact. > Alternatively you might be able to pass more info down to > ip6_fragment and move decisions there. > > In any case we should make same frag-or-no-frag decisions, > regardless of this being the orig skb or a segmented one,
On Mon, Oct 2, 2023 at 1:53 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > To add to that: if this is a suggestion to update the algorithm to > match RFC 8021, not a fix for a bug in the current implementation, > then I think this should target net-next. Ack
Yan Zhai <yan@cloudflare.com> wrote: > On Sat, Sep 30, 2023 at 6:09 AM Florian Westphal <fw@strlen.de> wrote: > > > > This helper is also called for skbs where IP6CB(skb)->frag_max_size > > exceeds the MTU, so this check looks wrong to me. > > > > Same remark for dst_allfrag() check in __ip6_finish_output(), > > after this patch, it would be ignored. > > > Thanks for covering my carelessness. I was just considering the GSO > case so frag_max_size was overlooked. dst_allfrag is indeed a case > based on the code logic. But just out of curiosity, do we still see > any use of this feature? From commit messages it is set when PMTU > values signals smaller than min IPv6 MTU. But such PMTU values are > just dropped in __ip6_rt_update_pmtu now. Iproute2 code also does not > provide this route feature anymore. So it might be actually a dead > check? I don't think iproute2 ever exposed it, I think we can axe dst_allfrag(). > > I think you should consider to first refactor __ip6_finish_output to make > > the existing checks more readable (e.g. handle gso vs. non-gso in separate > > branches) and then add the check to last seg in > > ip6_finish_output_gso_slowpath_drop(). > > > Agree with refactoring to mirror what IPv4 code is doing. It might not > hurt if we check every segments in this case, since it is already the > slowpath and it will make code more compact. No objections from my side.
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 951ba8089b5b..42f5f68a6e24 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -854,6 +854,13 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, __be32 frag_id; u8 *prevhdr, nexthdr = 0; + /* RFC-8021 recommended atomic fragments to be deprecated. Double check + * the actual packet size before fragment it. + */ + mtu = ip6_skb_dst_mtu(skb); + if (unlikely(skb->len <= mtu)) + return output(net, sk, skb); + err = ip6_find_1stfragopt(skb, &prevhdr); if (err < 0) goto fail; @@ -861,7 +868,6 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, nexthdr = *prevhdr; nexthdr_offset = prevhdr - skb_network_header(skb); - mtu = ip6_skb_dst_mtu(skb); /* We must not fragment if the socket is set to force MTU discovery * or if the skb it not generated by a local socket.
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". RFC-8021 explicitly recommend to deprecate such use case. An Existing report from APNIC also shows that atomic fragments can be dropped unexpectedly along the path [1]. Add an extra check in ip6_fragment to catch all possible generation of atomic fragments. Skip atomic header if it is called on a packet no larger than MTU. 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(-)