On Thu, Aug 03, 2023 at 02:22:17PM +0200, Nicolas Dichtel wrote: > Le 03/08/2023 à 13:00, Guillaume Nault a écrit : > > On Thu, Aug 03, 2023 at 11:37:00AM +0200, Nicolas Dichtel wrote: > >> Le 03/08/2023 à 10:46, Guillaume Nault a écrit : > >>> On Wed, Aug 02, 2023 at 02:21:06PM +0200, Nicolas Dichtel wrote: > >>>> This kind of interface doesn't have a mac header. > >>> > >>> Well, PPP does have a link layer header. > >> It has a link layer, but not an ethernet header. > > > > This is generic code. The layer two protocol involved doesn't matter. > > What matter is that the device requires a specific l2 header. > Ok. Note, that addr_len is set to 0 for these devices: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ppp/ppp_generic.c#n1614 PPP has no hardware address. It doesn't need any since it's point to point. But it still has an l2 header. > >>> Do you instead mean that PPP automatically adds it? > >>> > >>>> This patch fixes bpf_redirect() to a ppp interface. > >>> > >>> Can you give more details? Which kind of packets are you trying to > >>> redirect to PPP interfaces? > >> My ebpf program redirect an IP packet (eth / ip) from a physical ethernet device > >> at ingress to a ppp device at egress. > > > > So you're kind of bridging two incompatible layer two protocols. > > I see no reason to be surprised if that doesn't work out of the box. > I don't see the difference with a gre or ip tunnel. This kind of "bridging" is > supported. >From a protocol point of view, this feature just needs to strip the l2 header (or add it for the other way around). Here we have to remove the previous l2 header, then add a new one of a different kind. But honestly, even for the l3-tunnel<->Ethernet "bridging", I don't really like how the code tries to be too clever. It'd have been much simpler to just require the user to drop the l2 headers explicitely. Anyway, that ship has sailed. > > Let me be clearer too. As I said, this patch may be the best we can do. > > Making a proper l2 generic BPF-redirect/TC-mirred might require too > > much work for the expected gain (how many users of non-Ethernet l2 > > devices are going to use this). But at least we should make it clear in > > the commit message and in the code why we're finding it convenient to > > treat PPP as an l3 device. Like > > > > + /* PPP adds its l2 header automatically in ppp_start_xmit(). > > + * This makes it look like an l3 device to __bpf_redirect() and > > + * tcf_mirred_init(). > > + */ > > + case ARPHRD_PPP: > I better understand your point with this comment, I can add it, no problem. > But I fail to see why it is different from a L3 device. ip, gre, etc. tunnels > also add automatically another header (ipip.c has dev->addr_len configured to 4, > ip6_tunnels.c to 16, etc.). These are encapsulation protocols. They glue the inner and outer packets together. PPP doesn't do that, it's just an l2 protocol. To encapsulate PPP into IP or UDP, you need another protocol, like L2TP. We can compare GRE or IPIP to L2TP (to some extend), not to PPP. > A tcpdump on the physical output interface shows the same kind of packets (the > outer hdr (ppp / ip / etc.) followed by the encapsulated packet and a tcpdump on > the ppp or ip tunnel device shows only the inner packet. Packets captured on ppp interfaces seem to be a bit misleading. They don't show the l2-header, but the "Linux cooked capture" header instead. I don't know the reasoning behind that, maybe to help people differenciate between Rx and Tx packets. Anyway, that's different from the raw IP packets captured on ipip devices for example. Really, PPP isn't like any ip tunnel protocol. It's just not an encapsulation protocol. PPP is like Ethernet. And just like Ethernet, it can be encapsulated by tunnels, but that requires a separate tunneling protocol. As an example, Ethernet has VXLAN and PPP has L2TP. > Without my patch, a redirect from a ppp interface to another ppp interface would > have the same problem. True, but that's because the PPP code is so old and unmaintained, it hasn't evolved with the rest of the networking stack. And again, I agree that your patch is the easiest way to make it work. But it will also expose inconsistencies in how BPF and tc-mirred handle different l2 protocols. That makes the logic hard to get from a developper point of view and that's why I'm asking for a better commit message and some comments in the code. For the user space inconsistencies, well, I guess nobody will really care :(. > Regards, > Nicolas >
