From: peterz@infradead.org peterz@infradead.org
[ Upstream commit 23870f1227680d2aacff6f79c3ab2222bd04e86e ]
During the LPC RCU BoF Paul asked how come the "USED" <- "IN-NMI" detector doesn't trip over rcu_read_lock()'s lockdep annotation.
Looking into this I found a very embarrasing typo in verify_lock_unused():
- if (!(class->usage_mask & LOCK_USED)) + if (!(class->usage_mask & LOCKF_USED))
fixing that will indeed cause rcu_read_lock() to insta-splat :/
The above typo means that instead of testing for: 0x100 (1 << LOCK_USED), we test for 8 (LOCK_USED), which corresponds to (1 << LOCK_ENABLED_HARDIRQ).
So instead of testing for _any_ used lock, it will only match any lock used with interrupts enabled.
The rcu_read_lock() annotation uses .check=0, which means it will not set any of the interrupt bits and will thus never match.
In order to properly fix the situation and allow rcu_read_lock() to correctly work, split LOCK_USED into LOCK_USED and LOCK_USED_READ and by having .read users set USED_READ and test USED, pure read-recursive locks are permitted.
Fixes: f6f48e180404 ("lockdep: Teach lockdep about "USED" <- "IN-NMI" inversions") Signed-off-by: Peter Zijlstra (Intel) peterz@infradead.org Signed-off-by: Ingo Molnar mingo@kernel.org Tested-by: Masami Hiramatsu mhiramat@kernel.org Acked-by: Paul E. McKenney paulmck@kernel.org Link: https://lore.kernel.org/r/20200902160323.GK1362448@hirez.programming.kicks-a... Signed-off-by: Sasha Levin sashal@kernel.org --- kernel/locking/lockdep.c | 35 +++++++++++++++++++++++++----- kernel/locking/lockdep_internals.h | 2 ++ 2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 29a8de4c50b90..a611dedac7d60 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3923,13 +3923,18 @@ static int separate_irq_context(struct task_struct *curr, static int mark_lock(struct task_struct *curr, struct held_lock *this, enum lock_usage_bit new_bit) { - unsigned int new_mask = 1 << new_bit, ret = 1; + unsigned int old_mask, new_mask, ret = 1;
if (new_bit >= LOCK_USAGE_STATES) { DEBUG_LOCKS_WARN_ON(1); return 0; }
+ if (new_bit == LOCK_USED && this->read) + new_bit = LOCK_USED_READ; + + new_mask = 1 << new_bit; + /* * If already set then do not dirty the cacheline, * nor do any checks: @@ -3942,13 +3947,22 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, /* * Make sure we didn't race: */ - if (unlikely(hlock_class(this)->usage_mask & new_mask)) { - graph_unlock(); - return 1; - } + if (unlikely(hlock_class(this)->usage_mask & new_mask)) + goto unlock;
+ old_mask = hlock_class(this)->usage_mask; hlock_class(this)->usage_mask |= new_mask;
+ /* + * Save one usage_traces[] entry and map both LOCK_USED and + * LOCK_USED_READ onto the same entry. + */ + if (new_bit == LOCK_USED || new_bit == LOCK_USED_READ) { + if (old_mask & (LOCKF_USED | LOCKF_USED_READ)) + goto unlock; + new_bit = LOCK_USED; + } + if (!(hlock_class(this)->usage_traces[new_bit] = save_trace())) return 0;
@@ -3962,6 +3976,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, return 0; }
+unlock: graph_unlock();
/* @@ -4896,12 +4911,20 @@ static void verify_lock_unused(struct lockdep_map *lock, struct held_lock *hlock { #ifdef CONFIG_PROVE_LOCKING struct lock_class *class = look_up_lock_class(lock, subclass); + unsigned long mask = LOCKF_USED;
/* if it doesn't have a class (yet), it certainly hasn't been used yet */ if (!class) return;
- if (!(class->usage_mask & LOCK_USED)) + /* + * READ locks only conflict with USED, such that if we only ever use + * READ locks, there is no deadlock possible -- RCU. + */ + if (!hlock->read) + mask |= LOCKF_USED_READ; + + if (!(class->usage_mask & mask)) return;
hlock->class_idx = class - lock_classes; diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h index baca699b94e91..b0be1560ed17a 100644 --- a/kernel/locking/lockdep_internals.h +++ b/kernel/locking/lockdep_internals.h @@ -19,6 +19,7 @@ enum lock_usage_bit { #include "lockdep_states.h" #undef LOCKDEP_STATE LOCK_USED, + LOCK_USED_READ, LOCK_USAGE_STATES };
@@ -40,6 +41,7 @@ enum { #include "lockdep_states.h" #undef LOCKDEP_STATE __LOCKF(USED) + __LOCKF(USED_READ) };
#define LOCKDEP_STATE(__STATE) LOCKF_ENABLED_##__STATE |