[PATCH RFC v1] net: xdp: allow for layer 3 packets in generic skb handler

[PATCH RFC v1] net: xdp: allow for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

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(-)

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) {
-- 
2.26.2

Re: [PATCH RFC v1] net: xdp: allow for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

"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

Re: [PATCH RFC v1] net: xdp: allow for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

(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.

[PATCH RFC v2] net: xdp: allow for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

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(-)

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;
 }
-- 
2.26.2

Re: [PATCH RFC v2] net: xdp: allow for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

"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

Re: [PATCH RFC v2] net: xdp: allow for layer 3 packets in generic skb handler

[David Ahern]

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;

Re: [PATCH RFC v2] net: xdp: allow for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

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

Re: [PATCH RFC v2] net: xdp: allow for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

"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

Re: [PATCH RFC v2] net: xdp: allow for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

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

[PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

A user reported that packets from wireguard were possibly ignored by XDP
[1]. Apparently, 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 fixes the oversight. 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/

Reported-by: Adhipati Blambangan <adhipati@tuta.io>
Cc: David Ahern <dsahern@gmail.com>
Cc: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/core/dev.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 77c154107b0d..3bc9a96bc808 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4510,9 +4510,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	u32 metalen, act = XDP_DROP;
 	__be16 orig_eth_type;
 	struct ethhdr *eth;
+	u32 mac_len = ~0;
 	bool orig_bcast;
 	int hlen, off;
-	u32 mac_len;
 
 	/* Reinjected packets coming from act_mirred or similar should
 	 * not get XDP generic processing.
@@ -4544,6 +4544,12 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	 * header.
 	 */
 	mac_len = skb->data - skb_mac_header(skb);
+	if (!mac_len) {
+		eth = skb_push(skb, sizeof(struct ethhdr));
+		eth_zero_addr(eth->h_source);
+		eth_zero_addr(eth->h_dest);
+		eth->h_proto = skb->protocol;
+	}
 	hlen = skb_headlen(skb) + mac_len;
 	xdp->data = skb->data - mac_len;
 	xdp->data_meta = xdp->data;
@@ -4611,6 +4617,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 		kfree_skb(skb);
 		break;
 	}
+	if (!mac_len)
+		skb_pull(skb, sizeof(struct ethhdr));
 
 	return act;
 }
-- 
2.26.2

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:
> A user reported that packets from wireguard were possibly ignored by XDP
> [1]. Apparently, 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 fixes the oversight. 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/
> 
> Reported-by: Adhipati Blambangan <adhipati@tuta.io>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  net/core/dev.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 77c154107b0d..3bc9a96bc808 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4510,9 +4510,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	u32 metalen, act = XDP_DROP;
>  	__be16 orig_eth_type;
>  	struct ethhdr *eth;
> +	u32 mac_len = ~0;
>  	bool orig_bcast;
>  	int hlen, off;
> -	u32 mac_len;
>  
>  	/* Reinjected packets coming from act_mirred or similar should
>  	 * not get XDP generic processing.
> @@ -4544,6 +4544,12 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	 * header.
>  	 */
>  	mac_len = skb->data - skb_mac_header(skb);
> +	if (!mac_len) {
> +		eth = skb_push(skb, sizeof(struct ethhdr));
> +		eth_zero_addr(eth->h_source);
> +		eth_zero_addr(eth->h_dest);
> +		eth->h_proto = skb->protocol;
> +	}
>  	hlen = skb_headlen(skb) + mac_len;
>  	xdp->data = skb->data - mac_len;
>  	xdp->data_meta = xdp->data;
> @@ -4611,6 +4617,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  		kfree_skb(skb);
>  		break;
>  	}
> +	if (!mac_len)
> +		skb_pull(skb, sizeof(struct ethhdr));

Is this going to work correctly with XDP_TX? presumably wireguard
doesn't want the ethernet L2 on egress, either? And what about
redirects?

