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] [day] [month] [year] [list]

Message-ID: <ZrzFZXZq7UOz6dd3@ramen>
Date: Wed, 14 Aug 2024 17:55:33 +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 Wed, Aug 14, 2024 at 07:39:50AM -0700, Jakub Kicinski wrote:
> On Wed, 14 Aug 2024 16:07:14 +0300 Boris Sukholitko wrote:
> > > 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.
> 
> The packet either has correct format or it doesn't. You could easily
> construct a TC ruleset which pushes the VLAN using act_bpf, instead of
> act_vlan.
> 
> Let's not be too conservative, worrying about very unlikely
> regressions, IMHO. Such divergence makes the code base much harder 
> to maintainer.
> 

I agree. I'll prepare v3 version with the changes folded into skb_vlan_push,
updating the callers.

> > > Please also add your test case to
> > > tools/testing/selftests/net/forwarding/tc_actions.sh
> > > if you can.  
> > 
> > Done in v2.
> 
> Please do not respond to a discussion and immediate send the next
> version.

OK.

Thanks,
Boris.