| Message ID | 20200427011002.320081-1-Jason@zx2c4.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RFC,v1] net: xdp: allow for layer 3 packets in generic skb handler | expand |
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > A user reported a few days ago that packets from wireguard were possibly > ignored by XDP [1]. We haven't heard back from the original reporter to > receive more info, so this here is mostly speculative. Successfully nerd > sniped, Toke and I started poking around. Toke noticed that the generic > skb xdp handler path seems to assume that packets will always have an > ethernet header, which really isn't always the case for layer 3 packets, > which are produced by multiple drivers. This patch is untested, but I > wanted to gauge interest in this approach: if the mac_len is 0, then we > assume that it's a layer 3 packet, and figure out skb->protocol from > looking at the IP header. This patch also adds some stricter testing > around mac_len before we assume that it's an ethhdr. While your patch will fix the header pointer mangling for the skb, it unfortunately won't fix generic XDP for Wireguard: The assumption that there's an Ethernet header present is made for compatibility with native XDP, so you might say it's deliberate. I.e., the eBPF programs running in the XDP hook expect to see an Ethernet header as part of the packet data (and parses the packet like in [0]). So, to make XDP generic work for Wireguard (or other IP-header-only devices) we'd need to either (1) introduce a new XDP sub-type that assumes L4 packets, or (2) make Wireguard add a fake Ethernet header to the head of the packet and set the skb mac_header accordingly. We've discussed (1) before in other contexts (specifically, adding a 802.11 sub-type), but IIRC we decided that there wasn't enough interest. I wonder if the same wouldn't be the case for an IP sub-type, since users would have to re-write their XDP programs to fit that hook type, and it would only be usable for generic XDP on certain tunnel interface types. Not sure about the feasibility of (2). -Toke [0] https://github.com/xdp-project/xdp-tutorial/blob/master/packet01-parsing/xdp_prog_kern.c
(2) is infeasible, but there's another option: we can insert a pseudo ethernet header during the xdp hook for the case of mac_len==0. I'll play with that and send an RFC.
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > (2) is infeasible, but there's another option: we can insert a pseudo > ethernet header during the xdp hook for the case of mac_len==0. I'll > play with that and send an RFC. Oh, right, that might work too! :) -Toke
diff --git a/net/core/dev.c b/net/core/dev.c index 522288177bbd..1c4b0af09be2 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4551,9 +4551,11 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, xdp->data_hard_start = skb->data - skb_headroom(skb); orig_data_end = xdp->data_end; orig_data = xdp->data; - eth = (struct ethhdr *)xdp->data; - orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest); - orig_eth_type = eth->h_proto; + if (mac_len == sizeof(struct ethhdr)) { + eth = (struct ethhdr *)xdp->data; + orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest); + orig_eth_type = eth->h_proto; + } rxqueue = netif_get_rxqueue(skb); xdp->rxq = &rxqueue->xdp_rxq; @@ -4583,11 +4585,24 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, } /* check if XDP changed eth hdr such SKB needs update */ - eth = (struct ethhdr *)xdp->data; - if ((orig_eth_type != eth->h_proto) || - (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) { - __skb_push(skb, ETH_HLEN); - skb->protocol = eth_type_trans(skb, skb->dev); + if (mac_len == 0) { + switch (ip_hdr(skb)->version) { + case 4: + skb->protocol = htons(ETH_P_IP); + break; + case 6: + skb->protocol = htons(ETH_P_IPV6); + break; + default: + goto do_drop; + } + } else if (mac_len == sizeof(struct ethhdr)) { + eth = (struct ethhdr *)xdp->data; + if ((orig_eth_type != eth->h_proto) || + (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) { + __skb_push(skb, ETH_HLEN); + skb->protocol = eth_type_trans(skb, skb->dev); + } } switch (act) {
A user reported a few days ago that packets from wireguard were possibly ignored by XDP [1]. We haven't heard back from the original reporter to receive more info, so this here is mostly speculative. Successfully nerd sniped, Toke and I started poking around. Toke noticed that the generic skb xdp handler path seems to assume that packets will always have an ethernet header, which really isn't always the case for layer 3 packets, which are produced by multiple drivers. This patch is untested, but I wanted to gauge interest in this approach: if the mac_len is 0, then we assume that it's a layer 3 packet, and figure out skb->protocol from looking at the IP header. This patch also adds some stricter testing around mac_len before we assume that it's an ethhdr. [1] https://lore.kernel.org/wireguard/M5WzVK5--3-2@tuta.io/ Cc: Toke Høiland-Jørgensen <toke@redhat.com> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- net/core/dev.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)