Message-ID: <75b021a8-88cb-b44c-fd97-e34be83e702f@6wind.com>
Date: Thu, 3 Aug 2023 22:05:24 +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 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

Powered by blists - more mailing lists