| Message ID | 20210421084404.GA7262@noodle (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/sched: act_vlan: Fix vlan modify to allow zero priority | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: davem@davemloft.net; 2 maintainers not CCed: davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: 'existance' may be misspelled - perhaps 'existence'? |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/header_inline | success | Link |
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?
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!
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.
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.
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.
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) {
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(-)