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]

Message-ID: <1568836797.5576.182.camel@lca.pw>
Date:   Wed, 18 Sep 2019 15:59:57 -0400
From:   Qian Cai <cai@....pw>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     peterz@...radead.org, mingo@...hat.com, akpm@...ux-foundation.org,
        tglx@...utronix.de, thgarnie@...gle.com, tytso@....edu,
        cl@...ux.com, penberg@...nel.org, rientjes@...gle.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 Tue, 2019-09-17 at 09:16 +0200, Sebastian Andrzej Siewior wrote:
> On 2019-09-16 17:31:34 [-0400], Qian Cai wrote:
> …
> > get_random_u64() is also busted.
> 
> …
> > [  753.486588]  Possible unsafe locking scenario:
> > 
> > [  753.493890]        CPU0                    CPU1
> > [  753.499108]        ----                    ----
> > [  753.504324]   lock(batched_entropy_u64.lock);
> > [  753.509372]                                lock(&(&zone->lock)->rlock);
> > [  753.516675]                                lock(batched_entropy_u64.lock);
> > [  753.524238]   lock(random_write_wait.lock);
> > [  753.529113] 
> >                 *** DEADLOCK ***
> 
> This is the same scenario as the previous one in regard to the
> batched_entropy_….lock.

The commit 383776fa7527 ("locking/lockdep: Handle statically initialized percpu
locks properly") which increased the chance of false positives for percpu locks
significantly especially for large systems like in those examples since it makes
all of them the same class. Once there happens a false positive, lockdep will
become useless.

In reality, each percpu lock is a different lock as we have seen in those
examples where each CPU only take a local one. The only thing that should worry
about is the path that another CPU could take a non-local percpu lock. For
example, invalidate_batched_entropy() which is a for_each_possible_cpu() call.
Is there any other place that another CPU could take a non-local percpu lock but
not a for_each_possible_cpu() or similar iterator?

Even before the above commit, if the system is running long enough, it could
still catch a deadlock from those percpu lock iterators since it will register
each percpu lock usage in lockdep

Overall, it sounds to me the side-effects of commit 383776fa7527 outweight the
benefits, and should be reverted. Do I miss anything?