I'm not sure we can paper over the L2 differences between interfaces.
Isn't user supposed to know what interface the program is attached to?
I believe that's the case for cls_bpf ingress, right?

>  	return act;
>  }

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

On Mon, Apr 27, 2020 at 2:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> Is this going to work correctly with XDP_TX? presumably wireguard
> doesn't want the ethernet L2 on egress, either? And what about
> redirects?

I'll probably need to augment this patch to handle XDP_TX. I'm not
sure how redirects work, but I'm guessing I'll need to add the
pseudoheader there too? Or are the semantics there even weirder?

>
> I'm not sure we can paper over the L2 differences between interfaces.
> Isn't user supposed to know what interface the program is attached to?
> I believe that's the case for cls_bpf ingress, right?

That was my initial intuition too (see my v1), but Toke said that XDP
prefers having L2 everywhere, which is what motivated the L2-emulation
route that this v3 patch takes. The advantage of my v1 approach is
that it's closer to the truth and likely performs slightly better. The
advantage of the v3 approach is that existing XDP programs don't need
to worry about differences. I don't have a preference either way, and
I'm happy to implement either approach. Let me know what you guys
prefer.

Jason

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:
> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:
> > A user reported that packets from wireguard were possibly ignored by XDP
> > [1]. Apparently, 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 fixes the oversight. 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.
> 
> Is this going to work correctly with XDP_TX? presumably wireguard
> doesn't want the ethernet L2 on egress, either? And what about
> redirects?
> 
> I'm not sure we can paper over the L2 differences between interfaces.
> Isn't user supposed to know what interface the program is attached to?
> I believe that's the case for cls_bpf ingress, right?

In general we should also ask ourselves if supporting XDPgeneric on
software interfaces isn't just pointless code bloat, and it wouldn't
be better to let XDP remain clearly tied to the in-driver native use
case.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

On Mon, Apr 27, 2020 at 3:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:
> > On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:
> > > A user reported that packets from wireguard were possibly ignored by XDP
> > > [1]. Apparently, 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 fixes the oversight. 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.
> >
> > Is this going to work correctly with XDP_TX? presumably wireguard
> > doesn't want the ethernet L2 on egress, either? And what about
> > redirects?
> >
> > I'm not sure we can paper over the L2 differences between interfaces.
> > Isn't user supposed to know what interface the program is attached to?
> > I believe that's the case for cls_bpf ingress, right?
>
> In general we should also ask ourselves if supporting XDPgeneric on
> software interfaces isn't just pointless code bloat, and it wouldn't
> be better to let XDP remain clearly tied to the in-driver native use
> case.

Seems nice to be able to use XDP everywhere as a means of packet
processing without context switch, right?

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Mon, 27 Apr 2020 15:08:55 -0600 Jason A. Donenfeld wrote:
> On Mon, Apr 27, 2020 at 3:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:  
> > > On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:  
> > > > A user reported that packets from wireguard were possibly ignored by XDP
> > > > [1]. Apparently, 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 fixes the oversight. 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.  
> > >
> > > Is this going to work correctly with XDP_TX? presumably wireguard
> > > doesn't want the ethernet L2 on egress, either? And what about
> > > redirects?
> > >
> > > I'm not sure we can paper over the L2 differences between interfaces.
> > > Isn't user supposed to know what interface the program is attached to?
> > > I believe that's the case for cls_bpf ingress, right?  
> >
> > In general we should also ask ourselves if supporting XDPgeneric on
> > software interfaces isn't just pointless code bloat, and it wouldn't
> > be better to let XDP remain clearly tied to the in-driver native use
> > case.  
> 
> Seems nice to be able to use XDP everywhere as a means of packet
> processing without context switch, right?

