| Message ID | 20240322114624.160306-4-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | gro: various fixes related to UDP tunnels | expand |
Antoine Tenart wrote: > UDP GRO validates checksums and in udp4/6_gro_complete fraglist packets > are converted to CHECKSUM_UNNECESSARY to avoid later checks. However > this is an issue for CHECKSUM_PARTIAL packets as they can be looped in > an egress path and then their partial checksums are not fixed. > > Different issues can be observed, from invalid checksum on packets to > traces like: > > gen01: hw csum failure > skb len=3008 headroom=160 headlen=1376 tailroom=0 > mac=(106,14) net=(120,40) trans=160 > shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0)) > csum(0xffff232e ip_summed=2 complete_sw=0 valid=0 level=0) > hash(0x77e3d716 sw=1 l4=1) proto=0x86dd pkttype=0 iif=12 > ... > > Fix this by only converting CHECKSUM_NONE packets to > CHECKSUM_UNNECESSARY by reusing __skb_incr_checksum_unnecessary. > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") > Signed-off-by: Antoine Tenart <atenart@kernel.org> Sorry to have yet more questions, but Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment) have the same checksumming behavior? Second, this leaves CHECKSUM_COMPLETE as is. Is that intentional? I don't immediately see where GSO skb->csum would be updated. I can take a closer look too, but did not want to delay feedback. > --- > net/ipv4/udp_offload.c | 8 +------- > net/ipv6/udp_offload.c | 8 +------- > 2 files changed, 2 insertions(+), 14 deletions(-) > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 3bb69464930b..548476d78237 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -722,13 +722,7 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) > skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4); > skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count; > > - if (skb->ip_summed == CHECKSUM_UNNECESSARY) { > - if (skb->csum_level < SKB_MAX_CSUM_LEVEL) > - skb->csum_level++; > - } else { > - skb->ip_summed = CHECKSUM_UNNECESSARY; > - skb->csum_level = 0; > - } > + __skb_incr_checksum_unnecessary(skb); > > return 0; > } > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > index 312bcaeea96f..bbd347de00b4 100644 > --- a/net/ipv6/udp_offload.c > +++ b/net/ipv6/udp_offload.c > @@ -174,13 +174,7 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff) > skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4); > skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count; > > - if (skb->ip_summed == CHECKSUM_UNNECESSARY) { > - if (skb->csum_level < SKB_MAX_CSUM_LEVEL) > - skb->csum_level++; > - } else { > - skb->ip_summed = CHECKSUM_UNNECESSARY; > - skb->csum_level = 0; > - } > + __skb_incr_checksum_unnecessary(skb); > > return 0; > } > -- > 2.44.0 >
Quoting Willem de Bruijn (2024-03-22 18:29:40) > Antoine Tenart wrote: > > UDP GRO validates checksums and in udp4/6_gro_complete fraglist packets > > are converted to CHECKSUM_UNNECESSARY to avoid later checks. However > > this is an issue for CHECKSUM_PARTIAL packets as they can be looped in > > an egress path and then their partial checksums are not fixed. > > > > Different issues can be observed, from invalid checksum on packets to > > traces like: > > > > gen01: hw csum failure > > skb len=3008 headroom=160 headlen=1376 tailroom=0 > > mac=(106,14) net=(120,40) trans=160 > > shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0)) > > csum(0xffff232e ip_summed=2 complete_sw=0 valid=0 level=0) > > hash(0x77e3d716 sw=1 l4=1) proto=0x86dd pkttype=0 iif=12 > > ... > > > > Fix this by only converting CHECKSUM_NONE packets to > > CHECKSUM_UNNECESSARY by reusing __skb_incr_checksum_unnecessary. > > > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") > > Signed-off-by: Antoine Tenart <atenart@kernel.org> > > Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment) > have the same checksumming behavior? They can't as non-fraglist GRO packets can be aggregated, csum can't just be converted there. It seems non-fraglist handles csum as it should already, except for tunneled packets but that's why patch 4 prevents those packets from being GROed. > Second, this leaves CHECKSUM_COMPLETE as is. Is that intentional? I > don't immediately see where GSO skb->csum would be updated. That is intentional, fraglist GSO packets aren't modified and csums don't need to be updated. The issues are with converting the checksum type: partial checksum information can be lost. Thanks, Antoine
Antoine Tenart wrote: > Quoting Willem de Bruijn (2024-03-22 18:29:40) > > Antoine Tenart wrote: > > > UDP GRO validates checksums and in udp4/6_gro_complete fraglist packets > > > are converted to CHECKSUM_UNNECESSARY to avoid later checks. However > > > this is an issue for CHECKSUM_PARTIAL packets as they can be looped in > > > an egress path and then their partial checksums are not fixed. > > > > > > Different issues can be observed, from invalid checksum on packets to > > > traces like: > > > > > > gen01: hw csum failure > > > skb len=3008 headroom=160 headlen=1376 tailroom=0 > > > mac=(106,14) net=(120,40) trans=160 > > > shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0)) > > > csum(0xffff232e ip_summed=2 complete_sw=0 valid=0 level=0) > > > hash(0x77e3d716 sw=1 l4=1) proto=0x86dd pkttype=0 iif=12 > > > ... > > > > > > Fix this by only converting CHECKSUM_NONE packets to > > > CHECKSUM_UNNECESSARY by reusing __skb_incr_checksum_unnecessary. > > > > > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") > > > Signed-off-by: Antoine Tenart <atenart@kernel.org> > > > > Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment) > > have the same checksumming behavior? > > They can't as non-fraglist GRO packets can be aggregated, csum can't > just be converted there. I suppose this could be done. But it is just simpler to convert to CHECKSUM_UNNECESSARY. > It seems non-fraglist handles csum as it should > already, except for tunneled packets but that's why patch 4 prevents > those packets from being GROed. You mean that on segmentation, the segments are restored and thus skb->csum of each segment is again correct, right? I suppose this could be converted to CHECKSUM_UNNECESSARY if just for equivalence between the two UDP_GRO methods and simplicity. But also fine to leave as is. Can you at least summarize this in the commit message? Currently CHECKSUM_COMPLETE is not mentioned, but the behavior is not trivial. It may be helpful next time we again stumble on this code and do a git blame. > > > Second, this leaves CHECKSUM_COMPLETE as is. Is that intentional? I > > don't immediately see where GSO skb->csum would be updated. > > That is intentional, fraglist GSO packets aren't modified and csums > don't need to be updated. The issues are with converting the checksum > type: partial checksum information can be lost. > > Thanks, > Antoine
Quoting Willem de Bruijn (2024-03-25 19:39:46) > Antoine Tenart wrote: > > Quoting Willem de Bruijn (2024-03-22 18:29:40) > > > > > > Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment) > > > have the same checksumming behavior? > > > > They can't as non-fraglist GRO packets can be aggregated, csum can't > > just be converted there. > > I suppose this could be done. But it is just simpler to convert to > CHECKSUM_UNNECESSARY. Oh, do you mean using the non-fraglist behavior in fraglist? udp_gro_complete_segment converts all packets to CHECKSUM_PARTIAL (as packets could have been aggregated) but that's not required in fraglist. To say it another way: my understanding is packets in the non-fraglist case have to be converted to CHECKSUM_PARTIAL, while the fraglist case can keep the checksum info as-is (and have the conversion to unnecessary as an optimization when applicable). > You mean that on segmentation, the segments are restored and thus > skb->csum of each segment is again correct, right? In the fraglist case, yes. > I suppose this could be converted to CHECKSUM_UNNECESSARY if just > for equivalence between the two UDP_GRO methods and simplicity. > > But also fine to leave as is. I'm not sure I got your suggestion as I don't see how non-fraglist packets could be converted to CHECKSUM_UNNECESSARY when being aggregated (uh->len can change there). This series is aiming at fixing known issues and unifying the behavior would be net-next material IMO. I'll send a v4 for those fixes, but then I'm happy to discuss the above suggestion and investigate; so let's continue the discussion here in parallel. > Can you at least summarize this in the commit message? Currently > CHECKSUM_COMPLETE is not mentioned, but the behavior is not trivial. > It may be helpful next time we again stumble on this code and do a > git blame. Sure, I'll try to improve the commit log. Thanks! Antoine
Antoine Tenart wrote: > Quoting Willem de Bruijn (2024-03-25 19:39:46) > > Antoine Tenart wrote: > > > Quoting Willem de Bruijn (2024-03-22 18:29:40) > > > > > > > > Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment) > > > > have the same checksumming behavior? > > > > > > They can't as non-fraglist GRO packets can be aggregated, csum can't > > > just be converted there. > > > > I suppose this could be done. But it is just simpler to convert to > > CHECKSUM_UNNECESSARY. > > Oh, do you mean using the non-fraglist behavior in fraglist? > udp_gro_complete_segment converts all packets to CHECKSUM_PARTIAL (as > packets could have been aggregated) but that's not required in fraglist. > > To say it another way: my understanding is packets in the non-fraglist > case have to be converted to CHECKSUM_PARTIAL, while the fraglist case > can keep the checksum info as-is (and have the conversion to unnecessary > as an optimization when applicable). That makes sense. Thanks. > > You mean that on segmentation, the segments are restored and thus > > skb->csum of each segment is again correct, right? > > In the fraglist case, yes. > > > I suppose this could be converted to CHECKSUM_UNNECESSARY if just > > for equivalence between the two UDP_GRO methods and simplicity. > > > > But also fine to leave as is. > > I'm not sure I got your suggestion as I don't see how non-fraglist > packets could be converted to CHECKSUM_UNNECESSARY when being aggregated > (uh->len can change there). > > This series is aiming at fixing known issues and unifying the behavior > would be net-next material IMO. I'll send a v4 for those fixes, but then > I'm happy to discuss the above suggestion and investigate; so let's > continue the discussion here in parallel. The above explains why the solutions are distinct. No need to try to unify more in a follow-up, then. > > Can you at least summarize this in the commit message? Currently > > CHECKSUM_COMPLETE is not mentioned, but the behavior is not trivial. > > It may be helpful next time we again stumble on this code and do a > > git blame. > > Sure, I'll try to improve the commit log. > > Thanks! > Antoine
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 3bb69464930b..548476d78237 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -722,13 +722,7 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4); skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count; - if (skb->ip_summed == CHECKSUM_UNNECESSARY) { - if (skb->csum_level < SKB_MAX_CSUM_LEVEL) - skb->csum_level++; - } else { - skb->ip_summed = CHECKSUM_UNNECESSARY; - skb->csum_level = 0; - } + __skb_incr_checksum_unnecessary(skb); return 0; } diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c index 312bcaeea96f..bbd347de00b4 100644 --- a/net/ipv6/udp_offload.c +++ b/net/ipv6/udp_offload.c @@ -174,13 +174,7 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff) skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4); skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count; - if (skb->ip_summed == CHECKSUM_UNNECESSARY) { - if (skb->csum_level < SKB_MAX_CSUM_LEVEL) - skb->csum_level++; - } else { - skb->ip_summed = CHECKSUM_UNNECESSARY; - skb->csum_level = 0; - } + __skb_incr_checksum_unnecessary(skb); return 0; }
UDP GRO validates checksums and in udp4/6_gro_complete fraglist packets are converted to CHECKSUM_UNNECESSARY to avoid later checks. However this is an issue for CHECKSUM_PARTIAL packets as they can be looped in an egress path and then their partial checksums are not fixed. Different issues can be observed, from invalid checksum on packets to traces like: gen01: hw csum failure skb len=3008 headroom=160 headlen=1376 tailroom=0 mac=(106,14) net=(120,40) trans=160 shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0)) csum(0xffff232e ip_summed=2 complete_sw=0 valid=0 level=0) hash(0x77e3d716 sw=1 l4=1) proto=0x86dd pkttype=0 iif=12 ... Fix this by only converting CHECKSUM_NONE packets to CHECKSUM_UNNECESSARY by reusing __skb_incr_checksum_unnecessary. Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") Signed-off-by: Antoine Tenart <atenart@kernel.org> --- net/ipv4/udp_offload.c | 8 +------- net/ipv6/udp_offload.c | 8 +------- 2 files changed, 2 insertions(+), 14 deletions(-)