[PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

[PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

[Martin Willi]

If a generic XDP program changes the destination MAC address from/to
multicast/broadcast, the skb->pkt_type is updated to properly handle
the packet when passed up the stack. When changing the MAC from/to
the NICs MAC, PACKET_HOST/OTHERHOST is not updated, though, making
the behavior different from that of native XDP.

Remember the PACKET_HOST/OTHERHOST state before calling the program
in generic XDP, and update pkt_type accordingly if the destination
MAC address has changed. As eth_type_trans() assumes a default
pkt_type of PACKET_HOST, restore that before calling it.

The use case for this is when a XDP program wants to push received
packets up the stack by rewriting the MAC to the NICs MAC, for
example by cluster nodes sharing MAC addresses.

Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was mangled")
Signed-off-by: Martin Willi <martin@strongswan.org>
---
 net/core/dev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d9bf63dbe4fd..eed028aec6a4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4723,10 +4723,10 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	void *orig_data, *orig_data_end, *hard_start;
 	struct netdev_rx_queue *rxqueue;
 	u32 metalen, act = XDP_DROP;
+	bool orig_bcast, orig_host;
 	u32 mac_len, frame_sz;
 	__be16 orig_eth_type;
 	struct ethhdr *eth;
-	bool orig_bcast;
 	int off;
 
 	/* Reinjected packets coming from act_mirred or similar should
@@ -4773,6 +4773,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	orig_data_end = xdp->data_end;
 	orig_data = xdp->data;
 	eth = (struct ethhdr *)xdp->data;
+	orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);
 	orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest);
 	orig_eth_type = eth->h_proto;
 
@@ -4800,8 +4801,11 @@ 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_host != ether_addr_equal_64bits(eth->h_dest,
+						  skb->dev->dev_addr)) ||
 	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
 		__skb_push(skb, ETH_HLEN);
+		skb->pkt_type = PACKET_HOST;
 		skb->protocol = eth_type_trans(skb, skb->dev);
 	}
 
-- 
2.25.1

Re: [PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

[Toke Høiland-Jørgensen]

Martin Willi <martin@strongswan.org> writes:

> If a generic XDP program changes the destination MAC address from/to
> multicast/broadcast, the skb->pkt_type is updated to properly handle
> the packet when passed up the stack. When changing the MAC from/to
> the NICs MAC, PACKET_HOST/OTHERHOST is not updated, though, making
> the behavior different from that of native XDP.
>
> Remember the PACKET_HOST/OTHERHOST state before calling the program
> in generic XDP, and update pkt_type accordingly if the destination
> MAC address has changed. As eth_type_trans() assumes a default
> pkt_type of PACKET_HOST, restore that before calling it.
>
> The use case for this is when a XDP program wants to push received
> packets up the stack by rewriting the MAC to the NICs MAC, for
> example by cluster nodes sharing MAC addresses.
>
> Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was mangled")
> Signed-off-by: Martin Willi <martin@strongswan.org>
> ---
>  net/core/dev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d9bf63dbe4fd..eed028aec6a4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4723,10 +4723,10 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	void *orig_data, *orig_data_end, *hard_start;
>  	struct netdev_rx_queue *rxqueue;
>  	u32 metalen, act = XDP_DROP;
> +	bool orig_bcast, orig_host;
>  	u32 mac_len, frame_sz;
>  	__be16 orig_eth_type;
>  	struct ethhdr *eth;
> -	bool orig_bcast;
>  	int off;
>  
>  	/* Reinjected packets coming from act_mirred or similar should
> @@ -4773,6 +4773,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	orig_data_end = xdp->data_end;
>  	orig_data = xdp->data;
>  	eth = (struct ethhdr *)xdp->data;
> +	orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);

ether_addr_equal_64bits() seems to assume that the addresses passed to
it are padded to be 8 bytes long, which is not the case for eth->h_dest.
AFAICT the only reason the _64bits variant works for multicast is that
it happens to be only checking the top-most bit, but unless I'm missing
something you'll have to use the boring old ether_addr_equal() here, no?

>  	orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest);
>  	orig_eth_type = eth->h_proto;
>  
> @@ -4800,8 +4801,11 @@ 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_host != ether_addr_equal_64bits(eth->h_dest,
> +						  skb->dev->dev_addr)) ||
>  	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
>  		__skb_push(skb, ETH_HLEN);
> +		skb->pkt_type = PACKET_HOST;
>  		skb->protocol = eth_type_trans(skb, skb->dev);
>  	}

Okay, so this was a bit confusing to me at fist glance: eth_type_trans()
will reset the type, but not back to PACKET_HOST. So this works, just a
bit confusing :)

-Toke

Re: [PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

[Martin Willi]

Hi,

Thanks for your comments.

> >  	eth = (struct ethhdr *)xdp->data;
> > +	orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);
> 
> ether_addr_equal_64bits() seems to assume that the addresses passed to 
> it are padded to be 8 bytes long, which is not the case for eth->h_dest.
> AFAICT the only reason the _64bits variant works for multicast is that
> it happens to be only checking the top-most bit, but unless I'm missing
> something you'll have to use the boring old ether_addr_equal() here, no?

This is what eth_type_trans() uses below, so I assumed it is safe to
use. Isn't that working on the same data?

Also, the destination address in Ethernet is followed by the source
address, so two extra bytes in the source are used as padding. These
are then shifted out by ether_addr_equal_64bits(), no?

> > +		skb->pkt_type = PACKET_HOST;
> >  		skb->protocol = eth_type_trans(skb, skb->dev);
> >  	}
> 
> Okay, so this was a bit confusing to me at fist glance:
> eth_type_trans() will reset the type, but not back to PACKET_HOST. So
> this works, just a bit confusing :)

Indeed. I considered changing eth_type_trans() to always reset
pkt_type, but I didn't want to take the risk for any side effects.

Thanks!
Martin

Re: [PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

[Toke Høiland-Jørgensen]

Martin Willi <martin@strongswan.org> writes:

> Hi,
>
> Thanks for your comments.
>
>> >  	eth = (struct ethhdr *)xdp->data;
>> > +	orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);
>> 
>> ether_addr_equal_64bits() seems to assume that the addresses passed to 
>> it are padded to be 8 bytes long, which is not the case for eth->h_dest.
>> AFAICT the only reason the _64bits variant works for multicast is that
>> it happens to be only checking the top-most bit, but unless I'm missing
>> something you'll have to use the boring old ether_addr_equal() here, no?
>
> This is what eth_type_trans() uses below, so I assumed it is safe to
> use. Isn't that working on the same data?
>
> Also, the destination address in Ethernet is followed by the source
> address, so two extra bytes in the source are used as padding. These
> are then shifted out by ether_addr_equal_64bits(), no?

Ohh, you're right, it's shifting off the two extra bytes afterwards.
Clever! I obviously missed that, but yeah, that means it just needs the
two extra bytes to not be out-of-bounds reads, so this usage should be
fine :)

>> > +		skb->pkt_type = PACKET_HOST;
>> >  		skb->protocol = eth_type_trans(skb, skb->dev);
>> >  	}
>> 
>> Okay, so this was a bit confusing to me at fist glance:
>> eth_type_trans() will reset the type, but not back to PACKET_HOST. So
>> this works, just a bit confusing :)
>
> Indeed. I considered changing eth_type_trans() to always reset
> pkt_type, but I didn't want to take the risk for any side effects.

Hmm, yeah, it does seem there are quite a few call sites to audit if you
were to change the behaviour. I guess we'll have to live with the slight
confusion, then :)

-Toke


Given the above:

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

Re: [PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (refs/heads/master):

On Mon, 19 Apr 2021 16:15:59 +0200 you wrote:
> If a generic XDP program changes the destination MAC address from/to
> multicast/broadcast, the skb->pkt_type is updated to properly handle
> the packet when passed up the stack. When changing the MAC from/to
> the NICs MAC, PACKET_HOST/OTHERHOST is not updated, though, making
> the behavior different from that of native XDP.
> 
> Remember the PACKET_HOST/OTHERHOST state before calling the program
> in generic XDP, and update pkt_type accordingly if the destination
> MAC address has changed. As eth_type_trans() assumes a default
> pkt_type of PACKET_HOST, restore that before calling it.
> 
> [...]

Here is the summary with links:
  - [net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC
    https://git.kernel.org/bpf/bpf-next/c/22b6034323fd

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html