cls_bpf has more capabilities as it can integrate with the stack and
poke at various skb bits. Native XDP is faster, because it can short
circuit normal stack processing and skb allocation, if we can figure
out inside the BPF program that we want to redirect or drop the frame.
But software devices cannot take advantage of that speed up, the frame
is already inside the stack.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:
>> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:
>> > A user reported that packets from wireguard were possibly ignored by XDP
>> > [1]. Apparently, 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 fixes the oversight. 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.
>> 
>> Is this going to work correctly with XDP_TX? presumably wireguard
>> doesn't want the ethernet L2 on egress, either? And what about
>> redirects?
>> 
>> I'm not sure we can paper over the L2 differences between interfaces.
>> Isn't user supposed to know what interface the program is attached to?
>> I believe that's the case for cls_bpf ingress, right?
>
> In general we should also ask ourselves if supporting XDPgeneric on
> software interfaces isn't just pointless code bloat, and it wouldn't
> be better to let XDP remain clearly tied to the in-driver native use
> case.

I was mostly ignoring generic XDP for a long time for this reason. But
it seems to me that people find generic XDP quite useful, so I'm no
longer so sure this is the right thing to do...

-Toke

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Mon, 27 Apr 2020 23:14:05 +0200 Toke Høiland-Jørgensen wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> > On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:  
> >> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:  
> >> > A user reported that packets from wireguard were possibly ignored by XDP
> >> > [1]. Apparently, 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 fixes the oversight. 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.  
> >> 
> >> Is this going to work correctly with XDP_TX? presumably wireguard
> >> doesn't want the ethernet L2 on egress, either? And what about
> >> redirects?
> >> 
> >> I'm not sure we can paper over the L2 differences between interfaces.
> >> Isn't user supposed to know what interface the program is attached to?
> >> I believe that's the case for cls_bpf ingress, right?  
> >
> > In general we should also ask ourselves if supporting XDPgeneric on
> > software interfaces isn't just pointless code bloat, and it wouldn't
> > be better to let XDP remain clearly tied to the in-driver native use
> > case.  
> 
> I was mostly ignoring generic XDP for a long time for this reason. But
> it seems to me that people find generic XDP quite useful, so I'm no
> longer so sure this is the right thing to do...

I wonder, maybe our documentation is not clear. IOW we were saying that
XDP is a faster cls_bpf, which leaves out the part that XDP only makes
sense for HW/virt devices.

Kinda same story as XDP egress, folks may be asking for it but that
doesn't mean it makes sense.

Perhaps the original reporter realized this and that's why they
disappeared?

My understanding is that XDP generic is aimed at testing and stop gap
for drivers which don't implement native. Defining behavior based on
XDP generic's needs seems a little backwards, and risky.

That said, I don't feel particularly strongly about this.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

On Mon, Apr 27, 2020 at 3:31 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 27 Apr 2020 23:14:05 +0200 Toke Høiland-Jørgensen wrote:
> > Jakub Kicinski <kuba@kernel.org> writes:
> > > On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:
> > >> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:
> > >> > A user reported that packets from wireguard were possibly ignored by XDP
> > >> > [1]. Apparently, 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 fixes the oversight. 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.
> > >>
> > >> Is this going to work correctly with XDP_TX? presumably wireguard
> > >> doesn't want the ethernet L2 on egress, either? And what about
> > >> redirects?
> > >>
> > >> I'm not sure we can paper over the L2 differences between interfaces.
> > >> Isn't user supposed to know what interface the program is attached to?
> > >> I believe that's the case for cls_bpf ingress, right?
> > >
> > > In general we should also ask ourselves if supporting XDPgeneric on
> > > software interfaces isn't just pointless code bloat, and it wouldn't
> > > be better to let XDP remain clearly tied to the in-driver native use
> > > case.
> >
> > I was mostly ignoring generic XDP for a long time for this reason. But
> > it seems to me that people find generic XDP quite useful, so I'm no
> > longer so sure this is the right thing to do...
>
> I wonder, maybe our documentation is not clear. IOW we were saying that
> XDP is a faster cls_bpf, which leaves out the part that XDP only makes
> sense for HW/virt devices.
>
> Kinda same story as XDP egress, folks may be asking for it but that
> doesn't mean it makes sense.
>
> Perhaps the original reporter realized this and that's why they
> disappeared?
>
> My understanding is that XDP generic is aimed at testing and stop gap
> for drivers which don't implement native. Defining behavior based on
> XDP generic's needs seems a little backwards, and risky.
>
> That said, I don't feel particularly strongly about this.

