| Message ID | 20200427102229.414644-1-Jason@zx2c4.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RFC,v2] 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 in that case prepend a pseudo > ethhdr to the packet whose h_proto is copied from skb->protocol, which > will have the appropriate v4 or v6 ethertype. This allows us to keep XDP > programs' assumption correct about packets always having that ethernet > header, so that existing code doesn't break, while still allowing layer > 3 devices to use the generic XDP handler. Seems to me like this would work; let's see if anyone else has any comments :) -Toke
On 4/27/20 4:22 AM, Jason A. Donenfeld wrote: > @@ -4544,6 +4544,13 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, > * header. > */ > mac_len = skb->data - skb_mac_header(skb); > + if (!mac_len) { > + add_eth_hdr = true; > + mac_len = sizeof(struct ethhdr); > + *((struct ethhdr *)skb_push(skb, mac_len)) = (struct ethhdr) { > + .h_proto = skb->protocol > + }; please use a temp variable and explicit setting of the fields; that is not pleasant to read and can not be more performant than a more direct eth_zero_addr(eth->h_source); eth_zero_addr(eth->h_dest); eth->h_proto = skb->protocol;
On Mon, Apr 27, 2020 at 8:45 AM David Ahern <dsahern@gmail.com> wrote: > > On 4/27/20 4:22 AM, Jason A. Donenfeld wrote: > > @@ -4544,6 +4544,13 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, > > * header. > > */ > > mac_len = skb->data - skb_mac_header(skb); > > + if (!mac_len) { > > + add_eth_hdr = true; > > + mac_len = sizeof(struct ethhdr); > > + *((struct ethhdr *)skb_push(skb, mac_len)) = (struct ethhdr) { > > + .h_proto = skb->protocol > > + }; > > please use a temp variable and explicit setting of the fields; that is > not pleasant to read and can not be more performant than a more direct > > eth_zero_addr(eth->h_source); > eth_zero_addr(eth->h_dest); > eth->h_proto = skb->protocol; Ack, will change for the non-RFC v3 patch. I need to actually figure out how to test this thing first though... Jason
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > On Mon, Apr 27, 2020 at 8:45 AM David Ahern <dsahern@gmail.com> wrote: >> >> On 4/27/20 4:22 AM, Jason A. Donenfeld wrote: >> > @@ -4544,6 +4544,13 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, >> > * header. >> > */ >> > mac_len = skb->data - skb_mac_header(skb); >> > + if (!mac_len) { >> > + add_eth_hdr = true; >> > + mac_len = sizeof(struct ethhdr); >> > + *((struct ethhdr *)skb_push(skb, mac_len)) = (struct ethhdr) { >> > + .h_proto = skb->protocol >> > + }; >> >> please use a temp variable and explicit setting of the fields; that is >> not pleasant to read and can not be more performant than a more direct >> >> eth_zero_addr(eth->h_source); >> eth_zero_addr(eth->h_dest); >> eth->h_proto = skb->protocol; > > Ack, will change for the non-RFC v3 patch. I need to actually figure > out how to test this thing first though... You could try the xdp-filter application in this repo: https://github.com/xdp-project/xdp-tools (make sure you use --recurse-submodules when you clone it) That will allow you to install simple IP- and port-based filters; should be enough to check that XDP programs will correctly match the packet contents, I think? -Toke
On Mon, Apr 27, 2020 at 2:09 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > You could try the xdp-filter application in this repo: > > https://github.com/xdp-project/xdp-tools > > (make sure you use --recurse-submodules when you clone it) > > That will allow you to install simple IP- and port-based filters; should > be enough to check that XDP programs will correctly match the packet > contents, I think? Thanks. I'll give it a whirl. XDP here I come! About time I joined the party. Jason
diff --git a/net/core/dev.c b/net/core/dev.c index 522288177bbd..845a7d17abb9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4505,12 +4505,12 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp, struct bpf_prog *xdp_prog) { + bool orig_bcast, add_eth_hdr = false; struct netdev_rx_queue *rxqueue; void *orig_data, *orig_data_end; u32 metalen, act = XDP_DROP; __be16 orig_eth_type; struct ethhdr *eth; - bool orig_bcast; int hlen, off; u32 mac_len; @@ -4544,6 +4544,13 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, * header. */ mac_len = skb->data - skb_mac_header(skb); + if (!mac_len) { + add_eth_hdr = true; + mac_len = sizeof(struct ethhdr); + *((struct ethhdr *)skb_push(skb, mac_len)) = (struct ethhdr) { + .h_proto = skb->protocol + }; + } hlen = skb_headlen(skb) + mac_len; xdp->data = skb->data - mac_len; xdp->data_meta = xdp->data; @@ -4611,6 +4618,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, kfree_skb(skb); break; } + if (add_eth_hdr) + skb_pull(skb, sizeof(struct ethhdr)); return 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 in that case prepend a pseudo ethhdr to the packet whose h_proto is copied from skb->protocol, which will have the appropriate v4 or v6 ethertype. This allows us to keep XDP programs' assumption correct about packets always having that ethernet header, so that existing code doesn't break, while still allowing layer 3 devices to use the generic XDP handler. [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 | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)