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: <YK/QRFAcMMcXBvw9@dcaratti.users.ipa.redhat.com>
Date:   Thu, 27 May 2021 19:00:52 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Boris Sukholitko <boris.sukholitko@...adcom.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

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