Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0
From: Davide Caratti <hidden>
Date: 2021-05-25 20:36:00
On Tue, May 25, 2021 at 06:35:59PM +0300, Boris Sukholitko wrote:
Currently vlan modification action checks existence of vlan priority by
comparing it to 0. Therefore it is impossible to modify existing vlan
tag to have priority 0.
For example, the following tc command will change the vlan id but will
not affect vlan priority:
tc filter add dev eth1 ingress matchall action vlan modify id 300 \
priority 0 pipe mirred egress redirect dev eth2hello Boris, thanks a lot for following up! A small nit below:
quoted hunk ↗ jump to hunk
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c index 1cac3c6fbb49..cca10b5e99c9 100644 --- a/net/sched/act_vlan.c +++ b/net/sched/act_vlan.c
[...]
quoted hunk ↗ jump to hunk
@@ -121,6 +121,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla, struct tc_action_net *tn = net_generic(net, vlan_net_id); struct nlattr *tb[TCA_VLAN_MAX + 1]; struct tcf_chain *goto_ch = NULL; + bool push_prio_exists = false; struct tcf_vlan_params *p; struct tc_vlan *parm; struct tcf_vlan *v;@@ -189,7 +190,8 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla, push_proto = htons(ETH_P_8021Q); } - if (tb[TCA_VLAN_PUSH_VLAN_PRIORITY]) + push_prio_exists = !!tb[TCA_VLAN_PUSH_VLAN_PRIORITY];
when the VLAN tag is pushed, not modified, the value of 'push_prio' is used in the traffic path regardless of the true/false value of 'push_prio_exists'. See below: 50 case TCA_VLAN_ACT_PUSH: 51 err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid | 52 (p->tcfv_push_prio << VLAN_PRIO_SHIFT)); So, I think that 'p->push_prio_exists' should be identically set to true when 'v_action' is TCA_VLAN_ACT_PUSH. That would allow a better display of rules once patch 2 of your series is applied: otherwise, 2 rules configuring the same TCA_VLAN_ACT_PUSH rule would be displayed differently, depending on whether userspace provided or not the TCA_VLAN_PUSH_VLAN_PRIORITY attribute set to 0. In other words, the last hunk should be something like:
@@ -241,6 +243,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla, p->tcfv_action = action; p->tcfv_push_vid = push_vid; p->tcfv_push_prio = push_prio; + p->tcfv_push_prio_exists = push_prio_exists || action == TCA_VLAN_ACT_PUSH;
WDYT? thanks! -- davide