Re: [PATCH] locking/lockdep: Avoid recursion in lockdep_count_{for,back}ward_deps()

[Joel Fernandes]

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@xxxxxxxxxxxxxxxxx>

> > 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--;

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