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. Regards, Nicolas
