linux-kernel - Re: [tip: locking/core] lockdep: Fix usage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201030093806.GA2628@hirez.programming.kicks-ass.net>
Date:   Fri, 30 Oct 2020 10:38:06 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     Chris Wilson <chris@...is-wilson.co.uk>,
        linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
        tip-bot2 for Peter Zijlstra <tip-bot2@...utronix.de>,
        Qian Cai <cai@...hat.com>, x86 <x86@...nel.org>
Subject: Re: [tip: locking/core] lockdep: Fix usage_traceoverflow

On Fri, Oct 30, 2020 at 11:51:18AM +0800, Boqun Feng wrote:
> On Wed, Oct 28, 2020 at 08:59:10PM +0100, Peter Zijlstra wrote:

> Sorry for the late response.

No worries, glad you could have a look.

> > So that's commit f611e8cf98ec ("lockdep: Take read/write status in
> > consideration when generate chainkey") that did that.
> > 
> 
> Yeah, I think that's related, howver ...

It's the commit that made the chainkey depend on the read state, and
thus introduced this connondrum.

> > So validate_chain() requires the new chain_key, but can change ->read
> > which then invalidates the chain_key we just calculated.
> > 
> > This happens when check_deadlock() returns 2, which only happens when:
> > 
> >   - next->read == 2 && ... ; however @hext is our @hlock, so that's
> >     pointless
> > 
> 
> I don't think we should return 2 (earlier) in this case anymore. Because
> now we have recursive read deadlock detection, it's safe to add dep:
> "prev -> next" in the dependency graph. I think we can just continue in
> this case. Actually I think this is something I'm missing in my
> recursive read detection patchset :-/

Yes, I agree, this case should go. We now fully support recursive read
depndencies per your recent work.

> >   - when there's a nest_lock involved ; ww_mutex uses that !!!
> > 
> 
> That leaves check_deadlock() return 2 only if hlock is a nest_lock, and
> ...

> > @@ -3597,8 +3598,12 @@ static int validate_chain(struct task_struct *curr,
> >  		 * building dependencies (just like we jump over
> >  		 * trylock entries):
> >  		 */
> > -		if (ret == 2)
> > +		if (ret == 2) {
> >  			hlock->read = 2;
> > +			*chain_key = iterate_chain_key(hlock->prev_chain_key, hlock_id(hlock));
> 
> If "ret == 2" means hlock is a a nest_lock, than we don't need the
> "->read = 2" trick here and we don't need to update chain_key either.
> We used to have this "->read = 2" only because we want to skip the
> dependency adding step afterwards. So how about the following:
> 
> It survived a lockdep selftest at boot time.

Right, but our self-tests didn't trigger this problem to begin with, let
me go try and create one that does.

> ----------------------------->8
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 3e99dfef8408..b23ca6196561 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2765,7 +2765,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
>   * (Note that this has to be done separately, because the graph cannot
>   * detect such classes of deadlocks.)
>   *
> - * Returns: 0 on deadlock detected, 1 on OK, 2 on recursive read
> + * Returns: 0 on deadlock detected, 1 on OK, 2 on nest_lock
>   */
>  static int
>  check_deadlock(struct task_struct *curr, struct held_lock *next)
> @@ -2788,7 +2788,7 @@ check_deadlock(struct task_struct *curr, struct held_lock *next)
>  		 * lock class (i.e. read_lock(lock)+read_lock(lock)):
>  		 */
>  		if ((next->read == 2) && prev->read)
> -			return 2;
> +			continue;
>  
>  		/*
>  		 * We're holding the nest_lock, which serializes this lock's
> @@ -3592,16 +3592,9 @@ static int validate_chain(struct task_struct *curr,
>  
>  		if (!ret)
>  			return 0;
> -		/*
> -		 * Mark recursive read, as we jump over it when
> -		 * building dependencies (just like we jump over
> -		 * trylock entries):
> -		 */
> -		if (ret == 2)
> -			hlock->read = 2;
>  		/*
>  		 * Add dependency only if this lock is not the head
> -		 * of the chain, and if it's not a secondary read-lock:
> +		 * of the chain, and if it's not a nest_lock:
>  		 */
>  		if (!chain_head && ret != 2) {
>  			if (!check_prevs_add(curr, hlock))

I'm not entirely sure that doesn't still trigger the problem. Consider
@chain_head := true.

Anyway, let me go try and write this self-tests, maybe that'll get my
snot-addled brains sufficiently aligned to make sense of all this :/