Okay, well, I'll continue developing the v3 approach a little further
-- making sure I have tx path handled too and whatnot. Then at least
something viable will be available, and you can take or leave it
depending on what you all decide.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

On Mon, Apr 27, 2020 at 5:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Mon, Apr 27, 2020 at 3:31 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 27 Apr 2020 23:14:05 +0200 Toke Høiland-Jørgensen wrote:
> > > Jakub Kicinski <kuba@kernel.org> writes:
> > > > On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:
> > > >> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:
> > > >> > A user reported that packets from wireguard were possibly ignored by XDP
> > > >> > [1]. Apparently, 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 fixes the oversight. 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.
> > > >>
> > > >> Is this going to work correctly with XDP_TX? presumably wireguard
> > > >> doesn't want the ethernet L2 on egress, either? And what about
> > > >> redirects?
> > > >>
> > > >> I'm not sure we can paper over the L2 differences between interfaces.
> > > >> Isn't user supposed to know what interface the program is attached to?
> > > >> I believe that's the case for cls_bpf ingress, right?
> > > >
> > > > In general we should also ask ourselves if supporting XDPgeneric on
> > > > software interfaces isn't just pointless code bloat, and it wouldn't
> > > > be better to let XDP remain clearly tied to the in-driver native use
> > > > case.
> > >
> > > I was mostly ignoring generic XDP for a long time for this reason. But
> > > it seems to me that people find generic XDP quite useful, so I'm no
> > > longer so sure this is the right thing to do...
> >
> > I wonder, maybe our documentation is not clear. IOW we were saying that
> > XDP is a faster cls_bpf, which leaves out the part that XDP only makes
> > sense for HW/virt devices.
> >
> > Kinda same story as XDP egress, folks may be asking for it but that
> > doesn't mean it makes sense.
> >
> > Perhaps the original reporter realized this and that's why they
> > disappeared?
> >
> > My understanding is that XDP generic is aimed at testing and stop gap
> > for drivers which don't implement native. Defining behavior based on
> > XDP generic's needs seems a little backwards, and risky.
> >
> > That said, I don't feel particularly strongly about this.
>
> Okay, well, I'll continue developing the v3 approach a little further
> -- making sure I have tx path handled too and whatnot. Then at least
> something viable will be available, and you can take or leave it
> depending on what you all decide.

Actually, it looks like egress XDP still hasn't been merged. So I
think this patch should be good to go in terms of what it is.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Mon, 27 Apr 2020 17:45:12 -0600 Jason A. Donenfeld wrote:
> > Okay, well, I'll continue developing the v3 approach a little further
> > -- making sure I have tx path handled too and whatnot. Then at least
> > something viable will be available, and you can take or leave it
> > depending on what you all decide.  
> 
> Actually, it looks like egress XDP still hasn't been merged. So I
> think this patch should be good to go in terms of what it is.

TX and redirect don't require the XDP egress hook to function.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jason A. Donenfeld]

On Mon, Apr 27, 2020 at 6:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 27 Apr 2020 17:45:12 -0600 Jason A. Donenfeld wrote:
> > > Okay, well, I'll continue developing the v3 approach a little further
> > > -- making sure I have tx path handled too and whatnot. Then at least
> > > something viable will be available, and you can take or leave it
> > > depending on what you all decide.
> >
> > Actually, it looks like egress XDP still hasn't been merged. So I
> > think this patch should be good to go in terms of what it is.
>
> TX and redirect don't require the XDP egress hook to function.

