Message-ID: <856c3f29-aa38-4cf9-ae8f-a46279c3e262@6wind.com>
Date: Wed, 23 Aug 2023 15:38:28 +0200
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Guillaume Nault <gnault@...hat.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, Daniel Borkmann <daniel@...earbox.net>,
 Alexei Starovoitov <ast@...nel.org>,
 John Fastabend <john.fastabend@...il.com>, netdev@...r.kernel.org,
 bpf@...r.kernel.org, stable@...r.kernel.org,
 Siwar Zitouni <siwar.zitouni@...nd.com>
Subject: Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header_xmit()

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

Powered by blists - more mailing lists