| Message ID | 661b8bc7571c4619226fad9a00ca49352f43de45.1616345643.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | udp: GRO L4 improvements | expand |
On Sun, Mar 21, 2021 at 1:01 PM Paolo Abeni <pabeni@redhat.com> wrote: > > If UDP GRO forwarding (or list) is enabled, Please explicitly mention the gso type SKB_GSO_FRAGLIST. I, at least, didn't immediately grasp that gro forwarding is an alias for that. > and there are > udp tunnel available in the system, we could end-up doing L4 > aggregation for packets targeting the UDP tunnel. Is this specific to UDP tunnels, or can this also occur with others, such as GRE? (not implying that this patchset needs to address those at the same time) > That could inner protocol corruption, as no overaly network > parameters is taken in account at aggregation time. nit: overaly .. is taken -> overlay .. are taken You mean the packets on the frag list may have mtu exceeding the mtu of the tunnel? Please make the constraint more explicit. > Just skip the fwd GRO if this packet could land in an UDP > tunnel. Could you make more clear that this does not skip UDP GRO, only switches from fraglist-based to pure SKB_GSO_UDP_L4. > The current check is broader than what is strictly > needed, as the UDP tunnel could be e.g. on top of a different > device, but is simple and the performance downside looks not > relevant. > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
On Mon, 2021-03-22 at 09:24 -0400, Willem de Bruijn wrote: > On Sun, Mar 21, 2021 at 1:01 PM Paolo Abeni <pabeni@redhat.com> wrote: > > If UDP GRO forwarding (or list) is enabled, > > Please explicitly mention the gso type SKB_GSO_FRAGLIST. I, at least, > didn't immediately grasp that gro forwarding is an alias for that. I see the commit message was not clear at all, I'm sorry. The above means: gso_type & (NETIF_F_GSO_UDP_L4 | NETIF_F_GSO_FRAGLIST) :) > > and there are > > udp tunnel available in the system, we could end-up doing L4 > > aggregation for packets targeting the UDP tunnel. > > Is this specific to UDP tunnels, or can this also occur with others, > such as GRE? (not implying that this patchset needs to address those > at the same time) I did not look at that before your suggestion. Thanks for pointing out. I think the problem is specific to UDP: when processing the outer UDP header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and gro_receive aggregation and that is the root cause of the problem addressed here. > > Just skip the fwd GRO if this packet could land in an UDP > > tunnel. > > Could you make more clear that this does not skip UDP GRO, only > switches from fraglist-based to pure SKB_GSO_UDP_L4. Sure, I'll try to rewrite the commit message. Thanks! Paolo
> > > and there are > > > udp tunnel available in the system, we could end-up doing L4 > > > aggregation for packets targeting the UDP tunnel. > > > > Is this specific to UDP tunnels, or can this also occur with others, > > such as GRE? (not implying that this patchset needs to address those > > at the same time) I suppose GRE tunnels do not advertise GSO_UDP_L4 support, so GSO packets would get segmented before entering the tunnel device. Forwarded datagrams exceeding egress device MTU (whether tunnel or not) is a wholly separate problem. > I did not look at that before your suggestion. Thanks for pointing out. > > I think the problem is specific to UDP: when processing the outer UDP > header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and > gro_receive aggregation and that is the root cause of the problem > addressed here. Can you elaborate on the exact problem? The commit mentions "inner protocol corruption, as no overaly network parameters is taken in account at aggregation time." My understanding is that these are udp gro aggregated GSO_UDP_L4 packets forwarded to a udp tunnel device. They are not encapsulated yet. Which overlay network parameters are not, but should have been, taken account at aggregation time? > > > > > Just skip the fwd GRO if this packet could land in an UDP > > > tunnel. > > > > Could you make more clear that this does not skip UDP GRO, only > > switches from fraglist-based to pure SKB_GSO_UDP_L4. > > Sure, I'll try to rewrite the commit message. > > Thanks! > > Paolo >
On Tue, 2021-03-23 at 21:54 -0400, Willem de Bruijn wrote: > > I did not look at that before your suggestion. Thanks for pointing out. > > > > I think the problem is specific to UDP: when processing the outer UDP > > header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and > > gro_receive aggregation and that is the root cause of the problem > > addressed here. > > Can you elaborate on the exact problem? The commit mentions "inner > protocol corruption, as no overaly network parameters is taken in > account at aggregation time." > > My understanding is that these are udp gro aggregated GSO_UDP_L4 > packets forwarded to a udp tunnel device. They are not encapsulated > yet. Which overlay network parameters are not, but should have been, > taken account at aggregation time? The scenario is as follow: * a NIC has NETIF_F_GRO_UDP_FWD or NETIF_F_GRO_FRAGLIST enabled * an UDP tunnel is configured/enabled in the system * the above NIC receives some UDP-tunneled packets, targeting the mentioned tunnel * the packets go through gro_receive and they reache 'udp_gro_receive()' while processing the outer UDP header. without this patch, udp_gro_receive_segment() will kick in and the outer UDP header will be aggregated according to SKB_GSO_FRAGLIST or SKB_GSO_UDP_L4, even if this is really e.g. a vxlan packet. Different vxlan ids will be ignored/aggregated to the same GSO packet. Inner headers will be ignored, too, so that e.g. TCP over vxlan push packets will be held in the GRO engine till the next flush, etc. Please let me know if the above is more clear. Thanks! Paolo
On Wed, Mar 24, 2021 at 10:51 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Tue, 2021-03-23 at 21:54 -0400, Willem de Bruijn wrote: > > > I did not look at that before your suggestion. Thanks for pointing out. > > > > > > I think the problem is specific to UDP: when processing the outer UDP > > > header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and > > > gro_receive aggregation and that is the root cause of the problem > > > addressed here. > > > > Can you elaborate on the exact problem? The commit mentions "inner > > protocol corruption, as no overaly network parameters is taken in > > account at aggregation time." > > > > My understanding is that these are udp gro aggregated GSO_UDP_L4 > > packets forwarded to a udp tunnel device. They are not encapsulated > > yet. Which overlay network parameters are not, but should have been, > > taken account at aggregation time? > > The scenario is as follow: > > * a NIC has NETIF_F_GRO_UDP_FWD or NETIF_F_GRO_FRAGLIST enabled > * an UDP tunnel is configured/enabled in the system > * the above NIC receives some UDP-tunneled packets, targeting the > mentioned tunnel > * the packets go through gro_receive and they reache > 'udp_gro_receive()' while processing the outer UDP header. > > without this patch, udp_gro_receive_segment() will kick in and the > outer UDP header will be aggregated according to SKB_GSO_FRAGLIST > or SKB_GSO_UDP_L4, even if this is really e.g. a vxlan packet. > > Different vxlan ids will be ignored/aggregated to the same GSO packet. > Inner headers will be ignored, too, so that e.g. TCP over vxlan push > packets will be held in the GRO engine till the next flush, etc. > > Please let me know if the above is more clear. Yes, thanks a lot! That's very concrete. When processing the outer UDP tunnel header in the gro completion path, it is incorrectly identified as an inner UDP transport layer due to NAPI_GRO_CB(skb) identifying that such a layer is present (is_flist). The issue is that the UDP GRO layer distinguishes between tunnel and transport layer too late, in udp_gro_complete, while an offending assumption of that UDP == transport layer was already made in the callers udp4_gro_complete and udp6_gro_complete.
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index c5b4b586570fe..25134a3548e99 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -515,21 +515,24 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb, unsigned int off = skb_gro_offset(skb); int flush = 1; + /* we can do L4 aggregation only if the packet can't land in a tunnel + * otherwise we could corrupt the inner stream + */ NAPI_GRO_CB(skb)->is_flist = 0; - if (skb->dev->features & NETIF_F_GRO_FRAGLIST) - NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1; + if (!sk || !udp_sk(sk)->gro_receive) { + if (skb->dev->features & NETIF_F_GRO_FRAGLIST) + NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled : 1; - if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) || - (sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) { - pp = call_gro_receive(udp_gro_receive_segment, head, skb); + if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) || + (sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) + pp = call_gro_receive(udp_gro_receive_segment, head, skb); return pp; } - if (!sk || NAPI_GRO_CB(skb)->encap_mark || + if (NAPI_GRO_CB(skb)->encap_mark || (uh->check && skb->ip_summed != CHECKSUM_PARTIAL && NAPI_GRO_CB(skb)->csum_cnt == 0 && - !NAPI_GRO_CB(skb)->csum_valid) || - !udp_sk(sk)->gro_receive) + !NAPI_GRO_CB(skb)->csum_valid)) goto out; /* mark that this skb passed once through the tunnel gro layer */
If UDP GRO forwarding (or list) is enabled, and there are udp tunnel available in the system, we could end-up doing L4 aggregation for packets targeting the UDP tunnel. That could inner protocol corruption, as no overaly network parameters is taken in account at aggregation time. Just skip the fwd GRO if this packet could land in an UDP tunnel. The current check is broader than what is strictly needed, as the UDP tunnel could be e.g. on top of a different device, but is simple and the performance downside looks not relevant. Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/ipv4/udp_offload.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)