Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Boris Sukholitko <boris.sukholitko@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

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
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]