netdev - Re: [PATCH net-next v2 1/3] net/sched: act

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210526114553.GA31019@builder>
Date:   Wed, 26 May 2021 14:45:53 +0300
From:   Boris Sukholitko <boris.sukholitko@...adcom.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Ilya Lifshits <ilya.lifshits@...adcom.com>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow
 0

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
> 

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)