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/driv...
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