linux-kernel - Re: [PATCH] dump_stack: Avoid the livelock of the dump

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-next>] [day] [month] [year] [list]

Date:   Tue, 29 Oct 2019 22:09:21 +0800
From:   Kevin Hao <haokexin@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] dump_stack: Avoid the livelock of the dump_lock

On Tue, Oct 29, 2019 at 01:46:40PM +0100, Linus Torvalds wrote:
> Sorry for html crud, I'm at a conference without my laptop, so reading email on
> my phone..
> 
> On Tue, Oct 29, 2019, 10:31 Kevin Hao <haokexin@...il.com> wrote:
> 
>     In the current code, we uses the atomic_cmpxchg() to serialize the
>     output of the dump_stack(), but this implementation suffers the
>     thundering herd problem. Actually we can use a spinlock here and leverage
>     the
> 
>     implementation of the spinlock(either ticket or queued spinlock) to
>     mediate such kind of livelock. Since the dump_stack() runs with the
>     irq disabled, so use the raw_spinlock_t to make it safe for rt kernel.
> 
> 
> The problem with this is that I think it will deadlock easily with NMI's, won't
> it?
> 
> There is a window between getting the spin lock and setting 'dump_cpu' where an
> incoming NMI would then deadlock because it also tries to get the lock, because
> it hasn't seen the fact that this cpu already got it.

Indeed, sorry I missed that.

> 
> The cmpxchg is supposed to protect against that, and make the locking be atomic
> with the CPU setting.
> 
> Can you try instead to make the retry loop not jump right back to the cmpxchg,
> but start looping just reading the CPU value, and only jumping back when it
> becomes -1?

Do you mean something like this?
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 5cff72f18c4a..a0f1293a3638 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -107,6 +107,7 @@ asmlinkage __visible void dump_stack(void)
 	} else {
 		local_irq_restore(flags);
 		cpu_relax();
+		while (atomic_read(&dump_lock) != -1);
 		goto retry;
 	}
 
It does help. I will send a v2 if it pass more stress test.

Thanks,
Kevin

> 
>       Linus

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)