diff mbox series

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

Message ID 20210530114052.16483-3-boris.sukholitko@broadcom.com (mailing list archive)
State New
Headers show
Series net/sched: act_vlan: Fix modify to allow 0 | expand

Commit Message

Boris Sukholitko May 30, 2021, 11:40 a.m. UTC 
Dump vlan priority only if it has been previously set.

Fix the tests accordingly.

Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>
---
 net/sched/act_vlan.c                          | 19 ++++++++++++++-----
 .../tc-testing/tc-tests/actions/vlan.json     |  4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

Comments

Jakub Kicinski June 1, 2021, 5:21 a.m. UTC | #1 
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.

> +	return ret;
>  }
Boris Sukholitko June 1, 2021, 12:35 p.m. UTC | #2 
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.
Davide Caratti June 1, 2021, 12:48 p.m. UTC | #3 
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,
diff mbox series

Patch

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);
+
+	return ret;
 }
 
 static struct tc_action_ops act_vlan_ops = {
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json b/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
index 527ce5410314..1d9d261aa0b3 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
@@ -463,7 +463,7 @@ 
         "cmdUnderTest": "$TC actions add action vlan modify protocol 802.1Q id 5 index 100",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 100",
-        "matchPattern": "action order [0-9]+: vlan.*modify id 100 protocol 802.1Q priority 0 pipe.*index 100 ref",
+        "matchPattern": "action order [0-9]+: vlan.*modify id 100 protocol 802.1Q pipe.*index 100 ref",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action vlan"
@@ -487,7 +487,7 @@ 
         "cmdUnderTest": "$TC actions add action vlan modify protocol 802.1ad id 500 reclassify index 12",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 12",
-        "matchPattern": "action order [0-9]+: vlan.*modify id 500 protocol 802.1ad priority 0 reclassify.*index 12 ref",
+        "matchPattern": "action order [0-9]+: vlan.*modify id 500 protocol 802.1ad reclassify.*index 12 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"