linux-kernel - Re: [PATCH rcu 4/7] locking/lockdep: Improve the deadlock scenario print for sync and read lock

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: <20230320121305.GK2194297@hirez.programming.kicks-ass.net>
Date:   Mon, 20 Mar 2023 13:13:05 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     rcu@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Shuah Khan <shuah@...nel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        seanjc@...gle.com, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH rcu 4/7] locking/lockdep: Improve the deadlock scenario
 print for sync and read lock

On Thu, Mar 16, 2023 at 08:13:36PM -0700, Boqun Feng wrote:
> Lock scenario print is always a weak spot of lockdep splats. Improvement
> can be made if we rework the dependency search and the error printing.
> 
> However without touching the graph search, we can improve a little for
> the circular deadlock case, since we have the to-be-added lock
> dependency, and know whether these two locks are read/write/sync.
> 
> In order to know whether a held_lock is sync or not, a bit was
> "stolen" from ->references, which reduce our limit for the same lock
> class nesting from 2^12 to 2^11, and it should still be good enough.
> 
> Besides, since we now have bit in held_lock for sync, we don't need the
> "hardirqoffs being 1" trick, and also we can avoid the __lock_release()
> if we jump out of __lock_acquire() before the held_lock stored.
> 
> With these changes, a deadlock case evolved with read lock and sync gets
> a better print-out from:
> 
> 	[...]  Possible unsafe locking scenario:
> 	[...]
> 	[...]        CPU0                    CPU1
> 	[...]        ----                    ----
> 	[...]   lock(srcuA);
> 	[...]                                lock(srcuB);
> 	[...]                                lock(srcuA);
> 	[...]   lock(srcuB);
> 
> to
> 
> 	[...]  Possible unsafe locking scenario:
> 	[...]
> 	[...]        CPU0                    CPU1
> 	[...]        ----                    ----
> 	[...]   rlock(srcuA);
> 	[...]                                lock(srcuB);
> 	[...]                                lock(srcuA);
> 	[...]   sync(srcuB);
> 
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> ---
>  include/linux/lockdep.h  |  3 ++-
>  kernel/locking/lockdep.c | 48 ++++++++++++++++++++++++++--------------
>  2 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 14d9dbedc6c1..b32256e9e944 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -134,7 +134,8 @@ struct held_lock {
>  	unsigned int read:2;        /* see lock_acquire() comment */
>  	unsigned int check:1;       /* see lock_acquire() comment */
>  	unsigned int hardirqs_off:1;
> -	unsigned int references:12;					/* 32 bits */
> +	unsigned int sync:1;
> +	unsigned int references:11;					/* 32 bits */
>  	unsigned int pin_count;
>  };
>  

Yeah, I suppose we can do that -- another option is to steal some bits
from pin_count, but whatever (references used to be 11 a long while ago,
no problem going back to that).

Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>