ipv6: avoid atomic fragment on GSO packets
Subsystems (3)
PATCH 1/1 ipv6: avoid atomic fragment on GSO packets
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.
--- net/ipv6/ip6_output.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
net/ipv6/ip6_output.c +7 -1
@@ -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.
Signature · 1 line
2.30.2
Quoted 26 lines · Yan Zhai
GSO packets can contain a trailing segment that is smaller than
gso_size. When examining the dst MTU for such packet, if itsgso_sizeis too large, then all segments would be fragmented. However, there is a good chance the trailing segment has smaller actual size than bothgso_sizeas 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_fragmentto catch all possible generation of atomic fragments. Skip atomic header if it is called on a packet no larger than MTU.Fixes b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing")Reported-by David Wragg <[email protected]>Signed-off-by Yan Zhai <[email protected]>--- net/ipv6/ip6_output.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)net/ipv6/ip6_output.c +0
-0
net/ipv6/ip6_output.c int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, · line 863
Quoted 10 lines
net/ipv6/ip6_output.c +7
-0@@ -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,
Quoted 50 lines · Florian Westphal
Quoted 26 lines · Yan Zhai
GSO packets can contain a trailing segment that is smaller than
gso_size. When examining the dst MTU for such packet, if itsgso_sizeis too large, then all segments would be fragmented. However, there is a good chance the trailing segment has smaller actual size than bothgso_sizeas 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_fragmentto catch all possible generation of atomic fragments. Skip atomic header if it is called on a packet no larger than MTU.Fixes b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing")Reported-by David Wragg <[email protected]>Signed-off-by Yan Zhai <[email protected]>--- net/ipv6/ip6_output.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)net/ipv6/ip6_output.c +0
-0net/ipv6/ip6_output.c int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, · line 863
Quoted 10 lines
net/ipv6/ip6_output.c +7
-0@@ -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_outputto make the existing checks more readable (e.g. handle gso vs. non-gso in separate branches) and then add the check to last seg inip6_finish_output_gso_slowpath_drop().
Quoted 5 lines
Alternatively you might be able to pass more info down to
ip6_fragmentand 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.
Quoted 42 lines · Florian Westphal
Quoted 26 lines · Yan Zhai
GSO packets can contain a trailing segment that is smaller than
gso_size. When examining the dst MTU for such packet, if itsgso_sizeis too large, then all segments would be fragmented. However, there is a good chance the trailing segment has smaller actual size than bothgso_sizeas 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_fragmentto catch all possible generation of atomic fragments. Skip atomic header if it is called on a packet no larger than MTU.Fixes b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 processing")Reported-by David Wragg <[email protected]>Signed-off-by Yan Zhai <[email protected]>--- net/ipv6/ip6_output.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)net/ipv6/ip6_output.c +0
-0net/ipv6/ip6_output.c int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, · line 863
Quoted 10 lines
net/ipv6/ip6_output.c +7
-0@@ -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.
Quoted 3 lines
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?
Quoted 5 lines
I think you should consider to first refactor
__ip6_finish_outputto make the existing checks more readable (e.g. handle gso vs. non-gso in separate branches) and then add the check to last seg inip6_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.
Quoted 5 lines
Alternatively you might be able to pass more info down to
ip6_fragmentand 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,
Quoted 3 lines · Willem de Bruijn
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
Quoted 8 lines · Yan Zhai
Quoted 7 lines · Florian Westphal
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.
Quoted 8 lines
Thanks for covering my carelessness. I was just considering the GSO case so
frag_max_sizewas overlooked.dst_allfragis 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_pmtunow. 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().
Quoted 8 lines
Quoted 5 lines
I think you should consider to first refactor
__ip6_finish_outputto make the existing checks more readable (e.g. handle gso vs. non-gso in separate branches) and then add the check to last seg inip6_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.