[PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[Boris Sukholitko]

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

Currently vlan modification action checks existance 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.2


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

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

Re: [PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[Jakub Kicinski]

On Wed, 21 Apr 2021 11:44:04 +0300 Boris Sukholitko wrote:
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for 
> the use of the individual or entity to whom it is addressed and may contain 
> information that is confidential, legally privileged, protected by privacy 
> laws, or otherwise restricted from disclosure to anyone else. If you are 
> not the intended recipient or the person responsible for delivering the 
> e-mail to the intended recipient, you are hereby notified that any use, 
> copying, distributing, dissemination, forwarding, printing, or copying of 
> this e-mail is strictly prohibited. If you received this e-mail in error, 
> please return the e-mail to the sender, delete it from your computer, and 
> destroy any printed copy of it.

Could you please resend without this legal prayer?

Re: [PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[Davide Caratti]

hello Boris, thanks for this patch!

On Wed, 2021-04-21 at 11:44 +0300, Boris Sukholitko wrote:
> Currently vlan modification action checks existance 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 code looks OK to me; however maybe it's possible to do a small
improvement:

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

this boolean is true when the action is configured to mangle the VLAN
PCP (i.e., set it to the value stored in 'tcfv_push_prio'), and false
otherwise.
Now, when the action configuration is dumped from userspace, as follows:

# tc action show action vlan

act_vlan does the following:

302         if ((p->tcfv_action == TCA_VLAN_ACT_PUSH ||
303              p->tcfv_action == TCA_VLAN_ACT_MODIFY) &&
304             (nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, p->tcfv_push_vid) ||
305              nla_put_be16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL,
306                           p->tcfv_push_proto) ||
307              (nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY,
308                                               p->tcfv_push_prio))))
309                 goto nla_put_failure;


so, userspace can't understand if this action is willing to mangle the
PCP or not, because in tcf_vlan_dump() the TCA_VLAN_PUSH_VLAN_PRIORITY
attriubute is dumped regardless of the value of 'tcfv_push_prio_exists'.

What about avoiding

 nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY, p->tcfv_push_prio)

when 'tcfv_push_prio_exists' is false?

this would give us the advantage of making this feature (i.e., the
possibility to set the vlan priority to 0) testable using tdc
'vlan.json' (that probably needs a rescan after this patch, and a
dedicated testcase for the case where the PCP is not mangled to zero).

any feedback appreciated. Thanks!

-- 
davide

Re: [PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[Boris Sukholitko]

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

Hi Jakub,

On Wed, Apr 21, 2021 at 12:12:41PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Apr 2021 11:44:04 +0300 Boris Sukholitko wrote:
> > This electronic communication and the information and any files transmitted 
> > with it, or attached to it, are confidential and are intended solely for 
> > the use of the individual or entity to whom it is addressed and may contain 
> > information that is confidential, legally privileged, protected by privacy 
> > laws, or otherwise restricted from disclosure to anyone else. If you are 
> > not the intended recipient or the person responsible for delivering the 
> > e-mail to the intended recipient, you are hereby notified that any use, 
> > copying, distributing, dissemination, forwarding, printing, or copying of 
> > this e-mail is strictly prohibited. If you received this e-mail in error, 
> > please return the e-mail to the sender, delete it from your computer, and 
> > destroy any printed copy of it.
> 
> Could you please resend without this legal prayer?

Please accept my apologies. Apparently the "legal prayer" is added
automatically to all outgoing Broadcom emails and I have no control over its
existance.

Is there any way to regard it as a non-netiquette compliant, but
nevertheless a personal signature? AFAICT, it does not affect the patch
in question.

Thanks,
Boris.

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

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

Re: [PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[Jakub Kicinski]

On Sun, 25 Apr 2021 13:57:13 +0300 Boris Sukholitko wrote:
> On Wed, Apr 21, 2021 at 12:12:41PM -0700, Jakub Kicinski wrote:
> > On Wed, 21 Apr 2021 11:44:04 +0300 Boris Sukholitko wrote:  
> > > This electronic communication and the information and any files transmitted 
> > > with it, or attached to it, are confidential and are intended solely for 
> > > the use of the individual or entity to whom it is addressed and may contain 
> > > information that is confidential, legally privileged, protected by privacy 
> > > laws, or otherwise restricted from disclosure to anyone else. If you are 
> > > not the intended recipient or the person responsible for delivering the 
> > > e-mail to the intended recipient, you are hereby notified that any use, 
> > > copying, distributing, dissemination, forwarding, printing, or copying of 
> > > this e-mail is strictly prohibited. If you received this e-mail in error, 
> > > please return the e-mail to the sender, delete it from your computer, and 
> > > destroy any printed copy of it.  
> > 
> > Could you please resend without this legal prayer?  
> 
> Please accept my apologies. Apparently the "legal prayer" is added
> automatically to all outgoing Broadcom emails and I have no control over its
> existance.

Perhaps try resending from a personal email account? There are folks at
broadcom who communicate using the corporate address, perhaps they could
help you?

> Is there any way to regard it as a non-netiquette compliant, but
> nevertheless a personal signature? AFAICT, it does not affect the patch
> in question.

If the signature had no implications in legal sense the lawyers
wouldn't ask for it to be included. Code submitted to for inclusion
needs to be licensed under a FOSS license.

Re: [PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority

[Florian Fainelli]

On 4/25/21 3:57 AM, Boris Sukholitko wrote:
> Hi Jakub,
> 
> On Wed, Apr 21, 2021 at 12:12:41PM -0700, Jakub Kicinski wrote:
>> On Wed, 21 Apr 2021 11:44:04 +0300 Boris Sukholitko wrote:
>>> This electronic communication and the information and any files transmitted 
>>> with it, or attached to it, are confidential and are intended solely for 
>>> the use of the individual or entity to whom it is addressed and may contain 
>>> information that is confidential, legally privileged, protected by privacy 
>>> laws, or otherwise restricted from disclosure to anyone else. If you are 
>>> not the intended recipient or the person responsible for delivering the 
>>> e-mail to the intended recipient, you are hereby notified that any use, 
>>> copying, distributing, dissemination, forwarding, printing, or copying of 
>>> this e-mail is strictly prohibited. If you received this e-mail in error, 
>>> please return the e-mail to the sender, delete it from your computer, and 
>>> destroy any printed copy of it.
>>
>> Could you please resend without this legal prayer?
> 
> Please accept my apologies. Apparently the "legal prayer" is added
> automatically to all outgoing Broadcom emails and I have no control over its
> existance.

You can request an exemption with Broadcom's IT department in order to
send emails without this legalese footer, I will email you separately on
how to do that.
-- 
Florian