[PATCH] lockdep: Do no validate wait context for novalidate class

[PATCH] lockdep: Do no validate wait context for novalidate class

[Sebastian Andrzej Siewior]

The novalidate class is ignored in the lockchain validation but is
considered in the wait context validation.
If a mutex and a spinlock_t is ignored by using
lockdep_set_novalidate_class() then both locks will share the same lock
class. From the wait validation point of view the mutex will then appear
like a spinlock_t and the validator will complain if another mutex will
be acquired.

Ignore the nonvalidate locks from wait context checking.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/locking/lockdep.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 29a8de4c50b90..fb9a642d8ebef 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4067,7 +4067,7 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
 	 */
 	for (depth = curr->lockdep_depth - 1; depth >= 0; depth--) {
 		struct held_lock *prev = curr->held_locks + depth;
-		if (prev->irq_context != next->irq_context)
+		if (prev->check && prev->irq_context != next->irq_context)
 			break;
 	}
 	depth++;
@@ -4078,6 +4078,9 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
 		struct held_lock *prev = curr->held_locks + depth;
 		short prev_inner = hlock_class(prev)->wait_type_inner;
 
+		if (!prev->check)
+			continue;
+
 		if (prev_inner) {
 			/*
 			 * We can have a bigger inner than a previous one
-- 
2.27.0

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

[Sebastian Andrzej Siewior]

On 2020-06-29 22:15:29 [+0200], To linux-kernel@vger.kernel.org wrote:
> The novalidate class is ignored in the lockchain validation but is
> considered in the wait context validation.
> If a mutex and a spinlock_t is ignored by using
> lockdep_set_novalidate_class() then both locks will share the same lock
> class. From the wait validation point of view the mutex will then appear
> like a spinlock_t and the validator will complain if another mutex will
> be acquired.
> 
> Ignore the nonvalidate locks from wait context checking.

ping :)

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/locking/lockdep.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 29a8de4c50b90..fb9a642d8ebef 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4067,7 +4067,7 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
>  	 */
>  	for (depth = curr->lockdep_depth - 1; depth >= 0; depth--) {
>  		struct held_lock *prev = curr->held_locks + depth;
> -		if (prev->irq_context != next->irq_context)
> +		if (prev->check && prev->irq_context != next->irq_context)
>  			break;
>  	}
>  	depth++;
> @@ -4078,6 +4078,9 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
>  		struct held_lock *prev = curr->held_locks + depth;
>  		short prev_inner = hlock_class(prev)->wait_type_inner;
>  
> +		if (!prev->check)
> +			continue;
> +
>  		if (prev_inner) {
>  			/*
>  			 * We can have a bigger inner than a previous one

Sebastian

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

[peterz]

On Mon, Jun 29, 2020 at 10:15:29PM +0200, Sebastian Andrzej Siewior wrote:
> The novalidate class is ignored in the lockchain validation but is
> considered in the wait context validation.
> If a mutex and a spinlock_t is ignored by using
> lockdep_set_novalidate_class() then both locks will share the same lock
> class. From the wait validation point of view the mutex will then appear
> like a spinlock_t and the validator will complain if another mutex will
> be acquired.
> 
> Ignore the nonvalidate locks from wait context checking.

Hurmph.. but how? There was only a single user... /me greps.

drivers/base/core.c:    lockdep_set_novalidate_class(&dev->mutex);
drivers/md/bcache/btree.c:      lockdep_set_novalidate_class(&b->lock);
drivers/md/bcache/btree.c:      lockdep_set_novalidate_class(&b->write_lock);

Urgh.. there's more now :-(

So write_lock, like dev->mutex is a mutex.

Kent, what's the story with b->lock? It appears to have lockdep
annotations, but then is also the novalidate class. Also neither of
these lockdep_set_novalidate_class() thingies have a comment.

Anyway, all 3 users should have the same wait context, so where is the
actual problem?

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

[Sebastian Andrzej Siewior]

On 2020-08-20 13:40:36 [+0200], peterz@infradead.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.

Sebastian

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

[peterz]

On Thu, Aug 20, 2020 at 01:43:48PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-08-20 13:40:36 [+0200], peterz@infradead.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?

Because as is, this patch isn't needed.

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

[Sebastian Andrzej Siewior]

On 2020-08-20 14:38:59 [+0200], peterz@infradead.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@infradead.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