[PATCH net-next v2 0/3] net/sched: act_vlan: Fix modify to allow 0

[PATCH net-next v2 0/3] net/sched: act_vlan: Fix modify to allow 0

[Boris Sukholitko]

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

Currently vlan modification action checks existence of vlan priority by
comparing it to 0. Therefore it is impossible to modify existing vlan
tag to have priority 0.

For example, the following tc command will change the vlan id but will
not affect vlan priority:

tc filter add dev eth1 ingress matchall action vlan modify id 300 \
        priority 0 pipe mirred egress redirect dev eth2

The incoming packet on eth1:

ethertype 802.1Q (0x8100), vlan 200, p 4, ethertype IPv4

will be changed to:

ethertype 802.1Q (0x8100), vlan 300, p 4, ethertype IPv4

although the user has intended to have p == 0.

The fix is to add tcfv_push_prio_exists flag to struct tcf_vlan_params
and rely on it when deciding to set the priority.

The same flag is used to avoid dumping unset vlan priority.

Change Log:
v1 -> v2:
- Do not dump unset priority and fix tests accordingly
- Test for priority 0 modification

Boris Sukholitko (3):
  net/sched: act_vlan: Fix modify to allow 0
  net/sched: act_vlan: No dump for unset priority
  net/sched: act_vlan: Test priority 0 modification

 include/net/tc_act/tc_vlan.h                  |  1 +
 net/sched/act_vlan.c                          | 11 +++--
 .../tc-testing/tc-tests/actions/vlan.json     | 42 +++++++++++++++----
 3 files changed, 41 insertions(+), 13 deletions(-)

-- 
2.29.3


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

[PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Boris Sukholitko]

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

Currently vlan modification action checks existence of vlan priority by
comparing it to 0. Therefore it is impossible to modify existing vlan
tag to have priority 0.

For example, the following tc command will change the vlan id but will
not affect vlan priority:

tc filter add dev eth1 ingress matchall action vlan modify id 300 \
        priority 0 pipe mirred egress redirect dev eth2

The incoming packet on eth1:

ethertype 802.1Q (0x8100), vlan 200, p 4, ethertype IPv4

will be changed to:

ethertype 802.1Q (0x8100), vlan 300, p 4, ethertype IPv4

although the user has intended to have p == 0.

The fix is to add tcfv_push_prio_exists flag to struct tcf_vlan_params
and rely on it when deciding to set the priority.

Fixes: 45a497f2d149a4a8061c (net/sched: act_vlan: Introduce TCA_VLAN_ACT_MODIFY vlan action)
Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>
---
 include/net/tc_act/tc_vlan.h | 1 +
 net/sched/act_vlan.c         | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
index f051046ba034..f94b8bc26f9e 100644
--- a/include/net/tc_act/tc_vlan.h
+++ b/include/net/tc_act/tc_vlan.h
@@ -16,6 +16,7 @@ struct tcf_vlan_params {
 	u16               tcfv_push_vid;
 	__be16            tcfv_push_proto;
 	u8                tcfv_push_prio;
+	bool              tcfv_push_prio_exists;
 	struct rcu_head   rcu;
 };
 
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 1cac3c6fbb49..cca10b5e99c9 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -70,7 +70,7 @@ static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a,
 		/* replace the vid */
 		tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
 		/* replace prio bits, if tcfv_push_prio specified */
-		if (p->tcfv_push_prio) {
+		if (p->tcfv_push_prio_exists) {
 			tci &= ~VLAN_PRIO_MASK;
 			tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
 		}
@@ -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];
+		if (push_prio_exists)
 			push_prio = nla_get_u8(tb[TCA_VLAN_PUSH_VLAN_PRIORITY]);
 		break;
 	case TCA_VLAN_ACT_POP_ETH:
