Le 03/08/2023 à 14:22, Nicolas Dichtel a écrit : > 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 > >> >>>> 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. > >> >>> In this case, the bpf_redirect() function >>> should remove the ethernet header from the packet before calling the xmit ppp >>> function. >> >> That's what you need for your specific use case, not necessarily what >> the code "should" do. > At least, it was my understanding of bpf_redirect() (: > >> >>> Before my patch, the ppp xmit function adds a ppp header (protocol IP >>> / 0x0021) before the ethernet header. It results to a corrupted packet. After >>> the patch, the ppp xmit function encapsulates the IP packet, as expected. >> >> The problem is to treat the PPP link layer differently from the >> Ethernet one. >> >> Just try to redirect PPP frames to an Ethernet device. The PPP l2 >> header isn't going to be stripped, and no Ethernet header will be >> automatically added. >> >> Before your patch, bridging incompatible L2 protocols just didn't work. >> After your patch, some combinations work, some don't, Ethernet is >> handled in one way, PPP in another way. And these inconsistencies are >> exposed to user space. That's the problem I have with this patch. >> >>>> To me this looks like a hack to work around the fact that >>>> ppp_start_xmit() automatically adds a PPP header. Maybe that's the >>> It's not an hack, it works like for other kind of devices managed by the >>> function bpf_redirect() / dev_is_mac_header_xmit(). >> >> I don't think the users of dev_is_mac_header_xmit() (BPF redirect and >> TC mirred) actually work correctly with any non-Ethernet l2 devices. >> L3 devices are a bit different because we can test if an skb has a >> zero-length l2 header. >> >>> Hope it's more clear. >> >> 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.). > 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. > > Without my patch, a redirect from a ppp interface to another ppp interface would > have the same problem. I will be off for 15 days, I will come back on this when I return. Regards, Nicolas
