linux-kernel - Re: [PATCH] mm/slub: fix a deadlock in shuffle

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:   Wed, 25 Sep 2019 11:31:53 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Qian Cai <cai@....pw>
Cc:     akpm@...ux-foundation.org, bigeasy@...utronix.de,
        tglx@...utronix.de, thgarnie@...gle.com, tytso@....edu,
        cl@...ux.com, penberg@...nel.org, rientjes@...gle.com,
        mingo@...hat.com, will@...nel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, keescook@...omium.org
Subject: Re: [PATCH] mm/slub: fix a deadlock in shuffle_freelist()

On Fri, Sep 13, 2019 at 12:27:44PM -0400, Qian Cai wrote:
> The commit b7d5dc21072c ("random: add a spinlock_t to struct
> batched_entropy") insists on acquiring "batched_entropy_u32.lock" in
> get_random_u32() which introduced the lock chain,
> 
> "&rq->lock --> batched_entropy_u32.lock"
> 
> even after crng init. As the result, it could result in deadlock below.
> Fix it by using get_random_bytes() in shuffle_freelist() which does not
> need to take on the batched_entropy locks.
> 
> WARNING: possible circular locking dependency detected
> 5.3.0-rc7-mm1+ #3 Tainted: G             L
> ------------------------------------------------------
> make/7937 is trying to acquire lock:
> ffff900012f225f8 (random_write_wait.lock){....}, at:
> __wake_up_common_lock+0xa8/0x11c
> 
> but task is already holding lock:
> ffff0096b9429c00 (batched_entropy_u32.lock){-.-.}, at:
> get_random_u32+0x6c/0x1dc
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #3 (batched_entropy_u32.lock){-.-.}:
>        lock_acquire+0x31c/0x360
>        _raw_spin_lock_irqsave+0x7c/0x9c
>        get_random_u32+0x6c/0x1dc
>        new_slab+0x234/0x6c0
>        ___slab_alloc+0x3c8/0x650
>        kmem_cache_alloc+0x4b0/0x590
>        __debug_object_init+0x778/0x8b4
>        debug_object_init+0x40/0x50
>        debug_init+0x30/0x29c
>        hrtimer_init+0x30/0x50
>        init_dl_task_timer+0x24/0x44
>        __sched_fork+0xc0/0x168
>        init_idle+0x78/0x26c
>        fork_idle+0x12c/0x178
>        idle_threads_init+0x108/0x178
>        smp_init+0x20/0x1bc
>        kernel_init_freeable+0x198/0x26c
>        kernel_init+0x18/0x334
>        ret_from_fork+0x10/0x18
> 
> -> #2 (&rq->lock){-.-.}:

This relation is silly..

I suspect the below 'works'...

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 63900ca029e0..ec1d72f18b34 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6027,10 +6027,11 @@ void init_idle(struct task_struct *idle, int cpu)
 	struct rq *rq = cpu_rq(cpu);
 	unsigned long flags;
 
+	__sched_fork(0, idle);
+
 	raw_spin_lock_irqsave(&idle->pi_lock, flags);
 	raw_spin_lock(&rq->lock);
 
-	__sched_fork(0, idle);
 	idle->state = TASK_RUNNING;
 	idle->se.exec_start = sched_clock();
 	idle->flags |= PF_IDLE;