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

[PATCH net v4] 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]. Another user reported that modifying packets from layer 3
interfaces results in impossible to diagnose drops.

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 and so is
hard_header_len, then we know that the skb is 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.

For the XDP_PASS case, we remove the added ethernet header after the
program runs. And for the XDP_REDIRECT or XDP_TX cases, we leave it on.
This mirrors the logic for layer 2 packets, where mac_len is part of the
buffer given to xdp, and then is pushed for the XDP_REDIRECT or XDP_TX
cases, while it has already been naturally removed for the XDP_PASS
case, since it always existed in the head space. This should preserve
semantics for both cases.

Previous discussions have included the point that maybe XDP should just
be intentionally broken on layer 3 interfaces, by design, and that layer
3 people should be using cls_bpf. However, I think there are good
grounds to reconsider this perspective:

- Complicated deployments wind up applying XDP modifications to a
  variety of different devices on a given host, some of which are using
  specialized ethernet cards and other ones using virtual layer 3
  interfaces, such as WireGuard. Being able to apply one codebase to
  each of these winds up being essential.

- cls_bpf does not support the same feature set as XDP, and operates at
  a slightly different stage in the networking stack. You may reply,
  "then add all the features you want to cls_bpf", but that seems to be
  missing the point, and would still result in there being two ways to
  do everything, which is not desirable for anyone actually _using_ this
  code.

- While XDP was originally made for hardware offloading, and while many
  look disdainfully upon the generic mode, it nevertheless remains a
  highly useful and popular way of adding bespoke packet
  transformations, and from that perspective, a difference between layer
  2 and layer 3 packets is immaterial if the user is primarily concerned
  with transformations to layer 3 and beyond.

[1] https://lore.kernel.org/wireguard/M5WzVK5--3-2@tuta.io/

Reported-by: Thomas Ptacek <thomas@sockpuppet.org>
Reported-by: Adhipati Blambangan <adhipati@tuta.io>
Cc: David Ahern <dsahern@gmail.com>
Cc: Toke Høiland-Jørgensen <toke@redhat.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
I had originally dropped this patch, but the issue kept coming up in
user reports, so here's a v4 of it. Testing of it is still rather slim,
but hopefully that will change in the coming days.

