Le 04/08/2023 à 15:28, Guillaume Nault a écrit : > 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 :(. Thanks for the detailed explanations. Regards, Nicolas
