linux-kernel - Re: [PATCH v2 (repost)] locking/lockdep: add debug_show_all_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: <Y8J3gAXLf4yc0FcL@gmail.com>
Date:   Sat, 14 Jan 2023 10:36:00 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2 (repost)] locking/lockdep: add
 debug_show_all_lock_holders()


* Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp> wrote:

> --- a/include/linux/debug_locks.h
> +++ b/include/linux/debug_locks.h
> @@ -48,7 +48,18 @@ extern int debug_locks_off(void);
>  #endif
>  
>  #ifdef CONFIG_LOCKDEP
> -extern void debug_show_all_locks(void);
> +extern void __debug_show_all_locks(bool show_stack);
> +
> +static inline void debug_show_all_locks(void)
> +{
> +	__debug_show_all_locks(false);
> +}
> +
> +static inline void debug_show_all_lock_holders(void)
> +{
> +	__debug_show_all_locks(true);
> +}
> +
>  extern void debug_show_held_locks(struct task_struct *task);
>  extern void debug_check_no_locks_freed(const void *from, unsigned long len);
>  extern void debug_check_no_locks_held(void);
> @@ -61,6 +72,10 @@ static inline void debug_show_held_locks(struct task_struct *task)
>  {
>  }
>  
> +static inline void debug_show_all_lock_holders(void)
> +{
> +}
> +

> -		debug_show_all_locks();
> +		debug_show_all_lock_holders();

> -void debug_show_all_locks(void)
> +void __debug_show_all_locks(bool show_stack)
>  {
>  	struct task_struct *g, *p;
>  
> @@ -6495,12 +6496,19 @@ void debug_show_all_locks(void)
>  		pr_warn("INFO: lockdep is turned off.\n");
>  		return;
>  	}
> -	pr_warn("\nShowing all locks held in the system:\n");
> +	if (show_stack)
> +		pr_warn("\nShowing all threads with locks held in the system:\n");
> +	else
> +		pr_warn("\nShowing all locks held in the system:\n");
>  
>  	rcu_read_lock();
>  	for_each_process_thread(g, p) {
>  		if (!p->lockdep_depth)
>  			continue;
> +		if (p == current && p->lockdep_depth == 1)
> +			continue;
> +		if (show_stack)
> +			sched_show_task(p);
>  		lockdep_print_held_locks(p);
>  		touch_nmi_watchdog();
>  		touch_all_softlockup_watchdogs();

Yeah, so note how you introduce a function with a parameter:

	void __debug_show_all_locks(bool show_stack)

... only to then *hide* the new parameter via helper functions:

	static inline void debug_show_all_locks(void)
	{
		__debug_show_all_locks(false);
	}

	static inline void debug_show_all_lock_holders(void)
	{
		__debug_show_all_locks(true);
	}

... which is a *strong* hint by our universe that the new parameter was 
probably a bad idea to begin with.

Given how small debug_show_all_locks() is to begin with, I'd suggest simply 
duplicating the loop into debug_show_all_lock_holders() or so.

Thanks,

	Ingo