netdev - Re: [PATCH net-next v6 3/3] act_vlan: VLAN action rewrite to use RCU lock/unlock and update

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]

Date:   Tue, 7 Nov 2017 08:54:20 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Pieter Jansen van Vuuren <pieter.jansenvanvuuren@...ronome.com>
Cc:     Manish Kurup <kurup.manish@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Simon Horman <simon.horman@...ronome.com>,
        john.hurley@...ronome.com, oss-drivers@...ronome.com,
        Netdev <netdev@...r.kernel.org>, aring@...atatu.com,
        Roman Mashak <mrv@...atatu.com>,
        Manish Kurup <manish.kurup@...izon.com>
Subject: Re: [PATCH net-next v6 3/3] act_vlan: VLAN action rewrite to use RCU
 lock/unlock and update

On Mon, Nov 6, 2017 at 10:37 AM, Pieter Jansen van Vuuren
<pieter.jansenvanvuuren@...ronome.com> wrote:
> On Fri,  3 Nov 2017 11:50:47 -0400
> Manish Kurup <kurup.manish@...il.com> wrote:
>
>> Using a spinlock in the VLAN action causes performance issues when the VLAN
>> action is used on multiple cores. Rewrote the VLAN action to use RCU read
>> locking for reads and updates instead.
>>
>> Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
>> Acked-by: Jiri Pirko <jiri@...lanox.com>
>> Signed-off-by: Manish Kurup <manish.kurup@...izon.com>
>> ---
>>  include/net/tc_act/tc_vlan.h | 46 +++++++++++++++++++++------
>>  net/sched/act_vlan.c         | 75
>> ++++++++++++++++++++++++++++++-------------- 2 files changed, 88
>> insertions(+), 33 deletions(-)
> ...
>>
>> +static void tcf_vlan_cleanup(struct tc_action *a, int bind)
>> +{
>> +     struct tcf_vlan *v = to_vlan(a);
>> +     struct tcf_vlan_params *p;
>> +
>> +     p = rcu_dereference_protected(v->vlan_p, 1);
>> +     kfree_rcu(p, rcu);
>> +}
>> +
>>  static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
>>                        int bind, int ref)
>>  {
>>       unsigned char *b = skb_tail_pointer(skb);
>>       struct tcf_vlan *v = to_vlan(a);
>> +     struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);
> nack. This fails reverse xmas-tree.

Are we really going to be so strict about the reverse xmas-tree that
we won't allow for assignment w/ variable declaration because the
dependency order won't fit into that format?

Last I knew this kind of setup was an exception to the reverse
xmas-tree layout requirement because in this case 'p' relies on 'v' so
we can't reorder these without having to kick the assignment of 'p'
off onto a line by itself.

- Alex