Changes v3->v4:
- We now preserve the same logic for XDP_TX/XDP_REDIRECT as before.
- hard_header_len is checked in addition to mac_len.

 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 7df6c9617321..e6403e74a6aa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4591,12 +4591,12 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 				     struct xdp_buff *xdp,
 				     struct bpf_prog *xdp_prog)
 {
+	bool orig_bcast, skb_is_l3 = 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;
 
@@ -4630,6 +4630,13 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	 * header.
 	 */
 	mac_len = skb->data - skb_mac_header(skb);
+	skb_is_l3 = !mac_len && !skb->dev->hard_header_len;
+	if (skb_is_l3) {
+		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;
@@ -4684,6 +4691,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 		__skb_push(skb, mac_len);
 		break;
 	case XDP_PASS:
+		if (skb_is_l3)
+			skb_pull(skb, sizeof(struct ethhdr));
 		metalen = xdp->data - xdp->data_meta;
 		if (metalen)
 			skb_metadata_set(skb, metalen);
-- 
2.28.0

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

[Jakub Kicinski]

On Thu, 13 Aug 2020 21:58:16 +0200 Jason A. Donenfeld wrote:
> - cls_bpf does not support the same feature set as XDP, and operates at
>   a slightly different stage in the networking stack.

Please elaborate.

> I had originally dropped this patch, but the issue kept coming up in
> user reports, so here's a v4 of it. Testing of it is still rather slim,
> but hopefully that will change in the coming days.

Here an alternative patch, untested:

diff --git a/net/core/dev.c b/net/core/dev.c
index d3d53dc601f9..7f68831bdd4f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5434,6 +5434,11 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
        struct bpf_prog *new = xdp->prog;
        int ret = 0;
 
+       if (!dev->hard_header_len) {
+               NL_SET_ERR_MSG(xdp->extack, "generic XDP not supported on L3 devices, please use cls_bpf");
+               return -EINVAL;
+       }
+
        if (new) {
                u32 i;
 

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

[Jason A. Donenfeld]

On Thu, Aug 13, 2020 at 11:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > I had originally dropped this patch, but the issue kept coming up in
> > user reports, so here's a v4 of it. Testing of it is still rather slim,
> > but hopefully that will change in the coming days.
>
> Here an alternative patch, untested:

Funny. But come on now... Why would we want to deprive our users of
system consistency?

Doesn't it make sense to allow users to use the same code across
interfaces? You actually want them to rewrite their code to use a
totally different trigger point just because of some weird kernel
internals between interfaces?

Why not make XDP more useful and more generic across interfaces? It's
very common for systems to be receiving packets with a heavy ethernet
card from the current data center, in addition to receiving packets
from a tunnel interface connected to a remote data center, with a need
to run the same XDP program on both interfaces. Why not support that
kind of simplicity?

This is _actually_ something that's come up _repeatedly_. This is a
real world need from real users who are doing real things. Why not
help them?

It's not at the expense of any formal consistency, or performance, or
even semantic perfection. It costs very little to support these
popular use cases.

[FYI, there's one tweak I'd like to make, so I'll probably send v5 ~soon.]

Jason

[PATCH net v5] 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]. Another user reported that modifying packets from layer 3
interfaces results in impossible to diagnose drops.

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 and so is
hard_header_len, then we know that the skb is 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.

We push on the ethernet header and then pull it right off and set
mac_len to the ethernet header size, so that the rest of the XDP code
does not need any changes. That is, it makes it so that the skb has its
ethernet header just before the data pointer, of size ETH_HLEN. While
we're at it, this also fixes a small inconsistency from the prior code,
in which an XDP program that changes skb->protocol would wind up pushing
the ethernet header back on but would forget to take it back off
following the h_proto parsing.

Previous discussions have included the point that maybe XDP should just
be intentionally broken on layer 3 interfaces, by design, and that layer
3 people should be using cls_bpf. However, I think there are good
grounds to reconsider this perspective:

- Complicated deployments wind up applying XDP modifications to a
  variety of different devices on a given host, some of which are using
  specialized ethernet cards and other ones using virtual layer 3
  interfaces, such as WireGuard. Being able to apply one codebase to
  each of these winds up being essential.

- cls_bpf does not support the same feature set as XDP, and operates at
  a slightly different stage in the networking stack. You may reply,
  "then add all the features you want to cls_bpf", but that seems to be
  missing the point, and would still result in there being two ways to
  do everything, which is not desirable for anyone actually _using_ this
  code.

- While XDP was originally made for hardware offloading, and while many
  look disdainfully upon the generic mode, it nevertheless remains a
  highly useful and popular way of adding bespoke packet
  transformations, and from that perspective, a difference between layer
  2 and layer 3 packets is immaterial if the user is primarily concerned
  with transformations to layer 3 and beyond.

[1] https://lore.kernel.org/wireguard/M5WzVK5--3-2@tuta.io/

Reported-by: Thomas Ptacek <thomas@sockpuppet.org>
Reported-by: Adhipati Blambangan <adhipati@tuta.io>
Cc: David Ahern <dsahern@gmail.com>
Cc: Toke Høiland-Jørgensen <toke@redhat.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---

I had originally dropped this patch, but the issue kept coming up in
user reports, so here's a v4 of it. Testing of it is still rather slim,
but hopefully that will change in the coming days.

Changes v4->v5:
- Rather than tracking in a messy manner whether the skb is l3, we just
  do the check once, and then adjust the skb geometry to be identical to
  the l2 case. This simplifies the code quite a bit.
- Fix a preexisting bug where the l2 header remained attached if
  skb->protocol was updated.

Changes v3->v4:
- We now preserve the same logic for XDP_TX/XDP_REDIRECT as before.
- hard_header_len is checked in addition to mac_len.

 net/core/dev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7df6c9617321..79c15f4244e6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4630,6 +4630,18 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	 * header.
 	 */
 	mac_len = skb->data - skb_mac_header(skb);
+	if (!mac_len && !skb->dev->hard_header_len) {
+		/* For l3 packets, we push on a fake mac header, and then
+		 * pull it off again, so that it has the same skb geometry
+		 * as for the l2 case.
+		 */
+		eth = skb_push(skb, ETH_HLEN);
+		eth_zero_addr(eth->h_source);
+		eth_zero_addr(eth->h_dest);
+		eth->h_proto = skb->protocol;
+		__skb_pull(skb, ETH_HLEN);
+		mac_len = ETH_HLEN;
+	}
 	hlen = skb_headlen(skb) + mac_len;
 	xdp->data = skb->data - mac_len;
 	xdp->data_meta = xdp->data;
@@ -4676,6 +4688,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
 		__skb_push(skb, ETH_HLEN);
 		skb->protocol = eth_type_trans(skb, skb->dev);
+		__skb_pull(skb, ETH_HLEN);
 	}
 
 	switch (act) {
-- 
2.28.0

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

[Jakub Kicinski]

On Fri, 14 Aug 2020 08:56:48 +0200 Jason A. Donenfeld wrote:
> On Thu, Aug 13, 2020 at 11:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > I had originally dropped this patch, but the issue kept coming up in
> > > user reports, so here's a v4 of it. Testing of it is still rather slim,
> > > but hopefully that will change in the coming days.  
> >
> > Here an alternative patch, untested:  
> 
> Funny. But come on now... Why would we want to deprive our users of
> system consistency?

We should try for consistency between xdp and cls_bpf instead.

> Doesn't it make sense to allow users to use the same code across
> interfaces? You actually want them to rewrite their code to use a
> totally different trigger point just because of some weird kernel
> internals between interfaces?

We're not building an abstraction over the kernel stack so that users
won't have to worry how things work. Users need to have a minimal
understanding of how specific hooks integrate with the stack and what
they are for. And therefore why cls_bpf is actually more efficient to
use in L3 tunnel case.

> Why not make XDP more useful and more generic across interfaces? It's
> very common for systems to be receiving packets with a heavy ethernet
> card from the current data center, in addition to receiving packets
> from a tunnel interface connected to a remote data center, with a need
> to run the same XDP program on both interfaces. Why not support that
> kind of simplicity?
> 
> This is _actually_ something that's come up _repeatedly_. This is a
> real world need from real users who are doing real things. Why not
> help them?

I'm sure it comes up repeatedly because we don't return any errors,
so people waste time investigating why it doesn't work.

> It's not at the expense of any formal consistency, or performance, or
> even semantic perfection. It costs very little to support these
> popular use cases.

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

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Fri, 14 Aug 2020 09:30:48 +0200

> @@ -4676,6 +4688,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
>  		__skb_push(skb, ETH_HLEN);
>  		skb->protocol = eth_type_trans(skb, skb->dev);
> +		__skb_pull(skb, ETH_HLEN);
>  	}
>  
>  	switch (act) {

This bug fix is separate from your other changes.  Please do not combine
them.

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

[Jason A. Donenfeld]

On 8/14/20, David Miller <davem@davemloft.net> wrote:
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Date: Fri, 14 Aug 2020 09:30:48 +0200
>
>> @@ -4676,6 +4688,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff
>> *skb,
>>  	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
>>  		__skb_push(skb, ETH_HLEN);
>>  		skb->protocol = eth_type_trans(skb, skb->dev);
>> +		__skb_pull(skb, ETH_HLEN);
>>  	}
>>
>>  	switch (act) {
>
> This bug fix is separate from your other changes.  Please do not combine
> them.
>

No problem. I'll split this out and resend tomorrow morning.

Jason

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

[Jason A. Donenfeld]

On 8/14/20, Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 14 Aug 2020 08:56:48 +0200 Jason A. Donenfeld wrote:
>> On Thu, Aug 13, 2020 at 11:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> > > I had originally dropped this patch, but the issue kept coming up in
>> > > user reports, so here's a v4 of it. Testing of it is still rather
>> > > slim,
>> > > but hopefully that will change in the coming days.
>> >
>> > Here an alternative patch, untested:
>>
>> Funny. But come on now... Why would we want to deprive our users of
>> system consistency?
>
> We should try for consistency between xdp and cls_bpf instead.

And still require users to reimplement their packet processing logic twice?

>
>> Doesn't it make sense to allow users to use the same code across
>> interfaces? You actually want them to rewrite their code to use a
>> totally different trigger point just because of some weird kernel
>> internals between interfaces?
>
> We're not building an abstraction over the kernel stack so that users
> won't have to worry how things work. Users need to have a minimal
> understanding of how specific hooks integrate with the stack and what
> they are for. And therefore why cls_bpf is actually more efficient to
> use in L3 tunnel case.

It's not like adding 7 lines of code constitutes adding an abstraction
layer. It's a pretty basic fix to make real things work for real
users. While you might argue that users should do something different,
you also can't deny that being able to hook up the same packet
processing to eth0, eth1, extrafancyeth2, and tun0 is a huge
convenience.

>
>> Why not make XDP more useful and more generic across interfaces? It's
>> very common for systems to be receiving packets with a heavy ethernet
>> card from the current data center, in addition to receiving packets
>> from a tunnel interface connected to a remote data center, with a need
>> to run the same XDP program on both interfaces. Why not support that
>> kind of simplicity?
>>
>> This is _actually_ something that's come up _repeatedly_. This is a
>> real world need from real users who are doing real things. Why not
>> help them?
>
> I'm sure it comes up repeatedly because we don't return any errors,
> so people waste time investigating why it doesn't work.

What? No. It comes up repeatedly because people want to reuse their
XDP processing logic with layer 3 devices. You might be right that if
we tell them to go away, maybe they will, but on the other hand, why
not make this actually work for them? It seems pretty easy to do, and
saves everyone a lot of time.

Are you worried about adding a branch to the
already-slower-and-discouraged non-hardware generic path? If so, I
wouldn't object if you wanted to put unlikely() around the branch
condition in that if statement.

Jason

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

[David Miller]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 14 Aug 2020 08:31:53 -0700

> I'm sure it comes up repeatedly because we don't return any errors,
> so people waste time investigating why it doesn't work.

+1

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

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Fri, 14 Aug 2020 23:04:56 +0200

> What? No. It comes up repeatedly because people want to reuse their
> XDP processing logic with layer 3 devices.

XDP is a layer 2 packet processing technology.  It assumes an L2
ethernet and/or VLAN header is going to be there.

Putting a pretend ethernet header there doesn't really change that.

[PATCH net v6] 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]. Another user reported that modifying packets from layer 3
interfaces results in impossible to diagnose drops.

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 and so is
hard_header_len, then we know that the skb is 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.

We push on the ethernet header and then pull it right off and set
mac_len to the ethernet header size, so that the rest of the XDP code
does not need any changes. That is, it makes it so that the skb has its
ethernet header just before the data pointer, of size ETH_HLEN.

Previous discussions have included the point that maybe XDP should just
be intentionally broken on layer 3 interfaces, by design, and that layer
3 people should be using cls_bpf. However, I think there are good
grounds to reconsider this perspective:

- Complicated deployments wind up applying XDP modifications to a
  variety of different devices on a given host, some of which are using
  specialized ethernet cards and other ones using virtual layer 3
  interfaces, such as WireGuard. Being able to apply one codebase to
  each of these winds up being essential.

- cls_bpf does not support the same feature set as XDP, and operates at
  a slightly different stage in the networking stack. You may reply,
  "then add all the features you want to cls_bpf", but that seems to be
  missing the point, and would still result in there being two ways to
  do everything, which is not desirable for anyone actually _using_ this
  code.

- While XDP was originally made for hardware offloading, and while many
  look disdainfully upon the generic mode, it nevertheless remains a
  highly useful and popular way of adding bespoke packet
  transformations, and from that perspective, a difference between layer
  2 and layer 3 packets is immaterial if the user is primarily concerned
  with transformations to layer 3 and beyond.

- It's not impossible to imagine layer 3 hardware (e.g. a WireGuard PCIe
  card) including eBPF/XDP functionality built-in. In that case, why
  limit XDP as a technology to only layer 2? Then, having generic XDP
  work for layer 3 would naturally fit as well.

[1] https://lore.kernel.org/wireguard/M5WzVK5--3-2@tuta.io/

Reported-by: Thomas Ptacek <thomas@sockpuppet.org>
Reported-by: Adhipati Blambangan <adhipati@tuta.io>
Cc: David Ahern <dsahern@gmail.com>
Cc: Toke Høiland-Jørgensen <toke@redhat.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---

I had originally dropped this patch, but the issue kept coming up in
user reports, so here's a v4 of it. Testing of it is still rather slim,
but hopefully that will change in the coming days.

Changes v5->v6:
- The fix to the skb->protocol changing case is now in a separate
  stand-alone patch, and removed from this one, so that it can be
  evaluated separately.

Changes v4->v5:
- Rather than tracking in a messy manner whether the skb is l3, we just
  do the check once, and then adjust the skb geometry to be identical to
  the l2 case. This simplifies the code quite a bit.
- Fix a preexisting bug where the l2 header remained attached if
  skb->protocol was updated.

Changes v3->v4:
- We now preserve the same logic for XDP_TX/XDP_REDIRECT as before.
- hard_header_len is checked in addition to mac_len.

 net/core/dev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 151f1651439f..79c15f4244e6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4630,6 +4630,18 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	 * header.
 	 */
 	mac_len = skb->data - skb_mac_header(skb);
+	if (!mac_len && !skb->dev->hard_header_len) {
+		/* For l3 packets, we push on a fake mac header, and then
+		 * pull it off again, so that it has the same skb geometry
+		 * as for the l2 case.
+		 */
+		eth = skb_push(skb, ETH_HLEN);
+		eth_zero_addr(eth->h_source);
+		eth_zero_addr(eth->h_dest);
+		eth->h_proto = skb->protocol;
+		__skb_pull(skb, ETH_HLEN);
+		mac_len = ETH_HLEN;
+	}
 	hlen = skb_headlen(skb) + mac_len;
 	xdp->data = skb->data - mac_len;
 	xdp->data_meta = xdp->data;
-- 
2.28.0

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

[Jason A. Donenfeld]

On Fri, Aug 14, 2020 at 11:27 PM David Miller <davem@davemloft.net> wrote:
>
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Date: Fri, 14 Aug 2020 23:04:56 +0200
>
> > What? No. It comes up repeatedly because people want to reuse their
> > XDP processing logic with layer 3 devices.
>
> XDP is a layer 2 packet processing technology.  It assumes an L2
> ethernet and/or VLAN header is going to be there.
>
> Putting a pretend ethernet header there doesn't really change that.

Actually, I wasn't aware that XDP was explicitly limited to L2-only,
as some kind of fundamental thing. A while back when this patchset
first came up, I initially posted something that gave unmodified L3
frames to XDP programs in the generic handler, but commenters pointed
out that this loses the skb->protocol changing capability, which could
be useful (e.g. some kind of custom v4->v6 modifying code), and adding
the pretend ethernet header would allow people to keep the same
programs for the L2 case as for the L3 case, which seemed *extremely*
compelling to me. Hence, this patch series has gone in the pretend
ethernet header direction.

But, anyway, as I said, I wasn't aware that XDP was explicitly limited
to L2-only, as some kind of fundamental thing. This actually surprises
me. I always thought the original motivation of XDP was that it
allowed putting a lot of steering and modification logic into the
network card hardware, for fancy new cards that support eBPF. Later,
the generic handler got added on so people could reuse those programs
in heterogeneous environments, where some cards have hardware support
and others do not. That seemed like a good idea to me. Extending that
a step further for layer 3 devices seems like a logical next step, in
the sense that if that step is invalid, surely the previous one of
adding the generic handler must be invalid too? At least that's my
impression of the historical evolution of this; I'm confident you know
much better than I do.

It makes me wonder, though, what will you say when hardware comes out
that has layer 3 semantics and a thirst for eBPF? Also deny that
hardware of XDP, because "XDP is a layer 2 packet processing
technology"? I know what you'll say now: "we don't design our
networking stack around hypothetical hardware, so why even bring it
up? I won't entertain that." But nevertheless, contemplating those
hypotheticals might be a good exercise for seeing how committed you
are to the XDP=L2-only assertion. For example, maybe there are
fundamental L2 semantics that XDP needs, and can't be emulated with my
pretend ethernet header -- like if you really are relying on the MACs
for something I'm not aware of; were that the case, it'd be compelling
to me. But if it's a bit weaker, in the form of, "we just don't want
to try anything with L3 at all because," then I admit I'm still a bit
mystified.

Nevertheless, at the risk of irritating you further, I will willingly
drop this patchset at your request, even though I don't completely
understand the entire scope of reasoning for doing so. (For posterity,
I just posted a v6, which splits out that other bug fix into something
separate for you to take, so that this one here can exist on its own.)

Jason

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

[Jesper Dangaard Brouer]

On Sat, 15 Aug 2020 09:41:02 +0200
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> A user reported that packets from wireguard were possibly ignored by XDP
> [1]. Another user reported that modifying packets from layer 3
> interfaces results in impossible to diagnose drops.
> 
> 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 and so is
> hard_header_len, then we know that the skb is 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.
> 
> We push on the ethernet header and then pull it right off and set
> mac_len to the ethernet header size, so that the rest of the XDP code
> does not need any changes. That is, it makes it so that the skb has its
> ethernet header just before the data pointer, of size ETH_HLEN.
> 
> Previous discussions have included the point that maybe XDP should just
> be intentionally broken on layer 3 interfaces, by design, and that layer
> 3 people should be using cls_bpf. However, I think there are good
> grounds to reconsider this perspective:
> 
> - Complicated deployments wind up applying XDP modifications to a
>   variety of different devices on a given host, some of which are using
>   specialized ethernet cards and other ones using virtual layer 3
>   interfaces, such as WireGuard. Being able to apply one codebase to
>   each of these winds up being essential.
> 
> - cls_bpf does not support the same feature set as XDP, and operates at
>   a slightly different stage in the networking stack. You may reply,
>   "then add all the features you want to cls_bpf", but that seems to be
>   missing the point, and would still result in there being two ways to
>   do everything, which is not desirable for anyone actually _using_ this
>   code.
> 
> - While XDP was originally made for hardware offloading, and while many
>   look disdainfully upon the generic mode, it nevertheless remains a
>   highly useful and popular way of adding bespoke packet
>   transformations, and from that perspective, a difference between layer
>   2 and layer 3 packets is immaterial if the user is primarily concerned
>   with transformations to layer 3 and beyond.
> 
> - It's not impossible to imagine layer 3 hardware (e.g. a WireGuard PCIe
>   card) including eBPF/XDP functionality built-in. In that case, why
>   limit XDP as a technology to only layer 2? Then, having generic XDP
>   work for layer 3 would naturally fit as well.
> 
> [1] https://lore.kernel.org/wireguard/M5WzVK5--3-2@tuta.io/
> 
> Reported-by: Thomas Ptacek <thomas@sockpuppet.org>
> Reported-by: Adhipati Blambangan <adhipati@tuta.io>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Toke Høiland-Jørgensen <toke@redhat.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> 
> I had originally dropped this patch, but the issue kept coming up in
> user reports, so here's a v4 of it. Testing of it is still rather slim,
> but hopefully that will change in the coming days.
> 
> Changes v5->v6:
> - The fix to the skb->protocol changing case is now in a separate
>   stand-alone patch, and removed from this one, so that it can be
>   evaluated separately.
> 
> Changes v4->v5:
> - Rather than tracking in a messy manner whether the skb is l3, we just
>   do the check once, and then adjust the skb geometry to be identical to
>   the l2 case. This simplifies the code quite a bit.
> - Fix a preexisting bug where the l2 header remained attached if
>   skb->protocol was updated.
> 
> Changes v3->v4:
> - We now preserve the same logic for XDP_TX/XDP_REDIRECT as before.
> - hard_header_len is checked in addition to mac_len.
> 
>  net/core/dev.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 151f1651439f..79c15f4244e6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4630,6 +4630,18 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	 * header.
>  	 */
>  	mac_len = skb->data - skb_mac_header(skb);
> +	if (!mac_len && !skb->dev->hard_header_len) {
> +		/* For l3 packets, we push on a fake mac header, and then
> +		 * pull it off again, so that it has the same skb geometry
> +		 * as for the l2 case.
> +		 */
> +		eth = skb_push(skb, ETH_HLEN);
> +		eth_zero_addr(eth->h_source);
> +		eth_zero_addr(eth->h_dest);
> +		eth->h_proto = skb->protocol;
> +		__skb_pull(skb, ETH_HLEN);
> +		mac_len = ETH_HLEN;
> +	}

