Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps()
From: Peter Zijlstra
Date: Fri Mar 13 2020 - 05:33:35 EST
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
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);