netdev - Re: [PATCH net-next v3 2/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] [day] [month] [year] [list]

Message-ID: <YLYsmMw9x2kXLIpk@dcaratti.users.ipa.redhat.com>
Date:   Tue, 1 Jun 2021 14:48:24 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Boris Sukholitko <boris.sukholitko@...adcom.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        linux-kselftest@...r.kernel.org, shuah@...nel.org,
        Ilya Lifshits <ilya.lifshits@...adcom.com>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>
Subject: Re: [PATCH net-next v3 2/3] net/sched: act_vlan: No dump for unset
 priority

On Tue, Jun 01, 2021 at 03:35:10PM +0300, Boris Sukholitko wrote:
> Hi Jacub,
> 
> On Mon, May 31, 2021 at 10:21:36PM -0700, Jakub Kicinski wrote:
> > On Sun, 30 May 2021 14:40:51 +0300 Boris Sukholitko wrote:
> > > diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c

[...]

> > > @@ -362,10 +362,19 @@ static int tcf_vlan_search(struct net *net, struct tc_action **a, u32 index)
> > >  
> > >  static size_t tcf_vlan_get_fill_size(const struct tc_action *act)
> > >  {
> > > -	return nla_total_size(sizeof(struct tc_vlan))
> > > +	struct tcf_vlan *v = to_vlan(act);
> > > +	struct tcf_vlan_params *p;
> > > +	size_t ret = nla_total_size(sizeof(struct tc_vlan))
> > >  		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_ID */
> > > -		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
> > > -		+ nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
> > > +		+ nla_total_size(sizeof(u16)); /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
> > > +
> > > +	spin_lock_bh(&v->tcf_lock);
> > > +	p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
> > > +	if (p->tcfv_push_prio_exists)
> > > +		ret += nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
> > > +	spin_unlock_bh(&v->tcf_lock);
> > 
> > This jumps out a little bit - if we need to take this lock to inspect
> > tcf_vlan_params, then I infer its value may change. And if it may
> > change what guarantees it doesn't change between calculating the skb
> > length and dumping?
> > 
> > It's common practice to calculate the max skb len required when
> > attributes are this small.
> > 
> 
> I believe you are right.

ouch, that's my fault actually - it's true, TC rules can be
modified and dumped at the same time. Then the only thing we can
do is to account for TCA_VLAN_PUSH_VLAN_PRIORITY even if we will not
fill it.

thanks for spotting this,
-- 
davide