You are consuming a little bit of the headroom here.

>  	hlen = skb_headlen(skb) + mac_len;
>  	xdp->data = skb->data - mac_len;
>  	xdp->data_meta = xdp->data;

The XDP-prog is allowed to change eth->h_proto.  Later (in code) we
detect this and update skb->protocol with the new protocol.

What will happen if my XDP-prog adds a VLAN header?

The selftest tools/testing/selftests/bpf/test_xdp_vlan.sh test these
situations.  You can use it as an example, and write/construct a test
that does the same for your Wireguard devices.  As minimum you need to
provide such a selftest together with this patch.

Generally speaking, IMHO generic-XDP was a mistake, because it is hard
to maintain and slows down the development of XDP.  (I have a number of
fixes in my TODO backlog for generic-XDP).  Adding this will just give
us more corner-cases that need to be maintained.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

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

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Sat, 15 Aug 2020 09:41:02 +0200

> A user reported that packets from wireguard were possibly ignored by XDP
> [1]. Another user reported that modifying packets from layer 3
> interfaces results in impossible to diagnose drops.

Jason this really is a minefield.

If you make everything look like ethernet, even when it isn't, that is
a huge pile of worms.

If the XDP program changes the fake ethernet header's protocol field,
what will update the next protocol field in the wireguard
encapsulation headers so that it matches?

