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

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

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 27 Apr 2020 23:14:05 +0200 Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> > On Mon, 27 Apr 2020 13:52:54 -0700 Jakub Kicinski wrote:  
>> >> On Mon, 27 Apr 2020 14:42:08 -0600 Jason A. Donenfeld wrote:  
>> >> > A user reported that packets from wireguard were possibly ignored by XDP
>> >> > [1]. 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, then we assume
>> >> > that it's 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.  
>> >> 
>> >> Is this going to work correctly with XDP_TX? presumably wireguard
>> >> doesn't want the ethernet L2 on egress, either? And what about
>> >> redirects?
>> >> 
>> >> I'm not sure we can paper over the L2 differences between interfaces.
>> >> Isn't user supposed to know what interface the program is attached to?
>> >> I believe that's the case for cls_bpf ingress, right?  
>> >
>> > In general we should also ask ourselves if supporting XDPgeneric on
>> > software interfaces isn't just pointless code bloat, and it wouldn't
>> > be better to let XDP remain clearly tied to the in-driver native use
>> > case.  
>> 
>> I was mostly ignoring generic XDP for a long time for this reason. But
>> it seems to me that people find generic XDP quite useful, so I'm no
>> longer so sure this is the right thing to do...
>
> I wonder, maybe our documentation is not clear. IOW we were saying that
> XDP is a faster cls_bpf, which leaves out the part that XDP only makes
> sense for HW/virt devices.

I'm not sure it's just because people think it's faster. There's also a
semantic difference; if you just want to do ingress filtering, simply
sticking an XDP program on the interface is a natural fit. Whereas
figuring out the tc semantics for ingress is non-trivial. And also
reusability of XDP programs from the native hook is an important
consideration, I believe. Which is also why I think the pseudo-MAC
header approach is the right fix for L3 devices :)

> Kinda same story as XDP egress, folks may be asking for it but that
> doesn't mean it makes sense.

Well I do also happen to think that XDP egress is a good idea ;)

> Perhaps the original reporter realized this and that's why they
> disappeared?
>
> My understanding is that XDP generic is aimed at testing and stop gap
> for drivers which don't implement native. Defining behavior based on
> XDP generic's needs seems a little backwards, and risky.

That I can agree with - generic XDP should follow the semantics of
native XDP, not the other way around. But that's what we're doing here
(with the pseudo-MAC header approach), isn't it? Whereas if we were
saying "just write your XDP programs to assume only L3 packets" we would
be creating a new semantic for generic XDP...

-Toke