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 16:07:14 +0300

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.