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: <20240814073950.53c6d4d7@kernel.org>
Date: Wed, 14 Aug 2024 07:39:50 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Boris Sukholitko <boris.sukholitko@...adcom.com>
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, 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.

> > 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.