Oh, you meant the TX and redirect actions returned from the ingress
hook. Gotcha. The paths that those take don't appear to rely on having
the fake header though, whereas the actual xdp_progs that run do rely
on that, which is why it's added there.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Mon, 27 Apr 2020 18:17:16 -0600 Jason A. Donenfeld wrote:
> On Mon, Apr 27, 2020 at 6:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 27 Apr 2020 17:45:12 -0600 Jason A. Donenfeld wrote:  
> > > > Okay, well, I'll continue developing the v3 approach a little further
> > > > -- making sure I have tx path handled too and whatnot. Then at least
> > > > something viable will be available, and you can take or leave it
> > > > depending on what you all decide.  
> > >
> > > Actually, it looks like egress XDP still hasn't been merged. So I
> > > think this patch should be good to go in terms of what it is.  
> >
> > TX and redirect don't require the XDP egress hook to function.  
> 
> Oh, you meant the TX and redirect actions returned from the ingress
> hook. Gotcha. The paths that those take don't appear to rely on having
> the fake header though, whereas the actual xdp_progs that run do rely
> on that, which is why it's added there.

Ack, but if the redirection target is a real Ethernet device it will
see a frame without a L2 header, right? Redirect from a real Ethernet
device to a L3 one and vice versa remains broken.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 27 Apr 2020 23:14:05 +0200 Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> > On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:  
>> >> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:  
>> >> > A user reported that packets from wireguard were possibly ignored by XDP
>> >> > [1]. Apparently, 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 fixes the oversight. 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.  
>> >> 
>> >> Is this going to work correctly with XDP_TX? presumably wireguard
>> >> doesn't want the ethernet L2 on egress, either? And what about
>> >> redirects?
>> >> 
>> >> I'm not sure we can paper over the L2 differences between interfaces.
>> >> Isn't user supposed to know what interface the program is attached to?
>> >> I believe that's the case for cls_bpf ingress, right?  
>> >
>> > In general we should also ask ourselves if supporting XDPgeneric on
>> > software interfaces isn't just pointless code bloat, and it wouldn't
>> > be better to let XDP remain clearly tied to the in-driver native use
>> > case.  
>> 
>> I was mostly ignoring generic XDP for a long time for this reason. But
>> it seems to me that people find generic XDP quite useful, so I'm no
>> longer so sure this is the right thing to do...
>
> I wonder, maybe our documentation is not clear. IOW we were saying that
> XDP is a faster cls_bpf, which leaves out the part that XDP only makes
> sense for HW/virt devices.

I'm not sure it's just because people think it's faster. There's also a
semantic difference; if you just want to do ingress filtering, simply
sticking an XDP program on the interface is a natural fit. Whereas
figuring out the tc semantics for ingress is non-trivial. And also
reusability of XDP programs from the native hook is an important
consideration, I believe. Which is also why I think the pseudo-MAC
header approach is the right fix for L3 devices :)

> Kinda same story as XDP egress, folks may be asking for it but that
> doesn't mean it makes sense.

Well I do also happen to think that XDP egress is a good idea ;)

> Perhaps the original reporter realized this and that's why they
> disappeared?
>
> My understanding is that XDP generic is aimed at testing and stop gap
> for drivers which don't implement native. Defining behavior based on
> XDP generic's needs seems a little backwards, and risky.

That I can agree with - generic XDP should follow the semantics of
native XDP, not the other way around. But that's what we're doing here
(with the pseudo-MAC header approach), isn't it? Whereas if we were
saying "just write your XDP programs to assume only L3 packets" we would
be creating a new semantic for generic XDP...

-Toke

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Jakub Kicinski]

On Tue, 28 Apr 2020 11:27:31 +0200 Toke Høiland-Jørgensen wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> > I wonder, maybe our documentation is not clear. IOW we were saying that
> > XDP is a faster cls_bpf, which leaves out the part that XDP only makes
> > sense for HW/virt devices.  
> 
> I'm not sure it's just because people think it's faster. There's also a
> semantic difference; if you just want to do ingress filtering, simply
> sticking an XDP program on the interface is a natural fit.

