Re: [PATCH net-next v3 2/3] net/sched: act_vlan: No dump for unset priority

From: Boris Sukholitko <boris.sukholitko@broadcom.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org, Jamal Hadi Salim <jhs@mojatatu.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	linux-kselftest@vger.kernel.org, shuah@kernel.org,
	Ilya Lifshits <ilya.lifshits@broadcom.com>,
	Shmulik Ladkani <shmulik.ladkani@gmail.com>,
	Davide Caratti <dcaratti@redhat.com>
Subject: Re: [PATCH net-next v3 2/3] net/sched: act_vlan: No dump for unset priority
Date: Tue, 1 Jun 2021 15:35:10 +0300	[thread overview]
Message-ID: <20210601123510.GA3940@builder> (raw)
In-Reply-To: <20210531222136.26670598@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>

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

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
> > index a108469c664f..ccd1acfa4c55 100644
> > --- a/net/sched/act_vlan.c
> > +++ b/net/sched/act_vlan.c
> > @@ -307,8 +307,8 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
> >  	    (nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, p->tcfv_push_vid) ||
> >  	     nla_put_be16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL,
> >  			  p->tcfv_push_proto) ||
> > -	     (nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY,
> > -					      p->tcfv_push_prio))))
> > +	     (p->tcfv_push_prio_exists &&
> > +	      nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY, p->tcfv_push_prio))))
> >  		goto nla_put_failure;
> >  
> >  	if (p->tcfv_action == TCA_VLAN_ACT_PUSH_ETH) {
> > @@ -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.

I've just sent out v4 of the patch with tcf_vlan_get_fill_size change
reverted.

Thanks,
Boris.

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