[PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Nicolas Dichtel]

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;
-- 
2.39.2

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Guillaume Nault]

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
> 
> 

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Nicolas Dichtel]

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
>>
>>
> 

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Guillaume Nault]

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
> >>
> >>
> > 
> 

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Nicolas Dichtel]

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

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Nicolas Dichtel]

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

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Guillaume Nault]

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 :(.

> Regards,
> Nicolas
> 

Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

[Nicolas Dichtel]

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