How do you support pushing VLAN headers as some XDP programs do?  What
will undo the fake ethernet header and push the VLAN header into the
right place, and set it's next protocol field correctly?

And so on, and so forth...

With so many unanswered questions and unclear semantics the only
reasonable approach right now is to reject L3 devices from having XDP
programs attached at this time.

Arguably the best answer is the hardest answer, which is that we
expose device protocols and headers exactly how they are and don't try
to pretend they are something else.  But it really means that XDP
programs have to be written targetted to the attach point device type.
And it also means we need a way to update skb->protocol properly,
handle the pushing of new headers, etc.

In short, you can't just push a fake ethernet header and expect
everything to just work.

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

[Jason A. Donenfeld]

On Thu, Aug 20, 2020 at 1:22 AM David Miller <davem@davemloft.net> wrote:
>
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Date: Sat, 15 Aug 2020 09:41:02 +0200
>
> > A user reported that packets from wireguard were possibly ignored by XDP
> > [1]. Another user reported that modifying packets from layer 3
> > interfaces results in impossible to diagnose drops.
>
> Jason this really is a minefield.
>
> If you make everything look like ethernet, even when it isn't, that is
> a huge pile of worms.
>
> If the XDP program changes the fake ethernet header's protocol field,
> what will update the next protocol field in the wireguard
> encapsulation headers so that it matches?
>
> How do you support pushing VLAN headers as some XDP programs do?  What
> will undo the fake ethernet header and push the VLAN header into the
> right place, and set it's next protocol field correctly?
>
> And so on, and so forth...