I'm afraid if we take that stand we're going to struggle to deliver. 
I'd personally prefer to keep XDP simple and focused.

> Whereas figuring out the tc semantics for ingress is non-trivial.

You mean adding an ingress qdisc and installing a filter?
If it is perhaps we could alleviate that with better user space tooling?

> And also reusability of XDP programs from the native hook is an important
> consideration, I believe. Which is also why I think the pseudo-MAC
> header approach is the right fix for L3 devices :)

Yes, valid point, I've never tried it myself, but I believe the C-level
portability shouldn't be too hard? At least the context members are
named the same. And there isn't that much XDP can do..

Perhaps the portability is something we should keep in mind going
forward. Just in case.

> > Kinda same story as XDP egress, folks may be asking for it but that
> > doesn't mean it makes sense.  
> 
> Well I do also happen to think that XDP egress is a good idea ;)

I was planning not to get involved in that conversation any more,
let's move on :P

> > Perhaps the original reporter realized this and that's why they
> > disappeared?
> >
> > My understanding is that XDP generic is aimed at testing and stop gap
> > for drivers which don't implement native. Defining behavior based on
> > XDP generic's needs seems a little backwards, and risky.  
> 
> That I can agree with - generic XDP should follow the semantics of
> native XDP, not the other way around. But that's what we're doing here
> (with the pseudo-MAC header approach), isn't it? Whereas if we were
> saying "just write your XDP programs to assume only L3 packets" we would
> be creating a new semantic for generic XDP...

But you do see the problem this creates on redirect already, right?
Do we want to support all that? Add an if in the redirect fast path?
There will _never_ be native XDP for WireGuard, it just doesn't make
sense. 

Generic XDP is not a hook in its own right, frame is already firmly
inside the stack, XDP is on the perimeter.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Alexei Starovoitov]

On Tue, Apr 28, 2020 at 09:51:13AM -0700, Jakub Kicinski wrote:
> > > Kinda same story as XDP egress, folks may be asking for it but that
> > > doesn't mean it makes sense.  
> > 
> > Well I do also happen to think that XDP egress is a good idea ;)
> 
> I was planning not to get involved in that conversation any more,
> let's move on :P

getting involved like arguing till last men standing is not something
people appreciate, but expressing your opinion in xdp egress thread
is certainly valuable.

> > That I can agree with - generic XDP should follow the semantics of
> > native XDP, not the other way around. But that's what we're doing here
> > (with the pseudo-MAC header approach), isn't it? Whereas if we were
> > saying "just write your XDP programs to assume only L3 packets" we would
> > be creating a new semantic for generic XDP...
> 
> But you do see the problem this creates on redirect already, right?
> Do we want to support all that? Add an if in the redirect fast path?
> There will _never_ be native XDP for WireGuard, it just doesn't make
> sense. 
> 
> Generic XDP is not a hook in its own right, frame is already firmly
> inside the stack, XDP is on the perimeter.

I think the proper fix here is to disallow generic xdp on l3 devices.
imo the only use case for generic xdp is to provide fallback for
heterogeneous fleet of servers. On most of the machines XDP program
will be using native XDP, but some servers might have either old
kernel or old hw and in such cases generic XDP saves the day.
XDP programmer doens't need to special case their user and kernel side
for long tail of servers where performance doesn't matter.

cls_bpf addresses the use case for folks that want to process
ingress/egress traffic on l3 netdev. I don't think there is a need
for xdp based solution in addition to cls_bpf. It's going to be
much more limited comparing to cls_bpf.

Re: [PATCH net v3] net: xdp: account for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> A user reported that packets from wireguard were possibly ignored by XDP
> [1]. Apparently, 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 fixes the oversight. 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/
>
> Reported-by: Adhipati Blambangan <adhipati@tuta.io>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH RFC v1] net: xdp: allow for layer 3 packets in generic skb handler

[Toke Høiland-Jørgensen]

"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