Re: [PATCH net v3 0/3] net/sched: fix actions reading the network header in case of QinQ packets

[Davide Caratti <dcaratti@redhat.com>]

On Fri, 2019-05-31 at 11:42 -0700, Cong Wang wrote:
> On Fri, May 31, 2019 at 10:26 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > 'act_csum' was recently fixed to mangle the IPv4/IPv6 header if a packet
> > having one or more VLAN headers was processed: patch #1 ensures that all
> > VLAN headers are in the linear area of the skb.
> > Other actions might read or mangle the IPv4/IPv6 header: patch #2 and #3
> > fix 'act_pedit' and 'act_skbedit' respectively.
> 
> Maybe, just maybe, vlan tags are supposed to be handled by act_vlan?
> Which means maybe users have to pipe act_vlan to these actions.

but it's not possible with the current act_vlan code.
Each 'vlan' action pushes or pops a single tag, so:

1) we don't know how many vlan tags there are in each packet, so I should
put an (enough) high number of "pop" operations to ensure that a 'pedit'
rule correctly mangles the TTL in a IPv4 packet having 1 or more 802.1Q
tags in the L2 header.

2) after a vlan is popped with act_vlan, the kernel forgets about the VLAN
ID and the VLAN type. So, if I want to just mangle the TTL in a QinQ
packet, I need to reinject it in a place where both tags (including VLAN
type *and* VLAN id) are restored in the packet.

Clearly, act_vlan can't be used as is, because 'push' has hardcoded VLAN
ID and ethertype. Unless we change act_vlan code to enable rollback of
previous 'pop' operations, it's quite hard to pipe the correct sequence of
vlan 'pop' and 'push'.

> From the code reuse perspective, you are adding TCA_VLAN_ACT_POP
> to each of them.

No, these patches don't pop VLAN tags. All tags are restored after the
action completed his work, before returning a->tcfa_action.

May I ask you to read it as a followup of commit 2ecba2d1e45b ("net:
sched: act_csum: Fix csum calc for tagged packets"), where the 'csum'
action was modified to mangle the checksum of IPv4 headers even when
multiple 802.1Q tags were present?
With this series it becomes possible to mangle also the TTL field (with
pedit), and assign the diffserv bits to skb->priority (with skbedit).

> Thanks.

Thanks for reviewing, I look forward to see more comments from you.