* [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps()
@ 2020-03-12 15:12 Boqun Feng
2020-03-13 9:33 ` Peter Zijlstra
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Boqun Feng @ 2020-03-12 15:12 UTC (permalink / raw)
To: linux-kernel, rcu
Cc: Joel Fernandes (Google),
Paul E. McKenney, Madhuparna Bhowmik, Boqun Feng, Qian Cai,
Peter Zijlstra, Ingo Molnar, Will Deacon
Qian Cai reported a bug when PROVE_RCU_LIST=y, and read on /proc/lockdep
triggered a warning:
[26405.676199][ T3548] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
...
[26406.402408][ T3548] Call Trace:
[26406.416739][ T3548] lock_is_held_type+0x5d/0x150
[26406.438262][ T3548] ? rcu_lockdep_current_cpu_online+0x64/0x80
[26406.466463][ T3548] rcu_read_lock_any_held+0xac/0x100
[26406.490105][ T3548] ? rcu_read_lock_held+0xc0/0xc0
[26406.513258][ T3548] ? __slab_free+0x421/0x540
[26406.535012][ T3548] ? kasan_kmalloc+0x9/0x10
[26406.555901][ T3548] ? __kmalloc_node+0x1d7/0x320
[26406.578668][ T3548] ? kvmalloc_node+0x6f/0x80
[26406.599872][ T3548] __bfs+0x28a/0x3c0
[26406.617075][ T3548] ? class_equal+0x30/0x30
[26406.637524][ T3548] lockdep_count_forward_deps+0x11a/0x1a0
The warning got triggered because lockdep_count_forward_deps() call
__bfs() without current->lockdep_recursion being set, as a result
a lockdep internal function (__bfs()) is checked by lockdep, which is
unexpected, and the inconsistency between the irq-off state and the
state traced by lockdep caused the warning.
Apart from this warning, lockdep internal functions like __bfs() should
always be protected by current->lockdep_recursion to avoid potential
deadlocks and data inconsistency, therefore add the
current->lockdep_recursion on-and-off section to protect __bfs() in both
lockdep_count_forward_deps() and lockdep_count_backward_deps()
Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
kernel/locking/lockdep.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 32406ef0d6a2..5142a6b11bf5 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1719,9 +1719,11 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class)
this.class = class;
raw_local_irq_save(flags);
+ current->lockdep_recursion = 1;
arch_spin_lock(&lockdep_lock);
ret = __lockdep_count_forward_deps(&this);
arch_spin_unlock(&lockdep_lock);
+ current->lockdep_recursion = 0;
raw_local_irq_restore(flags);
return ret;
@@ -1746,9 +1748,11 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class)
this.class = class;
raw_local_irq_save(flags);
+ current->lockdep_recursion = 1;
arch_spin_lock(&lockdep_lock);
ret = __lockdep_count_backward_deps(&this);
arch_spin_unlock(&lockdep_lock);
+ current->lockdep_recursion = 0;
raw_local_irq_restore(flags);
return ret;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() 2020-03-12 15:12 [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Boqun Feng @ 2020-03-13 9:33 ` Peter Zijlstra 2020-03-15 1:04 ` Joel Fernandes 2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Fix bad recursion pattern tip-bot2 for Peter Zijlstra 2020-03-13 10:21 ` [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Peter Zijlstra 2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() tip-bot2 for Boqun Feng 2 siblings, 2 replies; 9+ messages in thread From: Peter Zijlstra @ 2020-03-13 9:33 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, rcu, Joel Fernandes (Google), Paul E. McKenney, Madhuparna Bhowmik, Qian Cai, Ingo Molnar, Will Deacon On Thu, Mar 12, 2020 at 11:12:55PM +0800, Boqun Feng wrote: Thanks! > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > index 32406ef0d6a2..5142a6b11bf5 100644 > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -1719,9 +1719,11 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class) > this.class = class; > > raw_local_irq_save(flags); > + current->lockdep_recursion = 1; > arch_spin_lock(&lockdep_lock); > ret = __lockdep_count_forward_deps(&this); > arch_spin_unlock(&lockdep_lock); > + current->lockdep_recursion = 0; > raw_local_irq_restore(flags); > > return ret; > @@ -1746,9 +1748,11 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class) > this.class = class; > > raw_local_irq_save(flags); > + current->lockdep_recursion = 1; > arch_spin_lock(&lockdep_lock); > ret = __lockdep_count_backward_deps(&this); > arch_spin_unlock(&lockdep_lock); > + current->lockdep_recursion = 0; > raw_local_irq_restore(flags); > > return ret; This copies a bad pattern though; all the sites that do not check lockdep_recursion_count first really should be using ++/-- instead. But I just found there are indeed already a few sites that violate this. I've taken this patch and done a general fixup on top. --- Subject: locking/lockdep: Fix bad recursion pattern From: Peter Zijlstra <peterz@infradead.org> Date: Fri Mar 13 09:56:38 CET 2020 There were two patterns for lockdep_recursion: Pattern-A: if (current->lockdep_recursion) return current->lockdep_recursion = 1; /* do stuff */ current->lockdep_recursion = 0; Pattern-B: current->lockdep_recursion++; /* do stuff */ current->lockdep_recursion--; But a third pattern has emerged: Pattern-C: current->lockdep_recursion = 1; /* do stuff */ current->lockdep_recursion = 0; And while this isn't broken per-se, it is highly dangerous because it doesn't nest properly. Get rid of all Pattern-C instances and shore up Pattern-A with a warning. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/locking/lockdep.c | 74 +++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 34 deletions(-) --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -389,6 +389,12 @@ void lockdep_on(void) } EXPORT_SYMBOL(lockdep_on); +static inline void lockdep_recursion_finish(void) +{ + if (WARN_ON_ONCE(--current->lockdep_recursion)) + current->lockdep_recursion = 0; +} + void lockdep_set_selftest_task(struct task_struct *task) { lockdep_selftest_task_struct = task; @@ -1719,11 +1725,11 @@ unsigned long lockdep_count_forward_deps this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; arch_spin_lock(&lockdep_lock); ret = __lockdep_count_forward_deps(&this); arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion = 0; + current->lockdep_recursion--; raw_local_irq_restore(flags); return ret; @@ -1748,11 +1754,11 @@ unsigned long lockdep_count_backward_dep this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; arch_spin_lock(&lockdep_lock); ret = __lockdep_count_backward_deps(&this); arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion = 0; + current->lockdep_recursion--; raw_local_irq_restore(flags); return ret; @@ -3433,9 +3439,9 @@ void lockdep_hardirqs_on(unsigned long i if (DEBUG_LOCKS_WARN_ON(current->hardirq_context)) return; - current->lockdep_recursion = 1; + current->lockdep_recursion++; __trace_hardirqs_on_caller(ip); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); } NOKPROBE_SYMBOL(lockdep_hardirqs_on); @@ -3491,7 +3497,7 @@ void trace_softirqs_on(unsigned long ip) return; } - current->lockdep_recursion = 1; + current->lockdep_recursion++; /* * We'll do an OFF -> ON transition: */ @@ -3506,7 +3512,7 @@ void trace_softirqs_on(unsigned long ip) */ if (curr->hardirqs_enabled) mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); } /* @@ -3759,9 +3765,9 @@ void lockdep_init_map(struct lockdep_map return; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; register_lock_class(lock, subclass, 1); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } } @@ -4441,11 +4447,11 @@ void lock_set_class(struct lockdep_map * return; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; check_flags(flags); if (__lock_set_class(lock, name, key, subclass, ip)) check_chain_key(current); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_set_class); @@ -4458,11 +4464,11 @@ void lock_downgrade(struct lockdep_map * return; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; check_flags(flags); if (__lock_downgrade(lock, ip)) check_chain_key(current); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_downgrade); @@ -4483,11 +4489,11 @@ void lock_acquire(struct lockdep_map *lo raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip); __lock_acquire(lock, subclass, trylock, read, check, irqs_disabled_flags(flags), nest_lock, ip, 0, 0); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_acquire); @@ -4501,11 +4507,11 @@ void lock_release(struct lockdep_map *lo raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; trace_lock_release(lock, ip); if (__lock_release(lock, ip)) check_chain_key(current); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_release); @@ -4521,9 +4527,9 @@ int lock_is_held_type(const struct lockd raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; ret = __lock_is_held(lock, read); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); return ret; @@ -4542,9 +4548,9 @@ struct pin_cookie lock_pin_lock(struct l raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; cookie = __lock_pin_lock(lock); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); return cookie; @@ -4561,9 +4567,9 @@ void lock_repin_lock(struct lockdep_map raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; __lock_repin_lock(lock, cookie); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_repin_lock); @@ -4578,9 +4584,9 @@ void lock_unpin_lock(struct lockdep_map raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; __lock_unpin_lock(lock, cookie); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_unpin_lock); @@ -4716,10 +4722,10 @@ void lock_contended(struct lockdep_map * raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; trace_lock_contended(lock, ip); __lock_contended(lock, ip); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_contended); @@ -4736,9 +4742,9 @@ void lock_acquired(struct lockdep_map *l raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; __lock_acquired(lock, ip); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_acquired); @@ -4963,7 +4969,7 @@ static void free_zapped_rcu(struct rcu_h raw_local_irq_save(flags); arch_spin_lock(&lockdep_lock); - current->lockdep_recursion = 1; + current->lockdep_recursion++; /* closed head */ pf = delayed_free.pf + (delayed_free.index ^ 1); @@ -4975,7 +4981,7 @@ static void free_zapped_rcu(struct rcu_h */ call_rcu_zapped(delayed_free.pf + delayed_free.index); - current->lockdep_recursion = 0; + current->lockdep_recursion--; arch_spin_unlock(&lockdep_lock); raw_local_irq_restore(flags); } @@ -5022,11 +5028,11 @@ static void lockdep_free_key_range_reg(v raw_local_irq_save(flags); arch_spin_lock(&lockdep_lock); - current->lockdep_recursion = 1; + current->lockdep_recursion++; pf = get_pending_free(); __lockdep_free_key_range(pf, start, size); call_rcu_zapped(pf); - current->lockdep_recursion = 0; + current->lockdep_recursion--; arch_spin_unlock(&lockdep_lock); raw_local_irq_restore(flags); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() 2020-03-13 9:33 ` Peter Zijlstra @ 2020-03-15 1:04 ` Joel Fernandes 2020-03-16 13:55 ` Peter Zijlstra 2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Fix bad recursion pattern tip-bot2 for Peter Zijlstra 1 sibling, 1 reply; 9+ messages in thread From: Joel Fernandes @ 2020-03-15 1:04 UTC (permalink / raw) To: Peter Zijlstra Cc: Boqun Feng, linux-kernel, rcu, Paul E. McKenney, Madhuparna Bhowmik, Qian Cai, Ingo Molnar, Will Deacon On Fri, Mar 13, 2020 at 10:33:25AM +0100, Peter Zijlstra wrote: > On Thu, Mar 12, 2020 at 11:12:55PM +0800, Boqun Feng wrote: > > Thanks! Thanks Peter and Boqun, the below patch makes sense to me. Just had some nits below, otherwise: Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > > index 32406ef0d6a2..5142a6b11bf5 100644 > > --- a/kernel/locking/lockdep.c > > +++ b/kernel/locking/lockdep.c > > @@ -1719,9 +1719,11 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class) > > this.class = class; > > > > raw_local_irq_save(flags); > > + current->lockdep_recursion = 1; > > arch_spin_lock(&lockdep_lock); > > ret = __lockdep_count_forward_deps(&this); > > arch_spin_unlock(&lockdep_lock); > > + current->lockdep_recursion = 0; > > raw_local_irq_restore(flags); > > > > return ret; > > @@ -1746,9 +1748,11 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class) > > this.class = class; > > > > raw_local_irq_save(flags); > > + current->lockdep_recursion = 1; > > arch_spin_lock(&lockdep_lock); > > ret = __lockdep_count_backward_deps(&this); > > arch_spin_unlock(&lockdep_lock); > > + current->lockdep_recursion = 0; > > raw_local_irq_restore(flags); > > > > return ret; > > This copies a bad pattern though; all the sites that do not check > lockdep_recursion_count first really should be using ++/-- instead. But > I just found there are indeed already a few sites that violate this. > > I've taken this patch and done a general fixup on top. > > --- > Subject: locking/lockdep: Fix bad recursion pattern > From: Peter Zijlstra <peterz@infradead.org> > Date: Fri Mar 13 09:56:38 CET 2020 > > There were two patterns for lockdep_recursion: > > Pattern-A: > if (current->lockdep_recursion) > return > > current->lockdep_recursion = 1; > /* do stuff */ > current->lockdep_recursion = 0; > > Pattern-B: > current->lockdep_recursion++; > /* do stuff */ > current->lockdep_recursion--; > > But a third pattern has emerged: > > Pattern-C: > current->lockdep_recursion = 1; > /* do stuff */ > current->lockdep_recursion = 0; > > And while this isn't broken per-se, it is highly dangerous because it > doesn't nest properly. > > Get rid of all Pattern-C instances and shore up Pattern-A with a > warning. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/locking/lockdep.c | 74 +++++++++++++++++++++++++---------------------- > 1 file changed, 40 insertions(+), 34 deletions(-) > > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -389,6 +389,12 @@ void lockdep_on(void) > } > EXPORT_SYMBOL(lockdep_on); > > +static inline void lockdep_recursion_finish(void) > +{ > + if (WARN_ON_ONCE(--current->lockdep_recursion)) > + current->lockdep_recursion = 0; > +} > + > void lockdep_set_selftest_task(struct task_struct *task) > { > lockdep_selftest_task_struct = task; > @@ -1719,11 +1725,11 @@ unsigned long lockdep_count_forward_deps > this.class = class; > > raw_local_irq_save(flags); > - current->lockdep_recursion = 1; > + current->lockdep_recursion++; > arch_spin_lock(&lockdep_lock); > ret = __lockdep_count_forward_deps(&this); > arch_spin_unlock(&lockdep_lock); > - current->lockdep_recursion = 0; > + current->lockdep_recursion--; This doesn't look like it should recurse. Why not just use the lockdep_recursion_finish() helper here? > raw_local_irq_restore(flags); > > return ret; > @@ -1748,11 +1754,11 @@ unsigned long lockdep_count_backward_dep > this.class = class; > > raw_local_irq_save(flags); > - current->lockdep_recursion = 1; > + current->lockdep_recursion++; > arch_spin_lock(&lockdep_lock); > ret = __lockdep_count_backward_deps(&this); > arch_spin_unlock(&lockdep_lock); > - current->lockdep_recursion = 0; > + current->lockdep_recursion--; And here. > @@ -4963,7 +4969,7 @@ static void free_zapped_rcu(struct rcu_h > > raw_local_irq_save(flags); > arch_spin_lock(&lockdep_lock); > - current->lockdep_recursion = 1; > + current->lockdep_recursion++; > > /* closed head */ > pf = delayed_free.pf + (delayed_free.index ^ 1); > @@ -4975,7 +4981,7 @@ static void free_zapped_rcu(struct rcu_h > */ > call_rcu_zapped(delayed_free.pf + delayed_free.index); > > - current->lockdep_recursion = 0; > + current->lockdep_recursion--; And here also if it applies. > arch_spin_unlock(&lockdep_lock); > raw_local_irq_restore(flags); > } > @@ -5022,11 +5028,11 @@ static void lockdep_free_key_range_reg(v > > raw_local_irq_save(flags); > arch_spin_lock(&lockdep_lock); > - current->lockdep_recursion = 1; > + current->lockdep_recursion++; > pf = get_pending_free(); > __lockdep_free_key_range(pf, start, size); > call_rcu_zapped(pf); > - current->lockdep_recursion = 0; > + current->lockdep_recursion--; And here also if it applies. thanks! - Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() 2020-03-15 1:04 ` Joel Fernandes @ 2020-03-16 13:55 ` Peter Zijlstra 2020-03-16 15:01 ` Joel Fernandes 0 siblings, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2020-03-16 13:55 UTC (permalink / raw) To: Joel Fernandes Cc: Boqun Feng, linux-kernel, rcu, Paul E. McKenney, Madhuparna Bhowmik, Qian Cai, Ingo Molnar, Will Deacon On Sat, Mar 14, 2020 at 09:04:22PM -0400, Joel Fernandes wrote: > On Fri, Mar 13, 2020 at 10:33:25AM +0100, Peter Zijlstra wrote: > Thanks Peter and Boqun, the below patch makes sense to me. Just had some nits > below, otherwise: > > @@ -1719,11 +1725,11 @@ unsigned long lockdep_count_forward_deps > > this.class = class; > > > > raw_local_irq_save(flags); > > - current->lockdep_recursion = 1; > > + current->lockdep_recursion++; > > arch_spin_lock(&lockdep_lock); > > ret = __lockdep_count_forward_deps(&this); > > arch_spin_unlock(&lockdep_lock); > > - current->lockdep_recursion = 0; > > + current->lockdep_recursion--; > > This doesn't look like it should recurse. Why not just use the > lockdep_recursion_finish() helper here? I chose to only add that to the sites that check recursion on entry. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() 2020-03-16 13:55 ` Peter Zijlstra @ 2020-03-16 15:01 ` Joel Fernandes 0 siblings, 0 replies; 9+ messages in thread From: Joel Fernandes @ 2020-03-16 15:01 UTC (permalink / raw) To: Peter Zijlstra Cc: Boqun Feng, linux-kernel, rcu, Paul E. McKenney, Madhuparna Bhowmik, Qian Cai, Ingo Molnar, Will Deacon On Mon, Mar 16, 2020 at 02:55:07PM +0100, Peter Zijlstra wrote: > On Sat, Mar 14, 2020 at 09:04:22PM -0400, Joel Fernandes wrote: > > On Fri, Mar 13, 2020 at 10:33:25AM +0100, Peter Zijlstra wrote: > > > Thanks Peter and Boqun, the below patch makes sense to me. Just had some nits > > below, otherwise: > > > @@ -1719,11 +1725,11 @@ unsigned long lockdep_count_forward_deps > > > this.class = class; > > > > > > raw_local_irq_save(flags); > > > - current->lockdep_recursion = 1; > > > + current->lockdep_recursion++; > > > arch_spin_lock(&lockdep_lock); > > > ret = __lockdep_count_forward_deps(&this); > > > arch_spin_unlock(&lockdep_lock); > > > - current->lockdep_recursion = 0; > > > + current->lockdep_recursion--; > > > > This doesn't look like it should recurse. Why not just use the > > lockdep_recursion_finish() helper here? > > I chose to only add that to the sites that check recursion on entry. Makes sense, thank you Peter. - Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip: locking/core] locking/lockdep: Fix bad recursion pattern 2020-03-13 9:33 ` Peter Zijlstra 2020-03-15 1:04 ` Joel Fernandes @ 2020-03-20 12:58 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 9+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2020-03-20 12:58 UTC (permalink / raw) To: linux-tip-commits; +Cc: Peter Zijlstra (Intel), x86, LKML The following commit has been merged into the locking/core branch of tip: Commit-ID: 10476e6304222ced7df9b3d5fb0a043b3c2a1ad8 Gitweb: https://git.kernel.org/tip/10476e6304222ced7df9b3d5fb0a043b3c2a1ad8 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Fri, 13 Mar 2020 09:56:38 +01:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Fri, 20 Mar 2020 13:06:25 +01:00 locking/lockdep: Fix bad recursion pattern There were two patterns for lockdep_recursion: Pattern-A: if (current->lockdep_recursion) return current->lockdep_recursion = 1; /* do stuff */ current->lockdep_recursion = 0; Pattern-B: current->lockdep_recursion++; /* do stuff */ current->lockdep_recursion--; But a third pattern has emerged: Pattern-C: current->lockdep_recursion = 1; /* do stuff */ current->lockdep_recursion = 0; And while this isn't broken per-se, it is highly dangerous because it doesn't nest properly. Get rid of all Pattern-C instances and shore up Pattern-A with a warning. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/20200313093325.GW12561@hirez.programming.kicks-ass.net --- kernel/locking/lockdep.c | 74 +++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 2564950..64ea69f 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -390,6 +390,12 @@ void lockdep_on(void) } EXPORT_SYMBOL(lockdep_on); +static inline void lockdep_recursion_finish(void) +{ + if (WARN_ON_ONCE(--current->lockdep_recursion)) + current->lockdep_recursion = 0; +} + void lockdep_set_selftest_task(struct task_struct *task) { lockdep_selftest_task_struct = task; @@ -1723,11 +1729,11 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class) this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; arch_spin_lock(&lockdep_lock); ret = __lockdep_count_forward_deps(&this); arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion = 0; + current->lockdep_recursion--; raw_local_irq_restore(flags); return ret; @@ -1752,11 +1758,11 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class) this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; arch_spin_lock(&lockdep_lock); ret = __lockdep_count_backward_deps(&this); arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion = 0; + current->lockdep_recursion--; raw_local_irq_restore(flags); return ret; @@ -3668,9 +3674,9 @@ void lockdep_hardirqs_on(unsigned long ip) if (DEBUG_LOCKS_WARN_ON(current->hardirq_context)) return; - current->lockdep_recursion = 1; + current->lockdep_recursion++; __trace_hardirqs_on_caller(ip); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); } NOKPROBE_SYMBOL(lockdep_hardirqs_on); @@ -3726,7 +3732,7 @@ void trace_softirqs_on(unsigned long ip) return; } - current->lockdep_recursion = 1; + current->lockdep_recursion++; /* * We'll do an OFF -> ON transition: */ @@ -3741,7 +3747,7 @@ void trace_softirqs_on(unsigned long ip) */ if (curr->hardirqs_enabled) mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); } /* @@ -3995,9 +4001,9 @@ void lockdep_init_map(struct lockdep_map *lock, const char *name, return; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; register_lock_class(lock, subclass, 1); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } } @@ -4677,11 +4683,11 @@ void lock_set_class(struct lockdep_map *lock, const char *name, return; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; check_flags(flags); if (__lock_set_class(lock, name, key, subclass, ip)) check_chain_key(current); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_set_class); @@ -4694,11 +4700,11 @@ void lock_downgrade(struct lockdep_map *lock, unsigned long ip) return; raw_local_irq_save(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; check_flags(flags); if (__lock_downgrade(lock, ip)) check_chain_key(current); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_downgrade); @@ -4719,11 +4725,11 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass, raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip); __lock_acquire(lock, subclass, trylock, read, check, irqs_disabled_flags(flags), nest_lock, ip, 0, 0); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_acquire); @@ -4737,11 +4743,11 @@ void lock_release(struct lockdep_map *lock, unsigned long ip) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; trace_lock_release(lock, ip); if (__lock_release(lock, ip)) check_chain_key(current); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_release); @@ -4757,9 +4763,9 @@ int lock_is_held_type(const struct lockdep_map *lock, int read) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; ret = __lock_is_held(lock, read); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); return ret; @@ -4778,9 +4784,9 @@ struct pin_cookie lock_pin_lock(struct lockdep_map *lock) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; cookie = __lock_pin_lock(lock); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); return cookie; @@ -4797,9 +4803,9 @@ void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie cookie) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; __lock_repin_lock(lock, cookie); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_repin_lock); @@ -4814,9 +4820,9 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; __lock_unpin_lock(lock, cookie); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_unpin_lock); @@ -4952,10 +4958,10 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; trace_lock_contended(lock, ip); __lock_contended(lock, ip); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_contended); @@ -4972,9 +4978,9 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip) raw_local_irq_save(flags); check_flags(flags); - current->lockdep_recursion = 1; + current->lockdep_recursion++; __lock_acquired(lock, ip); - current->lockdep_recursion = 0; + lockdep_recursion_finish(); raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_acquired); @@ -5176,7 +5182,7 @@ static void free_zapped_rcu(struct rcu_head *ch) raw_local_irq_save(flags); arch_spin_lock(&lockdep_lock); - current->lockdep_recursion = 1; + current->lockdep_recursion++; /* closed head */ pf = delayed_free.pf + (delayed_free.index ^ 1); @@ -5188,7 +5194,7 @@ static void free_zapped_rcu(struct rcu_head *ch) */ call_rcu_zapped(delayed_free.pf + delayed_free.index); - current->lockdep_recursion = 0; + current->lockdep_recursion--; arch_spin_unlock(&lockdep_lock); raw_local_irq_restore(flags); } @@ -5235,11 +5241,11 @@ static void lockdep_free_key_range_reg(void *start, unsigned long size) raw_local_irq_save(flags); arch_spin_lock(&lockdep_lock); - current->lockdep_recursion = 1; + current->lockdep_recursion++; pf = get_pending_free(); __lockdep_free_key_range(pf, start, size); call_rcu_zapped(pf); - current->lockdep_recursion = 0; + current->lockdep_recursion--; arch_spin_unlock(&lockdep_lock); raw_local_irq_restore(flags); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() 2020-03-12 15:12 [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Boqun Feng 2020-03-13 9:33 ` Peter Zijlstra @ 2020-03-13 10:21 ` Peter Zijlstra 2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Rework lockdep_lock tip-bot2 for Peter Zijlstra 2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() tip-bot2 for Boqun Feng 2 siblings, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2020-03-13 10:21 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, rcu, Joel Fernandes (Google), Paul E. McKenney, Madhuparna Bhowmik, Qian Cai, Ingo Molnar, Will Deacon On Thu, Mar 12, 2020 at 11:12:55PM +0800, Boqun Feng wrote: > The warning got triggered because lockdep_count_forward_deps() call > __bfs() without current->lockdep_recursion being set, as a result > a lockdep internal function (__bfs()) is checked by lockdep, which is > unexpected, and the inconsistency between the irq-off state and the > state traced by lockdep caused the warning. This also had me look at __bfs(), while there is a WARN in there, it doesn't really assert all the expectations. This lead to the below patch. --- Subject: locking/lockdep: Rework lockdep_lock From: Peter Zijlstra <peterz@infradead.org> Date: Fri Mar 13 11:09:49 CET 2020 A few sites want to assert we own the graph_lock/lockdep_lock, provide a more conventional lock interface for it with a number of trivial debug checks. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/locking/lockdep.c | 89 +++++++++++++++++++++++++---------------------- 1 file changed, 48 insertions(+), 41 deletions(-) --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -84,12 +84,39 @@ module_param(lock_stat, int, 0644); * to use a raw spinlock - we really dont want the spinlock * code to recurse back into the lockdep code... */ -static arch_spinlock_t lockdep_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; +static arch_spinlock_t __lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; +static struct task_struct *__owner; + +static inline void lockdep_lock(void) +{ + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); + + arch_spin_lock(&__lock); + __owner = current; + current->lockdep_recursion++; +} + +static inline void lockdep_unlock(void) +{ + if (debug_locks && DEBUG_LOCKS_WARN_ON(__owner != current)) + return; + + current->lockdep_recursion--; + __owner = NULL; + arch_spin_unlock(&__lock); +} + +static inline bool lockdep_assert_locked(void) +{ + return DEBUG_LOCKS_WARN_ON(__owner != current); +} + static struct task_struct *lockdep_selftest_task_struct; + static int graph_lock(void) { - arch_spin_lock(&lockdep_lock); + lockdep_lock(); /* * Make sure that if another CPU detected a bug while * walking the graph we dont change it (while the other @@ -97,27 +124,15 @@ static int graph_lock(void) * dropped already) */ if (!debug_locks) { - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); return 0; } - /* prevent any recursions within lockdep from causing deadlocks */ - current->lockdep_recursion++; return 1; } -static inline int graph_unlock(void) +static inline void graph_unlock(void) { - if (debug_locks && !arch_spin_is_locked(&lockdep_lock)) { - /* - * The lockdep graph lock isn't locked while we expect it to - * be, we're confused now, bye! - */ - return DEBUG_LOCKS_WARN_ON(1); - } - - current->lockdep_recursion--; - arch_spin_unlock(&lockdep_lock); - return 0; + lockdep_unlock(); } /* @@ -128,7 +143,7 @@ static inline int debug_locks_off_graph_ { int ret = debug_locks_off(); - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); return ret; } @@ -1475,6 +1490,8 @@ static int __bfs(struct lock_list *sourc struct circular_queue *cq = &lock_cq; int ret = 1; + lockdep_assert_locked(); + if (match(source_entry, data)) { *target_entry = source_entry; ret = 0; @@ -1497,8 +1514,6 @@ static int __bfs(struct lock_list *sourc head = get_dep_list(lock, offset); - DEBUG_LOCKS_WARN_ON(!irqs_disabled()); - list_for_each_entry_rcu(entry, head, entry) { if (!lock_accessed(entry)) { unsigned int cq_depth; @@ -1725,11 +1740,9 @@ unsigned long lockdep_count_forward_deps this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion++; - arch_spin_lock(&lockdep_lock); + lockdep_lock(); ret = __lockdep_count_forward_deps(&this); - arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion--; + lockdep_unlock(); raw_local_irq_restore(flags); return ret; @@ -1754,11 +1767,9 @@ unsigned long lockdep_count_backward_dep this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion++; - arch_spin_lock(&lockdep_lock); + lockdep_lock(); ret = __lockdep_count_backward_deps(&this); - arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion--; + lockdep_unlock(); raw_local_irq_restore(flags); return ret; @@ -2813,7 +2824,7 @@ static inline int add_chain_cache(struct * disabled to make this an IRQ-safe lock.. for recursion reasons * lockdep won't complain about its own locking errors. */ - if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) + if (lockdep_assert_locked()) return 0; chain = alloc_lock_chain(); @@ -4968,8 +4979,7 @@ static void free_zapped_rcu(struct rcu_h return; raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); - current->lockdep_recursion++; + lockdep_lock(); /* closed head */ pf = delayed_free.pf + (delayed_free.index ^ 1); @@ -4981,8 +4991,7 @@ static void free_zapped_rcu(struct rcu_h */ call_rcu_zapped(delayed_free.pf + delayed_free.index); - current->lockdep_recursion--; - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); } @@ -5027,13 +5036,11 @@ static void lockdep_free_key_range_reg(v init_data_structures_once(); raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); - current->lockdep_recursion++; + lockdep_lock(); pf = get_pending_free(); __lockdep_free_key_range(pf, start, size); call_rcu_zapped(pf); - current->lockdep_recursion--; - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); /* @@ -5055,10 +5062,10 @@ static void lockdep_free_key_range_imm(v init_data_structures_once(); raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); + lockdep_lock(); __lockdep_free_key_range(pf, start, size); __free_zapped_classes(pf); - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); } @@ -5154,10 +5161,10 @@ static void lockdep_reset_lock_imm(struc unsigned long flags; raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); + lockdep_lock(); __lockdep_reset_lock(pf, lock); __free_zapped_classes(pf); - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); } ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip: locking/core] locking/lockdep: Rework lockdep_lock 2020-03-13 10:21 ` [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Peter Zijlstra @ 2020-03-20 12:58 ` tip-bot2 for Peter Zijlstra 0 siblings, 0 replies; 9+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2020-03-20 12:58 UTC (permalink / raw) To: linux-tip-commits; +Cc: Peter Zijlstra (Intel), x86, LKML The following commit has been merged into the locking/core branch of tip: Commit-ID: 248efb2158f1e23750728e92ad9db3ab60c14485 Gitweb: https://git.kernel.org/tip/248efb2158f1e23750728e92ad9db3ab60c14485 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Fri, 13 Mar 2020 11:09:49 +01:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Fri, 20 Mar 2020 13:06:25 +01:00 locking/lockdep: Rework lockdep_lock A few sites want to assert we own the graph_lock/lockdep_lock, provide a more conventional lock interface for it with a number of trivial debug checks. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/20200313102107.GX12561@hirez.programming.kicks-ass.net --- kernel/locking/lockdep.c | 89 +++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 64ea69f..47e3acb 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -84,12 +84,39 @@ module_param(lock_stat, int, 0644); * to use a raw spinlock - we really dont want the spinlock * code to recurse back into the lockdep code... */ -static arch_spinlock_t lockdep_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; +static arch_spinlock_t __lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; +static struct task_struct *__owner; + +static inline void lockdep_lock(void) +{ + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); + + arch_spin_lock(&__lock); + __owner = current; + current->lockdep_recursion++; +} + +static inline void lockdep_unlock(void) +{ + if (debug_locks && DEBUG_LOCKS_WARN_ON(__owner != current)) + return; + + current->lockdep_recursion--; + __owner = NULL; + arch_spin_unlock(&__lock); +} + +static inline bool lockdep_assert_locked(void) +{ + return DEBUG_LOCKS_WARN_ON(__owner != current); +} + static struct task_struct *lockdep_selftest_task_struct; + static int graph_lock(void) { - arch_spin_lock(&lockdep_lock); + lockdep_lock(); /* * Make sure that if another CPU detected a bug while * walking the graph we dont change it (while the other @@ -97,27 +124,15 @@ static int graph_lock(void) * dropped already) */ if (!debug_locks) { - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); return 0; } - /* prevent any recursions within lockdep from causing deadlocks */ - current->lockdep_recursion++; return 1; } -static inline int graph_unlock(void) +static inline void graph_unlock(void) { - if (debug_locks && !arch_spin_is_locked(&lockdep_lock)) { - /* - * The lockdep graph lock isn't locked while we expect it to - * be, we're confused now, bye! - */ - return DEBUG_LOCKS_WARN_ON(1); - } - - current->lockdep_recursion--; - arch_spin_unlock(&lockdep_lock); - return 0; + lockdep_unlock(); } /* @@ -128,7 +143,7 @@ static inline int debug_locks_off_graph_unlock(void) { int ret = debug_locks_off(); - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); return ret; } @@ -1479,6 +1494,8 @@ static int __bfs(struct lock_list *source_entry, struct circular_queue *cq = &lock_cq; int ret = 1; + lockdep_assert_locked(); + if (match(source_entry, data)) { *target_entry = source_entry; ret = 0; @@ -1501,8 +1518,6 @@ static int __bfs(struct lock_list *source_entry, head = get_dep_list(lock, offset); - DEBUG_LOCKS_WARN_ON(!irqs_disabled()); - list_for_each_entry_rcu(entry, head, entry) { if (!lock_accessed(entry)) { unsigned int cq_depth; @@ -1729,11 +1744,9 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class) this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion++; - arch_spin_lock(&lockdep_lock); + lockdep_lock(); ret = __lockdep_count_forward_deps(&this); - arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion--; + lockdep_unlock(); raw_local_irq_restore(flags); return ret; @@ -1758,11 +1771,9 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class) this.class = class; raw_local_irq_save(flags); - current->lockdep_recursion++; - arch_spin_lock(&lockdep_lock); + lockdep_lock(); ret = __lockdep_count_backward_deps(&this); - arch_spin_unlock(&lockdep_lock); - current->lockdep_recursion--; + lockdep_unlock(); raw_local_irq_restore(flags); return ret; @@ -3046,7 +3057,7 @@ static inline int add_chain_cache(struct task_struct *curr, * disabled to make this an IRQ-safe lock.. for recursion reasons * lockdep won't complain about its own locking errors. */ - if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) + if (lockdep_assert_locked()) return 0; chain = alloc_lock_chain(); @@ -5181,8 +5192,7 @@ static void free_zapped_rcu(struct rcu_head *ch) return; raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); - current->lockdep_recursion++; + lockdep_lock(); /* closed head */ pf = delayed_free.pf + (delayed_free.index ^ 1); @@ -5194,8 +5204,7 @@ static void free_zapped_rcu(struct rcu_head *ch) */ call_rcu_zapped(delayed_free.pf + delayed_free.index); - current->lockdep_recursion--; - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); } @@ -5240,13 +5249,11 @@ static void lockdep_free_key_range_reg(void *start, unsigned long size) init_data_structures_once(); raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); - current->lockdep_recursion++; + lockdep_lock(); pf = get_pending_free(); __lockdep_free_key_range(pf, start, size); call_rcu_zapped(pf); - current->lockdep_recursion--; - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); /* @@ -5268,10 +5275,10 @@ static void lockdep_free_key_range_imm(void *start, unsigned long size) init_data_structures_once(); raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); + lockdep_lock(); __lockdep_free_key_range(pf, start, size); __free_zapped_classes(pf); - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); } @@ -5367,10 +5374,10 @@ static void lockdep_reset_lock_imm(struct lockdep_map *lock) unsigned long flags; raw_local_irq_save(flags); - arch_spin_lock(&lockdep_lock); + lockdep_lock(); __lockdep_reset_lock(pf, lock); __free_zapped_classes(pf); - arch_spin_unlock(&lockdep_lock); + lockdep_unlock(); raw_local_irq_restore(flags); } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [tip: locking/core] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() 2020-03-12 15:12 [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Boqun Feng 2020-03-13 9:33 ` Peter Zijlstra 2020-03-13 10:21 ` [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Peter Zijlstra @ 2020-03-20 12:58 ` tip-bot2 for Boqun Feng 2 siblings, 0 replies; 9+ messages in thread From: tip-bot2 for Boqun Feng @ 2020-03-20 12:58 UTC (permalink / raw) To: linux-tip-commits; +Cc: Qian Cai, Boqun Feng, Peter Zijlstra (Intel), x86, LKML The following commit has been merged into the locking/core branch of tip: Commit-ID: 25016bd7f4caf5fc983bbab7403d08e64cba3004 Gitweb: https://git.kernel.org/tip/25016bd7f4caf5fc983bbab7403d08e64cba3004 Author: Boqun Feng <boqun.feng@gmail.com> AuthorDate: Thu, 12 Mar 2020 23:12:55 +08:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Fri, 20 Mar 2020 13:06:25 +01:00 locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Qian Cai reported a bug when PROVE_RCU_LIST=y, and read on /proc/lockdep triggered a warning: [ ] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled) ... [ ] Call Trace: [ ] lock_is_held_type+0x5d/0x150 [ ] ? rcu_lockdep_current_cpu_online+0x64/0x80 [ ] rcu_read_lock_any_held+0xac/0x100 [ ] ? rcu_read_lock_held+0xc0/0xc0 [ ] ? __slab_free+0x421/0x540 [ ] ? kasan_kmalloc+0x9/0x10 [ ] ? __kmalloc_node+0x1d7/0x320 [ ] ? kvmalloc_node+0x6f/0x80 [ ] __bfs+0x28a/0x3c0 [ ] ? class_equal+0x30/0x30 [ ] lockdep_count_forward_deps+0x11a/0x1a0 The warning got triggered because lockdep_count_forward_deps() call __bfs() without current->lockdep_recursion being set, as a result a lockdep internal function (__bfs()) is checked by lockdep, which is unexpected, and the inconsistency between the irq-off state and the state traced by lockdep caused the warning. Apart from this warning, lockdep internal functions like __bfs() should always be protected by current->lockdep_recursion to avoid potential deadlocks and data inconsistency, therefore add the current->lockdep_recursion on-and-off section to protect __bfs() in both lockdep_count_forward_deps() and lockdep_count_backward_deps() Reported-by: Qian Cai <cai@lca.pw> Signed-off-by: Boqun Feng <boqun.feng@gmail.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/20200312151258.128036-1-boqun.feng@gmail.com --- kernel/locking/lockdep.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index e55c4ee..2564950 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -1723,9 +1723,11 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class) this.class = class; raw_local_irq_save(flags); + current->lockdep_recursion = 1; arch_spin_lock(&lockdep_lock); ret = __lockdep_count_forward_deps(&this); arch_spin_unlock(&lockdep_lock); + current->lockdep_recursion = 0; raw_local_irq_restore(flags); return ret; @@ -1750,9 +1752,11 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class) this.class = class; raw_local_irq_save(flags); + current->lockdep_recursion = 1; arch_spin_lock(&lockdep_lock); ret = __lockdep_count_backward_deps(&this); arch_spin_unlock(&lockdep_lock); + current->lockdep_recursion = 0; raw_local_irq_restore(flags); return ret; ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-20 12:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-12 15:12 [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Boqun Feng
2020-03-13 9:33 ` Peter Zijlstra
2020-03-15 1:04 ` Joel Fernandes
2020-03-16 13:55 ` Peter Zijlstra
2020-03-16 15:01 ` Joel Fernandes
2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Fix bad recursion pattern tip-bot2 for Peter Zijlstra
2020-03-13 10:21 ` [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() Peter Zijlstra
2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Rework lockdep_lock tip-bot2 for Peter Zijlstra
2020-03-20 12:58 ` [tip: locking/core] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps() tip-bot2 for Boqun Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).