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: <PH0PR11MB5880B73436ADB3B8216C045DDA409@PH0PR11MB5880.namprd11.prod.outlook.com>
Date:   Sat, 25 Dec 2021 08:39:49 +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.
>
>But there's no locking here, it's disabling interrupts. Yes, a lock is taken inside kasan_record_aux_stack_noalloc(), but that's not one you can do much about.

>I don't mind this patch, but I think there might be some confusion. A better explanation (in commit message or otherwise) would help make sure we're not talking about different things.

Hi Marco, Are the following modifications clear to you?

Subject: [PATCH] rcu: Reduce the consumption time of
 local_irq_save()/restore() critical area

In non-production KASAN kernel, a large number of call stacks are recorded,
it takes some time to acquire the global spinlock(depot_lock) inside
kasan_record_aux_stack_noalloc(), increased interrupts disable time,
kasan_record_aux_stack_noalloc() doesn't care if interrupts are enabled or
not when called, so move it outside the critical area.

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 9b58bae0527a..36bd3f9e57b3 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3068,8 +3068,8 @@ void 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);

Thanks,
Zqiang