linux-kernel - Re: [PATCH] lockdep: Do no validate wait context for novalidate class

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

Message-ID: <20200820140552.5xtk23a3lxvcfpvu@linutronix.de>
Date:   Thu, 20 Aug 2020 16:05:52 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     peterz@...radead.org
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Will Deacon <will@...nel.org>, tglx@...utronix.de,
        kent.overstreet@...il.com
Subject: Re: [PATCH] lockdep: Do no validate wait context for novalidate class

On 2020-08-20 14:38:59 [+0200], peterz@...radead.org wrote:
> On Thu, Aug 20, 2020 at 01:43:48PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2020-08-20 13:40:36 [+0200], peterz@...radead.org wrote:
> > > Anyway, all 3 users should have the same wait context, so where is the
> > > actual problem?
> > 
> > I have one in RT which is a per-CPU spinlock within local_bh_disable()
> > to act as a per-CPU BLK like mainline.
> 
> Then can we get to see that code and an explanation for what the problem
> is and why it is still correct?

An actual backtrace looks like this:
| WARNING: possible circular locking dependency detected
…
| Possible unsafe locking scenario:
|
|       CPU0                    CPU1
|       ----                    ----
|  lock(k-sk_lock-AF_NETLINK);
|                               lock((l).lock#2);
|                               lock(k-sk_lock-AF_NETLINK);
|  lock((l).lock#2);
|
|  *** DEADLOCK ***

The "k-sk_lock-AF_NETLINK" is global but "(l).lock#2" is per CPU. The
circular dependency can not occur because CPU0 and CPU1 can acquire the
lock simultaneously.
The softirq code is at
   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/patches/softirq-Add-preemptible-softirq.patch?h=linux-5.6.y-rt-patches&id=4ce1fda10dae882d494c6430cc438ff645a35603#n146

I'm not sure why sk_lock on CPU0 is before (l).lock. It doesn't change
even if the lock is acquired after trace_softirqs_off(). If the sk_lock
would be acquired with enabled BH then lockdep would complain.

The lovely in_atomic() check is due to irq_enter(), preempt_disable() +
local_bh_disable() and others.

> Because as is, this patch isn't needed.
I can hold on to this and maybe it is not needed the final version of
softirq ends up to be different :)

Thanks.

Sebastian