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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4b6e1c2e7dcece6ae45e9822d5e96f0f059691cc.camel@redhat.com>
Date:   Fri, 23 Apr 2021 10:11:11 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Boris Sukholitko <boris.sukholitko@...adcom.com>,
        netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>
Cc:     Ilya Lifshits <ilya.lifshits@...adcom.com>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>
Subject: Re: [PATCH net-next] net/sched: act_vlan: Fix vlan modify to allow
 zero priority

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