Martin Willi <martin@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