Huh, that's an interesting set of considerations. It looks like after
the generic XDP program runs, there's a call to
skb_vlan_untag()->skb_reorder_vlan_header() if skb->protocol is 8021q
or 8021qad, which makes me think the stack will just do the right
thing? I'm probably overlooking some critical detail that you and
Jesper find clear. My understanding of the generic XDP handler for L2
packets is:

1. They arrive with skb->data pointing at L3, but skb->data - mac_len
is the L2 header.
2. This skb->data - mac_len pointer is what's passed to the eBPF executor.
3. When it's done, skb->data still points to the L3 data, but the eBPF
program might have pushed some things on before that or altered the
ethernet header.
4. If the ethernet header's h_proto is changed, so skb->protocol is
updated (along with the broadcast/multicast flag too).
5. The skb is passed onto the rest of the stack, with skb->data still
pointing to L3, but with L2 existing in the area just before
skb->data, just like how it came in.

This patch attempts to add L3 semantics that slightly modify the flow
for L3 packets:

1. They arrive with skb->data pointing at L3, with nothing coherent
before skb->data.
2. An ethernet header is pushed onto the packet, and then pulled off
again, so that skb->data points at L3 but skb->data - ETH_HLEN points
to the fake L2.
3. Steps 2-5 from the above flow now apply.

