[PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint

[PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint

[Frederic Weisbecker]

Impact: provide useful missing info for developers

Kernel taint can occur in several situations such as warnings,
load of prorietary or staging modules, bad page, etc...

But when such taint happens, a developer might still be working on
the kernel, expecting that lockdep is still enabled. But a taint
disables lockdep without ever warning about it.
Such a kernel behaviour doesn't really help for kernel development.

This patch adds this missing warning.

Since the taint is done most of the time after the main message that
explain the real source issue, it seems safe to warn about it inside
add_taint() so that it appears at last, without hurting the main
information.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

diff --git a/kernel/panic.c b/kernel/panic.c
index 3fd8c5b..9e7420a 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,8 +213,14 @@ unsigned long get_taint(void)
 
 void add_taint(unsigned flag)
 {
-	/* can't trust the integrity of the kernel anymore: */
-	debug_locks = 0;
+	/*
+	 * Can't trust the integrity of the kernel anymore.
+	 * We don't call directly debug_locks_off() because the issue
+	 * is not necessarily serious enough to set oops_in_progress to 1
+	 */
+	if (xchg(&debug_locks, 0))
+		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+
 	set_bit(flag, &tainted_mask);
 }
 EXPORT_SYMBOL(add_taint);
-- 
1.6.1

[PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Frederic Weisbecker]

Lockdep is disabled after any kernel taints. This might be convenient
to ignore bad locking issues which sources come from outside the kernel
tree. Nevertheless, it might be a frustrating experience for the
staging developers or anyone who might develop a kernel that happens
to be tainted.

Just provide an option to let one choose to continue lock-debugging
after any taint.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

diff --git a/kernel/panic.c b/kernel/panic.c
index 9e7420a..eadc0b5 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,6 +213,7 @@ unsigned long get_taint(void)
 
 void add_taint(unsigned flag)
 {
+#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
 	/*
 	 * Can't trust the integrity of the kernel anymore.
 	 * We don't call directly debug_locks_off() because the issue
@@ -220,6 +221,7 @@ void add_taint(unsigned flag)
 	 */
 	if (xchg(&debug_locks, 0))
 		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+#endif
 
 	set_bit(flag, &tainted_mask);
 }
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c6e854f..599888b 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -436,6 +436,20 @@ config PROVE_LOCKING
 
 	 For more details, see Documentation/lockdep-design.txt.
 
+config LOCKDEP_IGNORE_TAINT
+	bool "Lock debugging: continue despite taints"
+	depends on LOCKDEP
+	default n
+	help
+	  If your kernel becomes tainted because of a warning,
+	  a staging or proprietary module loaded, etc... lockdep
+	  will be disabled by default. Select this option if you
+	  want lockdep to continue running despite a taint. If
+	  you are working on a staging driver, experiencing
+	  warnings but focusing on something else, or whatever
+	  reason that makes you developing on a kernel that is
+	  tainted, you might want to say Y here.
+
 config LOCKDEP
 	bool
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
-- 
1.6.1

Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Lockdep is disabled after any kernel taints. This might be 
> convenient to ignore bad locking issues which sources come from 
> outside the kernel tree. Nevertheless, it might be a frustrating 
> experience for the staging developers or anyone who might develop 
> a kernel that happens to be tainted.

Good point. Not having lockdep coverage for drivers/staging/ just 
prolongs their transition - not good.

But instead of this:

>  void add_taint(unsigned flag)
>  {
> +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
>  	/*
>  	 * Can't trust the integrity of the kernel anymore.
>  	 * We don't call directly debug_locks_off() because the issue
> @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
>  	 */
>  	if (xchg(&debug_locks, 0))
>  		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> +#endif

I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e. 
something like:

	if (!(flag & TAINT_CRAP) && debug_locks_off())
		printk(...);

will do the trick.

	Ingo

Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Frederic Weisbecker]

On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Lockdep is disabled after any kernel taints. This might be 
> > convenient to ignore bad locking issues which sources come from 
> > outside the kernel tree. Nevertheless, it might be a frustrating 
> > experience for the staging developers or anyone who might develop 
> > a kernel that happens to be tainted.
> 
> Good point. Not having lockdep coverage for drivers/staging/ just 
> prolongs their transition - not good.
> 
> But instead of this:
> 
> >  void add_taint(unsigned flag)
> >  {
> > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> >  	/*
> >  	 * Can't trust the integrity of the kernel anymore.
> >  	 * We don't call directly debug_locks_off() because the issue
> > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> >  	 */
> >  	if (xchg(&debug_locks, 0))
> >  		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > +#endif
> 
> I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e. 
> something like:
> 
> 	if (!(flag & TAINT_CRAP) && debug_locks_off())
> 		printk(...);
> 
> will do the trick.
> 
> 	Ingo


Ok, but this is not only about staging. It's also about TAINT_WARN.
Just imagine that you report a warning to a maintainer, and while
you are waiting for it to be fixed, you can't use lockdep for your
own needs.

Hm?

Frederic.

Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > Lockdep is disabled after any kernel taints. This might be 
> > > convenient to ignore bad locking issues which sources come from 
> > > outside the kernel tree. Nevertheless, it might be a frustrating 
> > > experience for the staging developers or anyone who might develop 
> > > a kernel that happens to be tainted.
> > 
> > Good point. Not having lockdep coverage for drivers/staging/ just 
> > prolongs their transition - not good.
> > 
> > But instead of this:
> > 
> > >  void add_taint(unsigned flag)
> > >  {
> > > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > >  	/*
> > >  	 * Can't trust the integrity of the kernel anymore.
> > >  	 * We don't call directly debug_locks_off() because the issue
> > > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > >  	 */
> > >  	if (xchg(&debug_locks, 0))
> > >  		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > +#endif
> > 
> > I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e. 
> > something like:
> > 
> > 	if (!(flag & TAINT_CRAP) && debug_locks_off())
> > 		printk(...);
> > 
> > will do the trick.
> > 
> > 	Ingo
> 
> 
> Ok, but this is not only about staging. It's also about 
> TAINT_WARN. Just imagine that you report a warning to a 
> maintainer, and while you are waiting for it to be fixed, you 
> can't use lockdep for your own needs.
> 
> Hm?

We can exclude TAINT_WARN too - i.e. (TAINT_CRAP|TAINT_WARN).

	Ingo

Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Frederic Weisbecker]

On Fri, Apr 10, 2009 at 03:45:20PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> > > 
> > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > 
> > > > Lockdep is disabled after any kernel taints. This might be 
> > > > convenient to ignore bad locking issues which sources come from 
> > > > outside the kernel tree. Nevertheless, it might be a frustrating 
> > > > experience for the staging developers or anyone who might develop 
> > > > a kernel that happens to be tainted.
> > > 
> > > Good point. Not having lockdep coverage for drivers/staging/ just 
> > > prolongs their transition - not good.
> > > 
> > > But instead of this:
> > > 
> > > >  void add_taint(unsigned flag)
> > > >  {
> > > > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > > >  	/*
> > > >  	 * Can't trust the integrity of the kernel anymore.
> > > >  	 * We don't call directly debug_locks_off() because the issue
> > > > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > > >  	 */
> > > >  	if (xchg(&debug_locks, 0))
> > > >  		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > > +#endif
> > > 
> > > I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e. 
> > > something like:
> > > 
> > > 	if (!(flag & TAINT_CRAP) && debug_locks_off())
> > > 		printk(...);
> > > 
> > > will do the trick.
> > > 
> > > 	Ingo
> > 
> > 
> > Ok, but this is not only about staging. It's also about 
> > TAINT_WARN. Just imagine that you report a warning to a 
> > maintainer, and while you are waiting for it to be fixed, you 
> > can't use lockdep for your own needs.
> > 
> > Hm?
> 
> We can exclude TAINT_WARN too - i.e. (TAINT_CRAP|TAINT_WARN).
> 
> 	Ingo


Fine :-)

See the v2 on further mails in this thread.

Side request: do you think you could merge them on kill-the-BKL tree?

Thanks!

Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Frederic Weisbecker]

On Sat, Apr 11, 2009 at 03:15:06AM +0200, Frederic Weisbecker wrote:
> On Fri, Apr 10, 2009 at 03:45:20PM +0200, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > > 
> > > > > Lockdep is disabled after any kernel taints. This might be 
> > > > > convenient to ignore bad locking issues which sources come from 
> > > > > outside the kernel tree. Nevertheless, it might be a frustrating 
> > > > > experience for the staging developers or anyone who might develop 
> > > > > a kernel that happens to be tainted.
> > > > 
> > > > Good point. Not having lockdep coverage for drivers/staging/ just 
> > > > prolongs their transition - not good.
> > > > 
> > > > But instead of this:
> > > > 
> > > > >  void add_taint(unsigned flag)
> > > > >  {
> > > > > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > > > >  	/*
> > > > >  	 * Can't trust the integrity of the kernel anymore.
> > > > >  	 * We don't call directly debug_locks_off() because the issue
> > > > > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > > > >  	 */
> > > > >  	if (xchg(&debug_locks, 0))
> > > > >  		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > > > +#endif
> > > > 
> > > > I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e. 
> > > > something like:
> > > > 
> > > > 	if (!(flag & TAINT_CRAP) && debug_locks_off())
> > > > 		printk(...);
> > > > 
> > > > will do the trick.
> > > > 
> > > > 	Ingo
> > > 
> > > 
> > > Ok, but this is not only about staging. It's also about 
> > > TAINT_WARN. Just imagine that you report a warning to a 
> > > maintainer, and while you are waiting for it to be fixed, you 
> > > can't use lockdep for your own needs.
> > > 
> > > Hm?
> > 
> > We can exclude TAINT_WARN too - i.e. (TAINT_CRAP|TAINT_WARN).
> > 
> > 	Ingo
> 
> 
> Fine :-)
> 
> See the v2 on further mails in this thread.
> 
> Side request: do you think you could merge them on kill-the-BKL tree?


Sorry, forget about this side request. I thought merging tip:master
into tip:core/kill-the-BKL would make me suffer a rain of conflicts
but actually I've just tried and I only encountered a very simple
conflict in sched.c so it's very easy to manage it locally. Then
we can keep the history of kill-the-BKL, for now it's good as is.

Frederic.


> Thanks!
> 

Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > Side request: do you think you could merge them on kill-the-BKL 
> > tree?
> 
> Sorry, forget about this side request. I thought merging 
> tip:master into tip:core/kill-the-BKL would make me suffer a rain 
> of conflicts but actually I've just tried and I only encountered a 
> very simple conflict in sched.c so it's very easy to manage it 
> locally. Then we can keep the history of kill-the-BKL, for now 
> it's good as is.

Ok - that's how we like topics anyway: as independent trees which 
happen to be mostly conflict-free when merged into tip:master.

	Ingo

[PATCH 1/2 v2] lockdep: warn about lockdep disabling after kernel taint

[Frederic Weisbecker]

Impact: provide useful missing info for developers

Kernel taint can occur in several situations such as warnings,
load of prorietary or staging modules, bad page, etc...

But when such taint happens, a developer might still be working on
the kernel, expecting that lockdep is still enabled. But a taint
disables lockdep without ever warning about it.
Such a kernel behaviour doesn't really help for kernel development.

This patch adds this missing warning.

Since the taint is done most of the time after the main message that
explain the real source issue, it seems safe to warn about it inside
add_taint() so that it appears at last, without hurting the main
information.

v2: Use a generic helper to disable lockdep instead of an open coded
xchg().

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/debug_locks.h |    7 +++++++
 kernel/panic.c              |   10 ++++++++--
 lib/debug_locks.c           |    2 +-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/debug_locks.h b/include/linux/debug_locks.h
index 096476f..493dedb 100644
--- a/include/linux/debug_locks.h
+++ b/include/linux/debug_locks.h
@@ -2,12 +2,19 @@
 #define __LINUX_DEBUG_LOCKING_H
 
 #include <linux/kernel.h>
+#include <asm/atomic.h>
 
 struct task_struct;
 
 extern int debug_locks;
 extern int debug_locks_silent;
 
+
+static inline int __debug_locks_off(void)
+{
+	return xchg(&debug_locks, 0);
+}
+
 /*
  * Generic 'turn off all lock debugging' function:
  */
diff --git a/kernel/panic.c b/kernel/panic.c
index 3fd8c5b..940ca14 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,8 +213,14 @@ unsigned long get_taint(void)
 
 void add_taint(unsigned flag)
 {
-	/* can't trust the integrity of the kernel anymore: */
-	debug_locks = 0;
+	/*
+	 * Can't trust the integrity of the kernel anymore.
+	 * We don't call directly debug_locks_off() because the issue
+	 * is not necessarily serious enough to set oops_in_progress to 1
+	 */
+	if (__debug_locks_off())
+		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+
 	set_bit(flag, &tainted_mask);
 }
 EXPORT_SYMBOL(add_taint);
diff --git a/lib/debug_locks.c b/lib/debug_locks.c
index 0218b46..bc3b117 100644
--- a/lib/debug_locks.c
+++ b/lib/debug_locks.c
@@ -36,7 +36,7 @@ int debug_locks_silent;
  */
 int debug_locks_off(void)
 {
-	if (xchg(&debug_locks, 0)) {
+	if (__debug_locks_off()) {
 		if (!debug_locks_silent) {
 			oops_in_progress = 1;
 			console_verbose();
-- 
1.6.1

[tip:core/urgent] lockdep: warn about lockdep disabling after kernel taint

[Frederic Weisbecker]

Commit-ID:  9eeba6138cefc0435695463ddadb0d95e0a6bcd2
Gitweb:     http://git.kernel.org/tip/9eeba6138cefc0435695463ddadb0d95e0a6bcd2
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sat, 11 Apr 2009 03:17:17 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 12 Apr 2009 16:10:51 +0200

lockdep: warn about lockdep disabling after kernel taint

Impact: provide useful missing info for developers

Kernel taint can occur in several situations such as warnings,
load of prorietary or staging modules, bad page, etc...

But when such taint happens, a developer might still be working on
the kernel, expecting that lockdep is still enabled. But a taint
disables lockdep without ever warning about it.
Such a kernel behaviour doesn't really help for kernel development.

This patch adds this missing warning.

Since the taint is done most of the time after the main message that
explain the real source issue, it seems safe to warn about it inside
add_taint() so that it appears at last, without hurting the main
information.

v2: Use a generic helper to disable lockdep instead of an
    open coded xchg().

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
LKML-Reference: <1239412638-6739-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 include/linux/debug_locks.h |    7 +++++++
 kernel/panic.c              |   10 ++++++++--
 lib/debug_locks.c           |    2 +-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/debug_locks.h b/include/linux/debug_locks.h
index 096476f..493dedb 100644
--- a/include/linux/debug_locks.h
+++ b/include/linux/debug_locks.h
@@ -2,12 +2,19 @@
 #define __LINUX_DEBUG_LOCKING_H
 
 #include <linux/kernel.h>
+#include <asm/atomic.h>
 
 struct task_struct;
 
 extern int debug_locks;
 extern int debug_locks_silent;
 
+
+static inline int __debug_locks_off(void)
+{
+	return xchg(&debug_locks, 0);
+}
+
 /*
  * Generic 'turn off all lock debugging' function:
  */
diff --git a/kernel/panic.c b/kernel/panic.c
index 3fd8c5b..940ca14 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,8 +213,14 @@ unsigned long get_taint(void)
 
 void add_taint(unsigned flag)
 {
-	/* can't trust the integrity of the kernel anymore: */
-	debug_locks = 0;
+	/*
+	 * Can't trust the integrity of the kernel anymore.
+	 * We don't call directly debug_locks_off() because the issue
+	 * is not necessarily serious enough to set oops_in_progress to 1
+	 */
+	if (__debug_locks_off())
+		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+
 	set_bit(flag, &tainted_mask);
 }
 EXPORT_SYMBOL(add_taint);
diff --git a/lib/debug_locks.c b/lib/debug_locks.c
index 0218b46..bc3b117 100644
--- a/lib/debug_locks.c
+++ b/lib/debug_locks.c
@@ -36,7 +36,7 @@ int debug_locks_silent;
  */
 int debug_locks_off(void)
 {
-	if (xchg(&debug_locks, 0)) {
+	if (__debug_locks_off()) {
 		if (!debug_locks_silent) {
 			oops_in_progress = 1;
 			console_verbose();

[PATCH 2/2 v2] lockdep: continue lock debugging despite some taints

[Frederic Weisbecker]

Lockdep is disabled after any kernel taints. This might be convenient
to ignore bad locking issues which sources come from outside the kernel
tree. Nevertheless, it might be a frustrating experience for the
staging developers or those who experience a warning but are focused
on another things that require lockdep.

The v2 of this patch simply don't disable anymore lockdep in case
of TAINT_CRAP and TAINT_WARN events.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg KH <gregkh@suse.de>
---
 kernel/panic.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 940ca14..934fb37 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -217,8 +217,10 @@ void add_taint(unsigned flag)
 	 * Can't trust the integrity of the kernel anymore.
 	 * We don't call directly debug_locks_off() because the issue
 	 * is not necessarily serious enough to set oops_in_progress to 1
+	 * Also we want to keep up lockdep for staging development and
+	 * post-warning case.
 	 */
-	if (__debug_locks_off())
+	if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
 		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
 
 	set_bit(flag, &tainted_mask);
-- 
1.6.1

[tip:core/urgent] lockdep: continue lock debugging despite some taints

[Frederic Weisbecker]

Commit-ID:  574bbe782057fdf0490dc7dec906a2dc26363e20
Gitweb:     http://git.kernel.org/tip/574bbe782057fdf0490dc7dec906a2dc26363e20
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sat, 11 Apr 2009 03:17:18 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 12 Apr 2009 16:10:52 +0200

lockdep: continue lock debugging despite some taints

Impact: broaden lockdep checks

Lockdep is disabled after any kernel taints. This might be convenient
to ignore bad locking issues which sources come from outside the kernel
tree. Nevertheless, it might be a frustrating experience for the
staging developers or those who experience a warning but are focused
on another things that require lockdep.

The v2 of this patch simply don't disable anymore lockdep in case
of TAINT_CRAP and TAINT_WARN events.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: LTP <ltp-list@lists.sourceforge.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg KH <gregkh@suse.de>
LKML-Reference: <1239412638-6739-2-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/panic.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 940ca14..934fb37 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -217,8 +217,10 @@ void add_taint(unsigned flag)
 	 * Can't trust the integrity of the kernel anymore.
 	 * We don't call directly debug_locks_off() because the issue
 	 * is not necessarily serious enough to set oops_in_progress to 1
+	 * Also we want to keep up lockdep for staging development and
+	 * post-warning case.
 	 */
-	if (__debug_locks_off())
+	if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
 		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
 
 	set_bit(flag, &tainted_mask);

Re: [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Impact: provide useful missing info for developers
> 
> Kernel taint can occur in several situations such as warnings,
> load of prorietary or staging modules, bad page, etc...
> 
> But when such taint happens, a developer might still be working on
> the kernel, expecting that lockdep is still enabled. But a taint
> disables lockdep without ever warning about it.
> Such a kernel behaviour doesn't really help for kernel development.
> 
> This patch adds this missing warning.
> 
> Since the taint is done most of the time after the main message that
> explain the real source issue, it seems safe to warn about it inside
> add_taint() so that it appears at last, without hurting the main
> information.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 3fd8c5b..9e7420a 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -213,8 +213,14 @@ unsigned long get_taint(void)
>  
>  void add_taint(unsigned flag)
>  {
> -	/* can't trust the integrity of the kernel anymore: */
> -	debug_locks = 0;
> +	/*
> +	 * Can't trust the integrity of the kernel anymore.
> +	 * We don't call directly debug_locks_off() because the issue
> +	 * is not necessarily serious enough to set oops_in_progress to 1
> +	 */
> +	if (xchg(&debug_locks, 0))
> +		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> +

nice idea - but please use the proper debug_locks_off() construct 
instead of an open-coded xchg(). Something like:

	if (debug_locks_off())
		printk(...);

should do the trick.

	Ingo

Re: [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint

[Frederic Weisbecker]

On Fri, Apr 10, 2009 at 02:12:43PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Impact: provide useful missing info for developers
> > 
> > Kernel taint can occur in several situations such as warnings,
> > load of prorietary or staging modules, bad page, etc...
> > 
> > But when such taint happens, a developer might still be working on
> > the kernel, expecting that lockdep is still enabled. But a taint
> > disables lockdep without ever warning about it.
> > Such a kernel behaviour doesn't really help for kernel development.
> > 
> > This patch adds this missing warning.
> > 
> > Since the taint is done most of the time after the main message that
> > explain the real source issue, it seems safe to warn about it inside
> > add_taint() so that it appears at last, without hurting the main
> > information.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 3fd8c5b..9e7420a 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -213,8 +213,14 @@ unsigned long get_taint(void)
> >  
> >  void add_taint(unsigned flag)
> >  {
> > -	/* can't trust the integrity of the kernel anymore: */
> > -	debug_locks = 0;
> > +	/*
> > +	 * Can't trust the integrity of the kernel anymore.
> > +	 * We don't call directly debug_locks_off() because the issue
> > +	 * is not necessarily serious enough to set oops_in_progress to 1
> > +	 */
> > +	if (xchg(&debug_locks, 0))
> > +		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > +
> 
> nice idea - but please use the proper debug_locks_off() construct 
> instead of an open-coded xchg(). Something like:
> 
> 	if (debug_locks_off())
> 		printk(...);
> 
> should do the trick.
> 
> 	Ingo


Yeah, I first wanted to do so but was shy about the oops_in_progress = 1
inside debug_locks_off(). Isn't it a problem?

Thanks,
Frederic.

Re: [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Apr 10, 2009 at 02:12:43PM +0200, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > Impact: provide useful missing info for developers
> > > 
> > > Kernel taint can occur in several situations such as warnings,
> > > load of prorietary or staging modules, bad page, etc...
> > > 
> > > But when such taint happens, a developer might still be working on
> > > the kernel, expecting that lockdep is still enabled. But a taint
> > > disables lockdep without ever warning about it.
> > > Such a kernel behaviour doesn't really help for kernel development.
> > > 
> > > This patch adds this missing warning.
> > > 
> > > Since the taint is done most of the time after the main message that
> > > explain the real source issue, it seems safe to warn about it inside
> > > add_taint() so that it appears at last, without hurting the main
> > > information.
> > > 
> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > > 
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index 3fd8c5b..9e7420a 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -213,8 +213,14 @@ unsigned long get_taint(void)
> > >  
> > >  void add_taint(unsigned flag)
> > >  {
> > > -	/* can't trust the integrity of the kernel anymore: */
> > > -	debug_locks = 0;
> > > +	/*
> > > +	 * Can't trust the integrity of the kernel anymore.
> > > +	 * We don't call directly debug_locks_off() because the issue
> > > +	 * is not necessarily serious enough to set oops_in_progress to 1
> > > +	 */
> > > +	if (xchg(&debug_locks, 0))
> > > +		printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > +
> > 
> > nice idea - but please use the proper debug_locks_off() construct 
> > instead of an open-coded xchg(). Something like:
> > 
> > 	if (debug_locks_off())
> > 		printk(...);
> > 
> > should do the trick.
> > 
> > 	Ingo
> 
> Yeah, I first wanted to do so but was shy about the 
> oops_in_progress = 1 inside debug_locks_off(). Isn't it a problem?

hm, indeed. How about providing a __debug_locks_off() primitive that 
only does the xchg and none of the oops_in_progress and 
console_verbose() calls?

	Ingo