This kind of interface doesn't have a mac header. This patch fixes bpf_redirect() to a ppp interface.
CC: stable@vger.kernel.org Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper") Signed-off-by: Nicolas Dichtel nicolas.dichtel@6wind.com Tested-by: Siwar Zitouni siwar.zitouni@6wind.com ---
v1 -> v2: - I forgot the 'Tested-by' tag in the v1 :/
include/linux/if_arp.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h index 1ed52441972f..8efbe29a6f0c 100644 --- a/include/linux/if_arp.h +++ b/include/linux/if_arp.h @@ -53,6 +53,7 @@ static inline bool dev_is_mac_header_xmit(const struct net_device *dev) case ARPHRD_NONE: case ARPHRD_RAWIP: case ARPHRD_PIMREG: + case ARPHRD_PPP: return false; default: return true;
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. 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?
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 best we can do given the current state of ppp_generic.c, but the commit message should be clear about what the real problem is and why the patch takes this approach to fix or work around it.
CC: stable@vger.kernel.org Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper") Signed-off-by: Nicolas Dichtel nicolas.dichtel@6wind.com Tested-by: Siwar Zitouni siwar.zitouni@6wind.com
v1 -> v2:
- I forgot the 'Tested-by' tag in the v1 :/
include/linux/if_arp.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h index 1ed52441972f..8efbe29a6f0c 100644 --- a/include/linux/if_arp.h +++ b/include/linux/if_arp.h @@ -53,6 +53,7 @@ static inline bool dev_is_mac_header_xmit(const struct net_device *dev) case ARPHRD_NONE: case ARPHRD_RAWIP: case ARPHRD_PIMREG:
- case ARPHRD_PPP: return false; default: return true;
-- 2.39.2
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.
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. In this case, the bpf_redirect() function should remove the ethernet header from the packet before calling the xmit ppp function. 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.
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().
Hope it's more clear.
Regards, Nicolas
best we can do given the current state of ppp_generic.c, but the commit message should be clear about what the real problem is and why the patch takes this approach to fix or work around it.
CC: stable@vger.kernel.org Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper") Signed-off-by: Nicolas Dichtel nicolas.dichtel@6wind.com Tested-by: Siwar Zitouni siwar.zitouni@6wind.com
v1 -> v2:
- I forgot the 'Tested-by' tag in the v1 :/
include/linux/if_arp.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h index 1ed52441972f..8efbe29a6f0c 100644 --- a/include/linux/if_arp.h +++ b/include/linux/if_arp.h @@ -53,6 +53,7 @@ static inline bool dev_is_mac_header_xmit(const struct net_device *dev) case ARPHRD_NONE: case ARPHRD_RAWIP: case ARPHRD_PIMREG:
- case ARPHRD_PPP: return false; default: return true;
-- 2.39.2
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.
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.
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.
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:
Regards, Nicolas
best we can do given the current state of ppp_generic.c, but the commit message should be clear about what the real problem is and why the patch takes this approach to fix or work around it.
CC: stable@vger.kernel.org Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper") Signed-off-by: Nicolas Dichtel nicolas.dichtel@6wind.com Tested-by: Siwar Zitouni siwar.zitouni@6wind.com
v1 -> v2:
- I forgot the 'Tested-by' tag in the v1 :/
include/linux/if_arp.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h index 1ed52441972f..8efbe29a6f0c 100644 --- a/include/linux/if_arp.h +++ b/include/linux/if_arp.h @@ -53,6 +53,7 @@ static inline bool dev_is_mac_header_xmit(const struct net_device *dev) case ARPHRD_NONE: case ARPHRD_RAWIP: case ARPHRD_PIMREG:
- case ARPHRD_PPP: return false; default: return true;
-- 2.39.2
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.
Regards, Nicolas
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
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/driv...
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
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/driv...
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
linux-stable-mirror@lists.linaro.org
-
Guillaume Nault -
Nicolas Dichtel