It seems like if an eBPF program pushes on a VLAN tag or changes the
protocol or does any other modification, it will be treated in exactly
the same way as the L2 packet above by the remaining parts of the
networking stack.

However, Jesper points out in his previous message (I think) that by
only calling skb_push(skb, ETH_HLEN), I'm not actually increasing the
head room enough for eBPF programs to safely tack on vlan tags and
other things. In other words, I need to increase the head room more,
beyond a measly ETH_HLEN. That seems like an easy change.

> With so many unanswered questions and unclear semantics the only
> reasonable approach right now is to reject L3 devices from having XDP
> programs attached at this time.

I don't know if there are _so_ many unanswered questions, but it seems
like there remain some unknowns, but Jesper has made a good suggestion
that I start going through that test suite and make sure that
everything works properly there. It might be that one test starts
failing catastrophically, and when I investigate I find that there's
not a very clear cut answer as to how to fix it, reinforcing your
point. Or, perhaps it will all kind of work nicely without scary or
fundamental changes required. Mostly out of my own curiosity, I'll
give it a try when I'm at my desk again and report back.

> Arguably the best answer is the hardest answer, which is that we
> expose device protocols and headers exactly how they are and don't try
> to pretend they are something else.  But it really means that XDP
> programs have to be written targetted to the attach point device type.
> And it also means we need a way to update skb->protocol properly,
> handle the pushing of new headers, etc.

