netdev - Re: [PATCH net-next 0/5] tc: adjust network header after second vlan push

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]

Message-ID: <ZrysAhVp8AaxPz4b@noodle>
Date: Wed, 14 Aug 2024 16:07:14 +0300
From: Boris Sukholitko <boris.sukholitko@...adcom.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	Mina Almasry <almasrymina@...gle.com>,
	Pavel Begunkov <asml.silence@...il.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	David Howells <dhowells@...hat.com>,
	Ilya Lifshits <ilya.lifshits@...adcom.com>
Subject: Re: [PATCH net-next 0/5] tc: adjust network header after second vlan
 push

On Mon, Aug 12, 2024 at 05:40:47PM -0700, Jakub Kicinski wrote:
> On Mon,  5 Aug 2024 13:56:44 +0300 Boris Sukholitko wrote:
> > More about the patch series:
> > 
> > * patches 1-3 refactor skb_vlan_push to make skb_vlan_flush helper
> > * patch 4 open codes skb_vlan_push in act_vlan.c
> > * patch 5 contains the actual fix
> 
> The series is structured quite nicely for review, so kudos for that.
> But I'm not seeing the motivation for changing how TC pushes VLANs
> and not changing OvS (or BPF?), IOW the other callers of
> skb_vlan_push().
> 
> Why would pushing a tag from TC actions behave differently?

IMHO, the difference between TC and OvS and BPF is that in the TC case
the dissector is invoked on the wrong position in the packet (IP vs L2
header). We can regard reading garbage from there as a bug.

I am not sure that this is the case in OvS or BPF. E.g. in the BPF
case there may some script expecting the skb to point to an IP header
after second vlan push. My change will break it.

> 
> Please also add your test case to
> tools/testing/selftests/net/forwarding/tc_actions.sh
> if you can.

Done in v2.

Thanks,
Boris.