linux-kernel - Re: [PATCH] rcu: record kasan stack before enter local_irq

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: <CANpmjNOmaSxqTjUd8y=A+sK0jGg6c00t8uvmOey+QrJsDamsmQ@mail.gmail.com>
Date:   Thu, 23 Dec 2021 16:08:31 +0100
From:   Marco Elver <elver@...gle.com>
To:     Zqiang <qiang1.zhang@...el.com>
Cc:     paulmck@...nel.org, ryabinin.a.a@...il.com, urezki@...il.com,
        jun.miao@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rcu: record kasan stack before enter local_irq_save()/restore()
 critical area

On Wed, 22 Dec 2021 at 08:54, Zqiang <qiang1.zhang@...el.com> wrote:
> The kasan_record_aux_stack_noalloc() only record stack, it doesn't need
> to be called in local_irq_save()/restore() critical area, and the global
> spinlock (depot_lock) will be acquired in this function, When enable
> kasan stack, locking contention may increase the time in the critical area.

I think the change itself is harmless, because
kasan_record_aux_stack_noalloc() doesn't care if interrupts are
enabled or not when called, but the justification isn't clear to me.

What "locking contention" are you speaking about? You're moving a
local_irq_save() which disables interrupts. Yes, it might be nice to
reduce the time interrupts are disabled, but in this case the benefit
(if any) isn't clear at all, also because this only benefits
non-production KASAN kernels.

Can you provide better justification? Did you encounter a specific
problem, maybe together with data?

Thanks,
-- Marco

> Signed-off-by: Zqiang <qiang1.zhang@...el.com>
> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 347dae1876a6..5198e44cb124 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3030,8 +3030,8 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func)
>         }
>         head->func = func;
>         head->next = NULL;
> -       local_irq_save(flags);
>         kasan_record_aux_stack_noalloc(head);
> +       local_irq_save(flags);
>         rdp = this_cpu_ptr(&rcu_data);
>
>         /* Add the callback to our list. */
> --
> 2.25.1
>