That's actually where this patch started many months ago, with just a
simple change to quit trying to cast skb->data-mac_len to an ethhdr in
the case that it's an L3 packet. Of course indeed that didn't address
skb->protocol. And it also didn't help L3 packets _become_ L2 packets,
which might be desirable. And in general it would mean that no
existing XDP programs would work with it, as Toke pointed out. So I
think if the pseudo ethernet header winds up being actually doable and
consistent, that's probably the best approach. You seem skeptical that
it is actually consistent, and you're probably right. I'll let you
know if I notice otherwise though, once I get that test suite rolling.

Jason

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

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Thu, 20 Aug 2020 11:13:49 +0200

> It seems like if an eBPF program pushes on a VLAN tag or changes the
> protocol or does any other modification, it will be treated in exactly
> the same way as the L2 packet above by the remaining parts of the
> networking stack.

What will update the skb metadata if the XDP program changes the
wireguard header(s)?

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

[Jason A. Donenfeld]

On 8/20/20, David Miller <davem@davemloft.net> wrote:
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Date: Thu, 20 Aug 2020 11:13:49 +0200
>
>> It seems like if an eBPF program pushes on a VLAN tag or changes the
>> protocol or does any other modification, it will be treated in exactly
>> the same way as the L2 packet above by the remaining parts of the
>> networking stack.
>
> What will update the skb metadata if the XDP program changes the
> wireguard header(s)?
>

XDP runs after decryption/decapsulation, in the netif_rx path, which
means there is no wireguard header at that point. All the wireguard
crypto/udp/header stuff is all inside the driver itself, and the rest
of the stack just deals in terms of plain vanilla L3 ipv4/ipv6
packets.

The skb->protocol metadata is handled by the fake ethernet header.

Is there other metadata I should keep in mind? WireGuard doesn't play
with skb_metadata_*, for example. (Though it may implicitly reach a
skb_metadata_clear via pskb_expand path.)