| Message ID | 20210525153601.6705-2-boris.sukholitko@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net/sched: act_vlan: Fix modify to allow 0 | expand |
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 eth2 hello Boris, thanks a lot for following up! A small nit below: > 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 [...] > @@ -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!
Hi Davide, On Tue, May 25, 2021 at 10:35:50PM +0200, Davide Caratti wrote: > On Tue, May 25, 2021 at 06:35:59PM +0300, Boris Sukholitko wrote: [snip] > > > @@ -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)); > Yes, the tcfv_push_prio is 0 here by default. > 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. Sorry for being obtuse, but I was under impression that we want to display priority if and only if it has been set by the userspace. Therefore the fact that the default vlan priority for the push is 0 has no relevance to such logic. Why do you think that the push should be made special regarding the display? Is there something I am missing here? Thanks, Boris. > 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 >
On Wed, May 26, 2021 at 02:45:53PM +0300, Boris Sukholitko wrote: > Hi Davide, > > On Tue, May 25, 2021 at 10:35:50PM +0200, Davide Caratti wrote: > > On Tue, May 25, 2021 at 06:35:59PM +0300, Boris Sukholitko wrote: hello Boris, [...] > > > @@ -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)); > > > > Yes, the tcfv_push_prio is 0 here by default. > > > 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. > > Sorry for being obtuse, but I was under impression that we want to > display priority if and only if it has been set by the userspace. don't get me wrong, I don't have strong opinions on this (I don't have strong opinions at all :) ). In my understanding, the patch was adding 'push_prio_exists' to allow using 0 in 'p->tcfv_push_prio' in the traffic path, instead of assuming that '0' implies no configuration by the user. My suggestion was just to simplify the end-user dump experience, so that the value of 'p->tcfv_push_prio' is dumped always in case of TCA_VLAN_ACT_PUSH. In this way, rules with equal "behavior" in the traffic path are always dumped in the same way. IOW, # tc action add action vlan push id 42 prio 0 index 1 and # tc action add action vlan push id 42 index 1 do exactly the same thing in the traffic path, so there is no need to dump them differently. On the contrary, these 2 rules: # tc action add action vlan modify id 42 prio 0 index 1 and # tc action add action vlan modify id 42 index 1 don't do the same thing, because packet hitting the first rule will have their priority identically set to 0, while the second one will leave the VLAN priority unmodified. So, I think it makes sense to have different dumps here (that was my comment to your v1). Another small nit - forgive me, I didn't spot it in the first review: not 100% sure, but I think that in tcf_vlan_get_fill_size() we need to avoid accounting for TCA_VLAN_PUSH_VLAN_PRIORITY in case the rule has 'push_prio_exists' equal to false. Otherwise we allocate an extra u8 netlink attribute in case of batch dump. Correct? thanks! -- davide
On Thu, May 27, 2021 at 07:00:52PM +0200, Davide Caratti wrote: [...] > > My suggestion was just to simplify the end-user dump experience, so > that the value of 'p->tcfv_push_prio' is dumped always in case of > TCA_VLAN_ACT_PUSH. In this way, rules with equal "behavior" in the > traffic path are always dumped in the same way. IOW, > > # tc action add action vlan push id 42 prio 0 index 1 > > and > > # tc action add action vlan push id 42 index 1 > > do exactly the same thing in the traffic path, so there is no need to > dump them differently. On the contrary, these 2 rules: > > # tc action add action vlan modify id 42 prio 0 index 1 > > and > > # tc action add action vlan modify id 42 index 1 > > don't do the same thing, because packet hitting the first rule will have > their priority identically set to 0, while the second one will leave the > VLAN priority unmodified. So, I think it makes sense to have different > dumps here (that was my comment to your v1). I am convinced. I've done this in v3. > > Another small nit - forgive me, I didn't spot it in the first review: > > not 100% sure, but I think that in tcf_vlan_get_fill_size() we need > to avoid accounting for TCA_VLAN_PUSH_VLAN_PRIORITY in case the rule > has 'push_prio_exists' equal to false. Otherwise we allocate an > extra u8 netlink attribute in case of batch dump. Correct? Also done in v3. Thanks, Boris. > > thanks! > -- > davide > > >
diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h index f051046ba034..f94b8bc26f9e 100644 --- a/include/net/tc_act/tc_vlan.h +++ b/include/net/tc_act/tc_vlan.h @@ -16,6 +16,7 @@ struct tcf_vlan_params { u16 tcfv_push_vid; __be16 tcfv_push_proto; u8 tcfv_push_prio; + bool tcfv_push_prio_exists; struct rcu_head rcu; }; 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 @@ -70,7 +70,7 @@ static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a, /* replace the vid */ tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid; /* replace prio bits, if tcfv_push_prio specified */ - if (p->tcfv_push_prio) { + if (p->tcfv_push_prio_exists) { tci &= ~VLAN_PRIO_MASK; tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT; } @@ -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]; + if (push_prio_exists) push_prio = nla_get_u8(tb[TCA_VLAN_PUSH_VLAN_PRIORITY]); break; case TCA_VLAN_ACT_POP_ETH: @@ -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; p->tcfv_push_proto = push_proto; if (action == TCA_VLAN_ACT_PUSH_ETH) {
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 eth2 The incoming packet on eth1: ethertype 802.1Q (0x8100), vlan 200, p 4, ethertype IPv4 will be changed to: ethertype 802.1Q (0x8100), vlan 300, p 4, ethertype IPv4 although the user has intended to have p == 0. The fix is to add tcfv_push_prio_exists flag to struct tcf_vlan_params and rely on it when deciding to set the priority. Fixes: 45a497f2d149a4a8061c (net/sched: act_vlan: Introduce TCA_VLAN_ACT_MODIFY vlan action) Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com> --- include/net/tc_act/tc_vlan.h | 1 + net/sched/act_vlan.c | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-)