Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iad51458e:Fastmail
Date:   Thu, 18 Aug 2022 14:49:17 -0700
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Vincent Whitchurch <vincent.whitchurch@axis.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        kernel@axis.com, Waiman Long <longman@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] lockdep: Panic on warning if panic_on_warn is set
Message-ID: <Yv6z3SlfpmN+ZeV1@boqun-archlinux>
References: <20220818114259.2203505-1-vincent.whitchurch@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220818114259.2203505-1-vincent.whitchurch@axis.com>
Precedence: bulk

Hi,

On Thu, Aug 18, 2022 at 01:42:58PM +0200, Vincent Whitchurch wrote:
> There does not seem to be any way to get the system to panic if a
> lockdep warning is emitted, since those warnings don't use the normal
> WARN() infrastructure.  Panicking on any lockdep warning can be
> desirable when the kernel is being run in a controlled environment
> solely for the purpose of testing.  Make lockdep respect panic_on_warn
> to allow this, similar to KASAN and others.
> 

I'm not completely against this, but could you explain why you want to
panic on lockdep warning? I assume you want to have a kdump so that you
can understand the lock bugs closely? But lockdep discovers lock issue
possiblity, so it's not an after-the-fact detector. In other words, when
lockdep warns, the deadlock cases don't happen in the meanwhile. And
also lockdep tries very hard to print useful information to locate the
issues.

This patch add lockdep_panic() to a few places, and it's a pain for
maintaining. So why do you want to panic on lockdep warning?

Regards,
Boqun

> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
>  kernel/locking/lockdep.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 64a13eb56078..d184bba02630 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -124,6 +124,12 @@ static __always_inline bool lockdep_enabled(void)
>  	return true;
>  }
>  
> +static void lockdep_panic(void)
> +{
> +	if (panic_on_warn)
> +		panic("panic_on_warn set ...\n");
> +}
> +
>  /*
>   * lockdep_lock: protects the lockdep graph, the hashes and the
>   *               class/list/hash allocators.
> @@ -977,6 +983,7 @@ static bool assign_lock_key(struct lockdep_map *lock)
>  		pr_err("you didn't initialize this object before use?\n");
>  		pr_err("turning off the locking correctness validator.\n");
>  		dump_stack();
> +		lockdep_panic();
>  		return false;
>  	}
>  
> @@ -2051,6 +2058,7 @@ static noinline void print_circular_bug(struct lock_list *this,
>  
>  	printk("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  static noinline void print_bfs_bug(int ret)
> @@ -2607,6 +2615,7 @@ print_bad_irq_dependency(struct task_struct *curr,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  static const char *state_names[] = {
> @@ -2986,6 +2995,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  /*
> @@ -3583,6 +3593,7 @@ static void print_collision(struct task_struct *curr,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  #endif
>  
> @@ -3959,6 +3970,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  /*
> @@ -4038,6 +4050,7 @@ print_irq_inversion_bug(struct task_struct *curr,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  /*
> @@ -4703,6 +4716,7 @@ print_lock_invalid_wait_context(struct task_struct *curr,
>  
>  	pr_warn("stack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  
>  	return 0;
>  }
> @@ -4892,6 +4906,7 @@ print_lock_nested_lock_not_held(struct task_struct *curr,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  static int __lock_is_held(const struct lockdep_map *lock, int read);
> @@ -5104,6 +5119,7 @@ static void print_unlock_imbalance_bug(struct task_struct *curr,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  static noinstr int match_held_lock(const struct held_lock *hlock,
> @@ -5795,6 +5811,7 @@ static void print_lock_contention_bug(struct task_struct *curr,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  static void
> @@ -6420,6 +6437,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,
>  
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  static inline int not_in_range(const void* mem_from, unsigned long mem_len,
> @@ -6475,6 +6493,7 @@ static void print_held_locks_bug(void)
>  	lockdep_print_held_locks(current);
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  
>  void debug_check_no_locks_held(void)
> @@ -6593,5 +6612,6 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
>  	lockdep_print_held_locks(curr);
>  	pr_warn("\nstack backtrace:\n");
>  	dump_stack();
> +	lockdep_panic();
>  }
>  EXPORT_SYMBOL_GPL(lockdep_rcu_suspicious);
> -- 
> 2.34.1
>