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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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>