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: Thu, 3 Aug 2023 13:00:54 +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 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@...r.kernel.org
> >> Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper")
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
> >> Tested-by: Siwar Zitouni <siwar.zitouni@...nd.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
> >>
> >>
> > 
>