netdev - Re: [PATCH net-next v6 01/11] net: sched: use rcu for action cookie 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:   Thu, 12 Jul 2018 20:52:12 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Yevgeny Kliteynik <kliteyn@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next v6 01/11] net: sched: use rcu for action cookie update

On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vladbu@...lanox.com> wrote:
>
> Implement functions to atomically update and free action cookie
> using rcu mechanism.

Without stating any reason..... Is this even a changelog?

>
> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>

Dear Marcelo, how did it pass your review? See below:

> +static void tcf_set_action_cookie(struct tc_cookie __rcu **old_cookie,
> +                                 struct tc_cookie *new_cookie)
> +{
> +       struct tc_cookie *old;
> +
> +       old = xchg(old_cookie, new_cookie);

This is an incorrect use of RCU, obviously should be rcu_assign_pointer()
here.

> @@ -65,10 +83,7 @@ static void free_tcf(struct tc_action *p)
>         free_percpu(p->cpu_bstats);
>         free_percpu(p->cpu_qstats);
>
> -       if (p->act_cookie) {
> -               kfree(p->act_cookie->data);
> -               kfree(p->act_cookie);
> -       }
> +       tcf_set_action_cookie(&p->act_cookie, NULL);

So, this is called in free_tcf(), where the action is already
invisible from readers so it is ready to be freed.

The question is:

If the action itself is already ready to be freed, why do you
need RCU here? What could still read 'act->act_cookie'
while 'act' is already invisible?

Its last refcnt is already gone, the fast path RCU readers
are gone too given filters use rcu work already.

Standalone action dump? Again, the last refcnt is already
gone.

Marcelo, Vlad, Jiri, please explain.

Thanks!