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

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

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>

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