Message-ID: <62a8762c-40b4-f03f-ca8f-13d33db84f10@6wind.com>
Date: Thu, 3 Aug 2023 14:22:17 +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 à 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

Powered by blists - more mailing lists