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]

Date:   Fri, 24 Dec 2021 03:22:57 +0000
From:   "Zhang, Qiang1" <qiang1.zhang@...el.com>
To:     Marco Elver <elver@...gle.com>
CC:     "paulmck@...nel.org" <paulmck@...nel.org>,
        "ryabinin.a.a@...il.com" <ryabinin.a.a@...il.com>,
        "urezki@...il.com" <urezki@...il.com>,
        "Miao, Jun" <jun.miao@...el.com>,
        "linux-kernel@...r.kernel.org" <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 for reply, Yes, this only benefits non-production KASAN kernel.  In KASAN kernel, 
there may be a lot of call stack recorded, in addition to locking competition, the find_stack()  will
also take a long time.


Thanks,
Zqiang

>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
>