@@ -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;
 	p->tcfv_push_proto = push_proto;
 
 	if (action == TCA_VLAN_ACT_PUSH_ETH) {
-- 
2.29.3


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

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

[Boris Sukholitko]

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

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                           |  4 ++--
 .../tc-testing/tc-tests/actions/vlan.json      | 18 +++++++++---------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index cca10b5e99c9..6765096b99b3 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) {
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 41d783254b08..4675f1c04d17 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
@@ -297,7 +297,7 @@
         "cmdUnderTest": "$TC actions add action vlan push id 123 index 18",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 18",
-        "matchPattern": "action order [0-9]+: vlan.*push id 123 protocol 802.1Q priority 0 pipe.*index 18 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 123 protocol 802.1Q pipe.*index 18 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"
@@ -345,7 +345,7 @@
         "cmdUnderTest": "$TC actions add action vlan push id 1024 protocol 802.1AD pass index 10000",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 10000",
-        "matchPattern": "action order [0-9]+: vlan.*push id 1024 protocol 802.1ad priority 0 pass.*index 10000 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 1024 protocol 802.1ad pass.*index 10000 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"
@@ -369,7 +369,7 @@
         "cmdUnderTest": "$TC actions add action vlan push id 4094 index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 1",
-        "matchPattern": "action order [0-9]+: vlan.*push id 4094.*protocol 802.1Q.*priority 0.*index 1 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 4094.*protocol 802.1Q.*index 1 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"
@@ -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"
@@ -512,7 +512,7 @@
         "cmdUnderTest": "$TC actions replace action vlan push id 700 pipe index 12",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 12",
-        "matchPattern": "action order [0-9]+: vlan.*push id 700 protocol 802.1Q priority 0 pipe.*index 12 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 700 protocol 802.1Q pipe.*index 12 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"
@@ -537,7 +537,7 @@
         "cmdUnderTest": "$TC actions replace action vlan push id 1 protocol 802.1ad pipe index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action vlan index 1",
-        "matchPattern": "action order [0-9]+: vlan.*push id 1 protocol 802.1ad priority 0 pipe.*index 1 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 1 protocol 802.1ad pipe.*index 1 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"
@@ -635,7 +635,7 @@
         "cmdUnderTest": "$TC actions del action vlan index 999",
         "expExitCode": "0",
         "verifyCmd": "$TC actions list action vlan",
-        "matchPattern": "action order [0-9]+: vlan.*push id 4094 protocol 802.1Q priority 0 pipe.*index 999 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 4094 protocol 802.1Q pipe.*index 999 ref",
         "matchCount": "0",
         "teardown": []
     },
@@ -708,7 +708,7 @@
         "cmdUnderTest": "$TC actions replace action vlan push id 500 goto chain 42 index 90 cookie c1a0c1a0",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action vlan index 90",
-        "matchPattern": "action order [0-9]+: vlan.*push id 500 protocol 802.1Q priority 0 pass.*index 90 ref",
+        "matchPattern": "action order [0-9]+: vlan.*push id 500 protocol 802.1Q pass.*index 90 ref",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action vlan"
-- 
2.29.3


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

[PATCH net-next v2 3/3] net/sched: act_vlan: Test priority 0 modification

[Boris Sukholitko]

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

Because explicitly being set, the priority 0 should appear
in the output.

Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>
---
 .../tc-testing/tc-tests/actions/vlan.json     | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

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 4675f1c04d17..10310c92ebe7 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
@@ -445,6 +445,30 @@
         "matchCount": "0",
         "teardown": []
     },
+    {
+        "id": "ba5b",
+        "name": "Add vlan modify action for protocol 802.1Q setting priority 0",
+        "category": [
+            "actions",
+            "vlan"
+        ],
+        "setup": [
+            [
+                "$TC actions flush action vlan",
+                0,
+                1,
+                255
+            ]
+        ],
+        "cmdUnderTest": "$TC actions add action vlan modify protocol 802.1Q id 5 priority 0 index 100",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions get action vlan index 100",
+        "matchPattern": "action order [0-9]+: vlan.*modify id 100 priority 0 protocol 802.1Q pipe.*index 100 ref",
+        "matchCount": "0",
+        "teardown": [
+            "$TC actions flush action vlan"
+        ]
+    },
     {
         "id": "6812",
         "name": "Add vlan modify action for protocol 802.1Q",
-- 
2.29.3


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

Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Davide Caratti]

On Tue, May 25, 2021 at 06:35:59PM +0300, Boris Sukholitko wrote:
> Currently vlan modification action checks existence of vlan priority by
> comparing it to 0. Therefore it is impossible to modify existing vlan
> tag to have priority 0.
> 
> For example, the following tc command will change the vlan id but will
> not affect vlan priority:
> 
> tc filter add dev eth1 ingress matchall action vlan modify id 300 \
>         priority 0 pipe mirred egress redirect dev eth2

hello Boris, thanks a lot for following up! A small nit below:
 
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 1cac3c6fbb49..cca10b5e99c9 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c

[...]

> @@ -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));

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. 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

Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Boris Sukholitko]

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

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
> 

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

Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Davide Caratti]

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

Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Boris Sukholitko]

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

On Thu, May 27, 2021 at 07:00:52PM +0200, Davide Caratti wrote:
[...]
> 
> 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).

I am convinced. I've done this in v3.

> 
> 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?

Also done in v3.

Thanks,
Boris.

> 
> thanks!
> -- 
> davide
> 
> 
> 

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