netdev - Re: [PATCH net v2] net: handle ARPHRD_PPP in dev_is_mac_header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 4 Aug 2023 15:28:08 +0200
From: Guillaume Nault <gnault@...hat.com>
To: Nicolas Dichtel <nicolas.dichtel@...nd.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()

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
>