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

From: Boris Sukholitko <[email-masked]>
Date: Wed, 14 Aug 2024 17:55:33 +0300

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.