On 01/07/2020 19:43, Jakub Kicinski wrote: > There's a number of drivers which try to match the UDP ports. That > seems fragile to me. Is it actually required for HW to operate > correctly? For EF10 hardware, yes, because the hardware parses the packet headers to find for itself the offsets at which to make the various TSO edits; thus its parser needs to know which UDP ports correspond to VXLAN or GENEVE. If a GSO skb arrives at the driver with skb->encapsulation set but on a UDP port that's not known to the hardware, the driver will have to reject it in ndo_features_check() or 'manually' fall back to software segmentation from the transmit path. EF10 also makes use of encap parsing on receive, for CHECKSUM_UNNECESSARY offload (with CSUM_LEVEL) as well as RSS and filtering on inner headers (although there is currently no driver support for inner-frame RXNFC, as ethtool's API doesn't cover it). > Aren't the ports per ns in the kernel? There's no guarantee that some > other netns won't send a TSO skb and whatever other UDP encap. That is indeed one of the flaws with port-based tunnel offloads; in theory the UDP port's scope is only the 3-tuple of the socket used by the tunnel device, so never mind netns, it would be logically valid to use the same port for different encap protocols on different IP addresses on the same network interface. AFAICT udp_tunnel_notify_add_rx_port() gets a netns from the sock and then calls the ndo for every netdev in that ns. So in a setup like that, the ndo would get called twice for the same port (without any IP address information other than sa_family being passed to the driver), the driver would ignore the second one (print a netif_dbg and return EEXIST, which the caller ignores), and any TSO skbs trying to use the second one would be parsed by the hardware with the wrong encap type and probably go out garbled on the wire. I think at the time everyone took the position that "this is a really unlikely setup and if anybody really wants to do that they'll just have to turn off encap TSO".
So ndo_udp_tunnel_add is a fundamentally broken interface that people shouldn't design new hardware to support but it's close enough that it seems reasonable to use it to get _some_ encap TSO mileage out of the port-based hardware that already exists. Agree/disagree/other? -ed
