lockdep,rtmutex,bug: Show taint flags on error
PATCH 1/2 lockdep,rtmutex,bug: Show taint flags on error
Currently lock debugging is disabled when the kernel is tainted, with a few exceptions. It is already recognised that this can be useful for staging modules (TAINT_CRAP), but that also goes for out-of-tree modules (TAINT_OOT_MODULE) so long as core kernel developers don't have to spend time debugging them. Also, there are several reasons for tainting that are unlikely to introduce false locking bug reports (e.g. TAINT_FIRMWARE_WORKAROUND).
Instead of disabling lock debugging, show the taint flags in all lockdep and rtmutex-debug error messages.
--- kernel/lockdep.c | 23 +++++++++++++++-------- kernel/panic.c | 10 ---------- kernel/rtmutex-debug.c | 1 + 3 files changed, 16 insertions(+), 18 deletions(-)
kernel/lockdep.c +15 -8
@@ -567,11 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr) } } -static void print_kernel_version (void ) +static void print_kernel_ident (void ) { -printk ("%s %.*s\n" ,init_utsname ()->release, +printk ("%s %.*s %s\n" ,init_utsname ()->release, (int )strcspn (init_utsname ()->version," " ), -init_utsname ()->version); +init_utsname ()->version, +print_tainted ()); }static int very_verbose (struct lock_class *class)
@@ -1148,7 +1149,7 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,printk ("\n" );printk ("======================================================\n" );printk ("[ INFO: possible circular locking dependency detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("-------------------------------------------------------\n" );printk ("%s/%d is trying to acquire lock:\n" , curr->comm,task_pid_nr (curr));
@@ -1487,7 +1488,7 @@ print_bad_irq_dependency(struct task_struct *curr,printk ("======================================================\n" );printk ("[ INFO: %s-safe -> %s-unsafe lock order detected ]\n" , irqclass, irqclass); -print_kernel_version (); +print_kernel_ident ();printk ("------------------------------------------------------\n" ); printk("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n", curr->comm, task_pid_nr(curr),
@@ -1716,7 +1717,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,printk ("\n" );printk ("=============================================\n" );printk ("[ INFO: possible recursive locking detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------------------\n" );printk ("%s/%d is trying to acquire lock:\n" , curr->comm,task_pid_nr (curr));
@@ -2223,7 +2224,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,printk ("\n" );printk ("=================================\n" );printk ("[ INFO: inconsistent lock state ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------\n" ); printk("inconsistent {%s} -> {%s} usage.\n",
@@ -2288,7 +2289,7 @@ print_irq_inversion_bug(struct task_struct *curr,printk ("\n" );printk ("=========================================================\n" );printk ("[ INFO: possible irq lock inversion dependency detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------------------------------\n" );printk ("%s/%d just changed the state of lock:\n" , curr->comm,task_pid_nr (curr));
@@ -3169,6 +3170,7 @@ print_unlock_inbalance_bug(struct task_struct *curr, struct lockdep_map *lock,printk ("\n" );printk ("=====================================\n" );printk ("[ BUG: bad unlock balance detected! ]\n" ); +print_kernel_ident ();printk ("-------------------------------------\n" );printk ("%s/%d is trying to release lock (" , curr->comm,task_pid_nr (curr));
@@ -3613,6 +3615,7 @@ print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,printk ("\n" );printk ("=================================\n" );printk ("[ BUG: bad contention detected! ]\n" ); +print_kernel_ident ();printk ("---------------------------------\n" );printk ("%s/%d is trying to contend lock (" , curr->comm,task_pid_nr (curr));
@@ -3987,6 +3990,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,printk ("\n" );printk ("=========================\n" );printk ("[ BUG: held lock freed! ]\n" ); +print_kernel_ident ();printk ("-------------------------\n" );printk ("%s/%d is freeing memory %p-%p, with a lock still held there!\n" , curr->comm,task_pid_nr (curr), mem_from, mem_to-1 );
@@ -4044,6 +4048,7 @@ static void print_held_locks_bug(struct task_struct *curr)printk ("\n" );printk ("=====================================\n" );printk ("[ BUG: lock held at task exit time! ]\n" ); +print_kernel_ident ();printk ("-------------------------------------\n" );printk ("%s/%d is exiting with locks still held!\n" , curr->comm,task_pid_nr (curr));
@@ -4141,6 +4146,7 @@ void lockdep_sys_exit(void)printk ("\n" );printk ("================================================\n" );printk ("[ BUG: lock held when returning to user space! ]\n" ); +print_kernel_ident ();printk ("------------------------------------------------\n" );printk ("%s/%d is leaving the kernel with locks still held!\n" , curr->comm, curr->pid);
@@ -4160,6 +4166,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)printk ("\n" );printk ("===============================\n" );printk ("[ INFO: suspicious RCU usage. ]\n" ); +print_kernel_ident ();printk ("-------------------------------\n" );printk ("%s:%d %s!\n" , file, line, s);printk ("\nother info that might help us debug this:\n\n" );
kernel/panic.c +0 -10
@@ -233,16 +233,6 @@ unsigned long get_taint(void)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 (flag !=TAINT_CRAP && flag !=TAINT_WARN &&__debug_locks_off ()) -printk (KERN_WARNING "Disabling lock debugging due to kernel taint\n" ); -set_bit (flag, &tainted_mask); }EXPORT_SYMBOL (add_taint);
kernel/rtmutex-debug.c +1 -0
@@ -101,6 +101,7 @@ void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter)printk ("\n============================================\n" );printk ("[ BUG: circular locking deadlock detected! ]\n" ); +printk ("%s\n" ,print_tainted ());printk ("--------------------------------------------\n" ); printk("%s/%d is deadlocking current task %s/%d\n\n", task->comm, task_pid_nr(task),
Signature · 1 line
1.7.7
Quoted 8 lines · Ben Hutchings
Currently lock debugging is disabled when the kernel is tainted, with a few exceptions. It is already recognised that this can be useful for staging modules (
TAINT_CRAP), but that also goes for out-of-tree modules (TAINT_OOT_MODULE) so long as core kernel developers don't have to spend time debugging them. Also, there are several reasons for tainting that are unlikely to introduce false locking bug reports (e.g.TAINT_FIRMWARE_WORKAROUND).
Quoted 4 lines
Instead of disabling lock debugging, show the taint flags in all lockdep and rtmutex-debug error messages.
Signed-off-by Ben Hutchings <[email protected]>
Please could you ack or nak this?
Ben.
Quoted 169 lines · Ben Hutchings
Currently lock debugging is disabled when the kernel is tainted, with a few exceptions. It is already recognised that this can be useful for staging modules (
TAINT_CRAP), but that also goes for out-of-tree modules (TAINT_OOT_MODULE) so long as core kernel developers don't have to spend time debugging them. Also, there are several reasons for tainting that are unlikely to introduce false locking bug reports (e.g.TAINT_FIRMWARE_WORKAROUND).Instead of disabling lock debugging, show the taint flags in all lockdep and rtmutex-debug error messages.
Signed-off-by Ben Hutchings <[email protected]>--- kernel/lockdep.c | 23 +++++++++++++++-------- kernel/panic.c | 10 ---------- kernel/rtmutex-debug.c | 1 + 3 files changed, 16 insertions(+), 18 deletions(-)kernel/lockdep.c +15
-8@@ -567,11 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr) } } -static void print_kernel_version (void ) +static void print_kernel_ident (void ) { -printk ("%s %.*s\n" ,init_utsname ()->release, +printk ("%s %.*s %s\n" ,init_utsname ()->release, (int )strcspn (init_utsname ()->version," " ), -init_utsname ()->version); +init_utsname ()->version, +print_tainted ()); }static int very_verbose (struct lock_class *class)@@ -1148,7 +1149,7 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,printk ("\n" );printk ("======================================================\n" );printk ("[ INFO: possible circular locking dependency detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("-------------------------------------------------------\n" );printk ("%s/%d is trying to acquire lock:\n" , curr->comm,task_pid_nr (curr));@@ -1487,7 +1488,7 @@ print_bad_irq_dependency(struct task_struct *curr,printk ("======================================================\n" );printk ("[ INFO: %s-safe -> %s-unsafe lock order detected ]\n" , irqclass, irqclass); -print_kernel_version (); +print_kernel_ident ();printk ("------------------------------------------------------\n" ); printk("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n", curr->comm, task_pid_nr(curr),@@ -1716,7 +1717,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,printk ("\n" );printk ("=============================================\n" );printk ("[ INFO: possible recursive locking detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------------------\n" );printk ("%s/%d is trying to acquire lock:\n" , curr->comm,task_pid_nr (curr));@@ -2223,7 +2224,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,printk ("\n" );printk ("=================================\n" );printk ("[ INFO: inconsistent lock state ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------\n" ); printk("inconsistent {%s} -> {%s} usage.\n",@@ -2288,7 +2289,7 @@ print_irq_inversion_bug(struct task_struct *curr,printk ("\n" );printk ("=========================================================\n" );printk ("[ INFO: possible irq lock inversion dependency detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------------------------------\n" );printk ("%s/%d just changed the state of lock:\n" , curr->comm,task_pid_nr (curr));@@ -3169,6 +3170,7 @@ print_unlock_inbalance_bug(struct task_struct *curr, struct lockdep_map *lock,printk ("\n" );printk ("=====================================\n" );printk ("[ BUG: bad unlock balance detected! ]\n" ); +print_kernel_ident ();printk ("-------------------------------------\n" );printk ("%s/%d is trying to release lock (" , curr->comm,task_pid_nr (curr));@@ -3613,6 +3615,7 @@ print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,printk ("\n" );printk ("=================================\n" );printk ("[ BUG: bad contention detected! ]\n" ); +print_kernel_ident ();printk ("---------------------------------\n" );printk ("%s/%d is trying to contend lock (" , curr->comm,task_pid_nr (curr));@@ -3987,6 +3990,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,printk ("\n" );printk ("=========================\n" );printk ("[ BUG: held lock freed! ]\n" ); +print_kernel_ident ();printk ("-------------------------\n" );printk ("%s/%d is freeing memory %p-%p, with a lock still held there!\n" , curr->comm,task_pid_nr (curr), mem_from, mem_to-1 );@@ -4044,6 +4048,7 @@ static void print_held_locks_bug(struct task_struct *curr)printk ("\n" );printk ("=====================================\n" );printk ("[ BUG: lock held at task exit time! ]\n" ); +print_kernel_ident ();printk ("-------------------------------------\n" );printk ("%s/%d is exiting with locks still held!\n" , curr->comm,task_pid_nr (curr));@@ -4141,6 +4146,7 @@ void lockdep_sys_exit(void)printk ("\n" );printk ("================================================\n" );printk ("[ BUG: lock held when returning to user space! ]\n" ); +print_kernel_ident ();printk ("------------------------------------------------\n" );printk ("%s/%d is leaving the kernel with locks still held!\n" , curr->comm, curr->pid);@@ -4160,6 +4166,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)printk ("\n" );printk ("===============================\n" );printk ("[ INFO: suspicious RCU usage. ]\n" ); +print_kernel_ident ();printk ("-------------------------------\n" );printk ("%s:%d %s!\n" , file, line, s);printk ("\nother info that might help us debug this:\n\n" );kernel/panic.c +0
-10@@ -233,16 +233,6 @@ unsigned long get_taint(void)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 (flag !=TAINT_CRAP && flag !=TAINT_WARN &&__debug_locks_off ()) -printk (KERN_WARNING "Disabling lock debugging due to kernel taint\n" ); -set_bit (flag, &tainted_mask); }EXPORT_SYMBOL (add_taint);kernel/rtmutex-debug.c +1
-0@@ -101,6 +101,7 @@ void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter)printk ("\n============================================\n" );printk ("[ BUG: circular locking deadlock detected! ]\n" ); +printk ("%s\n" ,print_tainted ());printk ("--------------------------------------------\n" ); printk("%s/%d is deadlocking current task %s/%d\n\n", task->comm, task_pid_nr(task),Signature · 1 line
1.7.7
Signature · 3 lines
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert CamusDave Jones
Reviewed-byQuoted 3 lines · Ben Hutchings
Please could you ack or nak this?
Ben.
sorry, thought I already had.
Quoted 11 lines · Ben Hutchings
Quoted 7 lines · Ben Hutchings
Currently lock debugging is disabled when the kernel is tainted, with a few exceptions. It is already recognised that this can be useful for staging modules (
TAINT_CRAP), but that also goes for out-of-tree modules (TAINT_OOT_MODULE) so long as core kernel developers don't have to spend time debugging them. Also, there are several reasons for tainting that are unlikely to introduce false locking bug reports (e.g.TAINT_FIRMWARE_WORKAROUND).
Quoted 2 lines
Quoted 2 lines
Instead of disabling lock debugging, show the taint flags in all lockdep and rtmutex-debug error messages.
So this is two patches in one. I took the last part, the printing of the taint flags thing, not the first part.
I did a small patch adding TAINT_FIRMWARE_WORKAROUND to the list of TAINTS that shouldn't disable lockdep.
As for OOT_MODULE, with staging the only reason not to merge a module is it being the wrong license and I really can't be arsed about OOT stuff anyway, so no.
Quoted 19 lines · Peter Zijlstra
Quoted 11 lines · Ben Hutchings
Quoted 7 lines · Ben Hutchings
Currently lock debugging is disabled when the kernel is tainted, with a few exceptions. It is already recognised that this can be useful for staging modules (
TAINT_CRAP), but that also goes for out-of-tree modules (TAINT_OOT_MODULE) so long as core kernel developers don't have to spend time debugging them. Also, there are several reasons for tainting that are unlikely to introduce false locking bug reports (e.g.TAINT_FIRMWARE_WORKAROUND).Quoted 2 lines
Quoted 2 lines
Instead of disabling lock debugging, show the taint flags in all lockdep and rtmutex-debug error messages.
So this is two patches in one. I took the last part, the printing of the taint flags thing, not the first part.
Quoted 6 lines
I did a small patch adding
TAINT_FIRMWARE_WORKAROUNDto the list of TAINTS that shouldn't disable lockdep.As for
OOT_MODULE, with staging the only reason not to merge a module is it being the wrong license and I really can't be arsed about OOT stuff anyway, so no.
That's not true. For example, VirtualBox OSE is GPL but would not be accepted into staging. But if VirtualBox developers want to use lockdep to fix their crap, we shouldn't stop them.
Ben.
Signature · 2 lines
Ben Hutchings The world is coming to an end. Please log off.
[tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
In reply to Ben HutchingsCommit-ID: fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Gitweb: http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Author: Ben Hutchings <[email protected]> AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 6 Dec 2011 08:16:49 +0100
lockdep, rtmutex, bug: Show taint flags on error
Show the taint flags in all lockdep and rtmutex-debug error messages.
--- kernel/lockdep.c | 23 +++++++++++++++-------- kernel/rtmutex-debug.c | 1 + 2 files changed, 16 insertions(+), 8 deletions(-)
kernel/lockdep.c +15 -8
@@ -567,11 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr) } } -static void print_kernel_version (void ) +static void print_kernel_ident (void ) { -printk ("%s %.*s\n" ,init_utsname ()->release, +printk ("%s %.*s %s\n" ,init_utsname ()->release, (int )strcspn (init_utsname ()->version," " ), -init_utsname ()->version); +init_utsname ()->version, +print_tainted ()); }static int very_verbose (struct lock_class *class)
@@ -1149,7 +1150,7 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,printk ("\n" );printk ("======================================================\n" );printk ("[ INFO: possible circular locking dependency detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("-------------------------------------------------------\n" );printk ("%s/%d is trying to acquire lock:\n" , curr->comm,task_pid_nr (curr));
@@ -1488,7 +1489,7 @@ print_bad_irq_dependency(struct task_struct *curr,printk ("======================================================\n" );printk ("[ INFO: %s-safe -> %s-unsafe lock order detected ]\n" , irqclass, irqclass); -print_kernel_version (); +print_kernel_ident ();printk ("------------------------------------------------------\n" ); printk("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n", curr->comm, task_pid_nr(curr),
@@ -1717,7 +1718,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,printk ("\n" );printk ("=============================================\n" );printk ("[ INFO: possible recursive locking detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------------------\n" );printk ("%s/%d is trying to acquire lock:\n" , curr->comm,task_pid_nr (curr));
@@ -2224,7 +2225,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,printk ("\n" );printk ("=================================\n" );printk ("[ INFO: inconsistent lock state ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------\n" ); printk("inconsistent {%s} -> {%s} usage.\n",
@@ -2289,7 +2290,7 @@ print_irq_inversion_bug(struct task_struct *curr,printk ("\n" );printk ("=========================================================\n" );printk ("[ INFO: possible irq lock inversion dependency detected ]\n" ); -print_kernel_version (); +print_kernel_ident ();printk ("---------------------------------------------------------\n" );printk ("%s/%d just changed the state of lock:\n" , curr->comm,task_pid_nr (curr));
@@ -3170,6 +3171,7 @@ print_unlock_inbalance_bug(struct task_struct *curr, struct lockdep_map *lock,printk ("\n" );printk ("=====================================\n" );printk ("[ BUG: bad unlock balance detected! ]\n" ); +print_kernel_ident ();printk ("-------------------------------------\n" );printk ("%s/%d is trying to release lock (" , curr->comm,task_pid_nr (curr));
@@ -3614,6 +3616,7 @@ print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,printk ("\n" );printk ("=================================\n" );printk ("[ BUG: bad contention detected! ]\n" ); +print_kernel_ident ();printk ("---------------------------------\n" );printk ("%s/%d is trying to contend lock (" , curr->comm,task_pid_nr (curr));
@@ -3988,6 +3991,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,printk ("\n" );printk ("=========================\n" );printk ("[ BUG: held lock freed! ]\n" ); +print_kernel_ident ();printk ("-------------------------\n" );printk ("%s/%d is freeing memory %p-%p, with a lock still held there!\n" , curr->comm,task_pid_nr (curr), mem_from, mem_to-1 );
@@ -4045,6 +4049,7 @@ static void print_held_locks_bug(struct task_struct *curr)printk ("\n" );printk ("=====================================\n" );printk ("[ BUG: lock held at task exit time! ]\n" ); +print_kernel_ident ();printk ("-------------------------------------\n" );printk ("%s/%d is exiting with locks still held!\n" , curr->comm,task_pid_nr (curr));
@@ -4142,6 +4147,7 @@ void lockdep_sys_exit(void)printk ("\n" );printk ("================================================\n" );printk ("[ BUG: lock held when returning to user space! ]\n" ); +print_kernel_ident ();printk ("------------------------------------------------\n" );printk ("%s/%d is leaving the kernel with locks still held!\n" , curr->comm, curr->pid);
@@ -4161,6 +4167,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)printk ("\n" );printk ("===============================\n" );printk ("[ INFO: suspicious RCU usage. ]\n" ); +print_kernel_ident ();printk ("-------------------------------\n" );printk ("%s:%d %s!\n" , file, line, s);printk ("\nother info that might help us debug this:\n\n" );
kernel/rtmutex-debug.c +1 -0
@@ -101,6 +101,7 @@ void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter)printk ("\n============================================\n" );printk ("[ BUG: circular locking deadlock detected! ]\n" ); +printk ("%s\n" ,print_tainted ());printk ("--------------------------------------------\n" ); printk("%s/%d is deadlocking current task %s/%d\n\n", task->comm, task_pid_nr(task),
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 7 lines · tip-bot for Ben Hutchings
Commit-ID: fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Gitweb: http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Author: Ben Hutchings <[email protected]> AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 6 Dec 2011 08:16:49 +0100
Quoted 8 lines
lockdep, rtmutex, bug: Show taint flags on error
Show the taint flags in all lockdep and rtmutex-debug error messages.
Signed-off-by Ben Hutchings <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Signed-off-by Ingo Molnar <[email protected]>
[...]
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
Ben.
Signature · 2 lines
Ben Hutchings Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 3 lines · Ben Hutchings
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
The easy solution is that I never take patches from you again, ever. Consider that done. I'll let Ingo see if he can remove your SOB.
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 7 lines · On Tue, 06 Dec 2011 18:48:51 +0100 Peter Zijlstra <[email protected]>…
Quoted 3 lines · Ben Hutchings
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
The easy solution is that I never take patches from you again, ever. Consider that done. I'll let Ingo see if he can remove your SOB.
In which case you are presumably ceasing to be a maintainer for that code ? Your statement above appears to be inconsistent with the rôle of a maintainer.
Alan
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 8 lines · Alan Cox
Quoted 5 lines · On Tue, 06 Dec 2011 18:48:51 +0100 Peter Zijlstra <[email protected]>…
Quoted 3 lines · Ben Hutchings
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
Quoted 6 lines
Quoted 2 lines
The easy solution is that I never take patches from you again, ever. Consider that done. I'll let Ingo see if he can remove your SOB.
In which case you are presumably ceasing to be a maintainer for that code ? Your statement above appears to be inconsistent with the rôle of a maintainer.
There's plenty people who have various other people blacklisted in the procmail. My blacklist is shorter than some, Merkey isn't actually on it for instance.
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
In reply to Alan CoxQuoted 9 lines · Alan Cox
Quoted 6 lines · On Tue, 06 Dec 2011 18:48:51 +0100 Peter Zijlstra <[email protected]>…
Quoted 3 lines · Ben Hutchings
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
Quoted 7 lines
Quoted 3 lines
The easy solution is that I never take patches from you again, ever. Consider that done. I'll let Ingo see if he can remove your SOB.
In which case you are presumably ceasing to be a maintainer for that code ? Your statement above appears to be inconsistent with the rôle of a maintainer.
What Peter did was rather sensible: he split a patch that did two things into two and applied one standalone, uncontroversial half of it and kept the part of the part of the changelog that related to that change.
What Peter probably could have done is to add one more line before his SOB:
Otherwise Ben Hutchings's objection here makes little sense.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 16 lines · Ingo Molnar
Quoted 13 lines · Alan Cox
Quoted 9 lines · On Tue, 06 Dec 2011 18:48:51 +0100 Peter Zijlstra <[email protected]>…
Quoted 3 lines · Ben Hutchings
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
The easy solution is that I never take patches from you again, ever. Consider that done. I'll let Ingo see if he can remove your SOB.
Quoted 8 lines
Quoted 3 lines
In which case you are presumably ceasing to be a maintainer for that code ? Your statement above appears to be inconsistent with the rôle of a maintainer.
What Peter did was rather sensible: he split a patch that did two things into two and applied one standalone, uncontroversial half of it and kept the part of the part of the changelog that related to that change.
[...]
I can accept that if the change from my submission is commented in the commit message. (I sometimes do that with other people's patches.) But this was modified without comment.
Giving credit for authorship is important, but it is also important not to say someone is the sole author of something when they aren't and haven't approved the subsequent changes (cf. Alan Smithee).
Ben.
Signature · 3 lines
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert CamusRe: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 10 lines · Ben Hutchings
Quoted 4 lines
What Peter did was rather sensible: he split a patch that did two things into two and applied one standalone, uncontroversial half of it and kept the part of the part of the changelog that related to that change.
[...]
I can accept that if the change from my submission is commented in the commit message. (I sometimes do that with other people's patches.) But this was modified without comment.
That was not what you requested though:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
What happened is that you did two things in a single patch and Peter applied the uncontroversial, unrelated bits in full.
Quoted 4 lines
Giving credit for authorship is important, but it is also important not to say someone is the sole author of something when they aren't and haven't approved the subsequent changes (cf. Alan Smithee).
It's all hunks submitted by you, you are the only author.
He did not modify your changes - you submitted several changes.
You are actually the sole author of those changes in the commit. Peter only split up your original patch - he was actually being very constructive and 100% helpful with upstreaming your debug printout changes - which makes your original objection doubly offensive.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Anyway, SOB flaming aside, i think there's now consensus that we don't want to disable lockdep for TAINT_OOT modules.
Mind (re-)sending a delta patch against tip:master that excludes that taint flag in addition to the already excluded TAINT_FIRMWARE case? That should close this rather verbose chapter of taint flag handling changes for good ;-)
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 3 lines · Ingo Molnar
Anyway, SOB flaming aside, i think there's now consensus that we don't want to disable lockdep for
TAINT_OOTmodules.
OK, glad we could work this out.
Quoted 4 lines
Mind (re-)sending a delta patch against tip:master that excludes that taint flag in addition to the already excluded
TAINT_FIRMWAREcase? That should close this rather verbose chapter of taint flag handling changes for good ;-)
I originally assumed that lock debugging was being disabled on a tainted kernel in order to save kernel developers from having to handle false lockdep errors that are due to external bugs. The intent of my proposed change was to achieve this same effect by showing taint flags instead. Although the immediate motivation for this was the new TAINT_OOT_MODULE, I don't see that it is important for any taint flag to have this disabling effect now.
If you want to ensure that lock debugging is not available to proprietary modules, I don't have a problem with that, although there is no legal or practical way for us to stop proprietary module developers evading this check and doing lock debugging in the privacy of their offices.
So what I would like to do is either:
1. No longer disable lock debugging due to any taint
or
2. Disable lock debugging only when TAINT_PROPRIETARY_MODULE is set
Ben.
Signature · 3 lines
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert CamusRe: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 5 lines · Ben Hutchings
So what I would like to do is either:
1. No longer disable lock debugging due to any taint
or
2. Disable lock debugging only whenTAINT_PROPRIETARY_MODULEis set
I'd like a whitelist - i.e. disable on taint by default, but allow case by case exceptions such as OOT_MODULE.
Nor am i singling out bin-only modules, look at the current set of taint flags:
#define TAINT_PROPRIETARY_MODULE 0 #define TAINT_FORCED_MODULE 1 #define TAINT_UNSAFE_SMP 2 #define TAINT_FORCED_RMMOD 3 #define TAINT_MACHINE_CHECK 4 #define TAINT_BAD_PAGE 5 #define TAINT_USER 6 #define TAINT_DIE 7 #define TAINT_OVERRIDDEN_ACPI_TABLE 8 #define TAINT_WARN 9 #define TAINT_CRAP 10 #define TAINT_FIRMWARE_WORKAROUND 11 #define TAINT_OOT_MODULE 12
*all* except TAINT_FIRMWARE_WORKAROUND, TAINT_CRAP and OOT should result in lockdep disabling. (perhaps OVERRIDDEN_ACPI_TABLE, as a variant of FIRMWARE_WORKAROUND, could be added as an exception too)
Module forcing causes various lifetime issues and lockdep tracks lifetime so can get confused on that. Unsafe SMP - enough said. Lockdep also wants to disable on the first WARN_ON() - which is typically the sign of some initial badness, which should be investigated before any lockdep splat is looked into.
Since most of the taint flags show deep kernel or hardware troubles i'd like most new taint flags to be lockdep off by default. As new taint flags are introduced they can be added to the whitelist when justified.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
In reply to Alan CoxQuoted 8 lines · Alan Cox
Quoted 5 lines · On Tue, 06 Dec 2011 18:48:51 +0100 Peter Zijlstra <[email protected]>…
Quoted 3 lines · Ben Hutchings
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
Quoted 6 lines
Quoted 2 lines
The easy solution is that I never take patches from you again, ever. Consider that done. I'll let Ingo see if he can remove your SOB.
In which case you are presumably ceasing to be a maintainer for that code ? Your statement above appears to be inconsistent with the rôle of a maintainer.
Just for the record, Peter is co-maintaining the lockdep code with me (and is doing a superb job with that) and as such he has no obligation whatsoever to interact with every contributor - as long as at least one maintainer considers every patch submitted.
Personally, reading various past discussions i can understand Peter not wanting to work with Ben Hutchings for some time - i'll look at his future patches and will consider them.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
In reply to Ben HutchingsQuoted 16 lines · Ben Hutchings
Quoted 7 lines · tip-bot for Ben Hutchings
Commit-ID: fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Gitweb: http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Author: Ben Hutchings <[email protected]> AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 6 Dec 2011 08:16:49 +0100Quoted 8 lines
lockdep, rtmutex, bug: Show taint flags on error
Show the taint flags in all lockdep and rtmutex-debug error messages.
Signed-off-by Ben Hutchings <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Signed-off-by Ingo Molnar <[email protected]>
Quoted 5 lines
[...]
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
FYI, there's no "I will only sign off on a patch doing two things if it's applied in full" kind of condition in the SOB definition, allowing that would break the GPL: people have the right to take your modifications to the GPL-ed kernel and modify it further.
Your original patch did two things. Peter did the sensible
thing: he split out the print_kernel_ident() changes from your patch which stand on their own and kept your authorship in place
- that is what the above patch does.
Once you send it out the SOB is valid and people can (and typically will) modify it - and if you are still the main author (which you are here 100%) then keeping you as the author is the proper approach - Peter added his own SOB after yours.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 23 lines · Ingo Molnar
Quoted 16 lines · Ben Hutchings
Quoted 7 lines · tip-bot for Ben Hutchings
Commit-ID: fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Gitweb: http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013 Author: Ben Hutchings <[email protected]> AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 6 Dec 2011 08:16:49 +0100Quoted 8 lines
lockdep, rtmutex, bug: Show taint flags on error
Show the taint flags in all lockdep and rtmutex-debug error messages.
Signed-off-by Ben Hutchings <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Signed-off-by Ingo Molnar <[email protected]>Quoted 5 lines
[...]
If you disagree with a patch, do not silently drop parts of it. I demand that you remove my 'Signed-off-by' as this is not the change I submitted.
Quoted 5 lines
FYI, there's no "I will only sign off on a patch doing two things if it's applied in full" kind of condition in the SOB definition, allowing that would break the GPL: people have the right to take your modifications to the GPL-ed kernel and modify it further.
If you want to play the GPL card, you will note that the GPL requires modified versions of a work to carry prominent notices that they have been modified, including the date of any change.
A signoff by a maintainer does not imply that any modifications have been made, so I don't think that counts as "prominent".
Quoted 4 lines
Your original patch did two things. Peter did the sensible
thing: he split out theprint_kernel_ident()changes from your patch which stand on their own and kept your authorship in place
- that is what the above patch does.
In which case, the changelog should have been amended to state that it's a modification of Ben's original submission.
Quoted 4 lines
Once you send it out the SOB is valid and people can (and typically will) modify it - and if you are still the main author (which you are here 100%) then keeping you as the author is the proper approach - Peter added his own SOB after yours.
Linux convention is to add a signoff *and* a description of any changes to the original submission in square brackets.
Cheers,
Signature · 1 line
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 5 lines
FYI, there's no "I will only sign off on a patch doing two things if it's applied in full" kind of condition in the SOB definition, allowing that would break the GPL: people have the right to take your modifications to the GPL-ed kernel and modify it further.
That gets into moral rights, and at least in the EU representing someone as the author of something they are not isn't a good idea even before you get to any contractual or licensing questions. I don't believe the EU is unusual in this point either.
Alan
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
Quoted 9 lines · Alan Cox
Quoted 5 lines
FYI, there's no "I will only sign off on a patch doing two things if it's applied in full" kind of condition in the SOB definition, allowing that would break the GPL: people have the right to take your modifications to the GPL-ed kernel and modify it further.
That gets into moral rights, and at least in the EU representing someone as the author of something they are not [..]
Alan, stop being silly - you clearly don't know what you are talking about.
The fact is, *every single line of code* in that commit was written by Ben Hutchinson. Peter was being excessively helpful, polite and did Ben a favor by splitting out the debug-printks from the original submission and upstreaming them.
What he did not do was to apply an unrelated, still under discussion change mixed into that debug-printouts patch.
Look at the original submission by Ben on lkml:
Subject: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
The hunk that Peter left out of that patch was not declared in the patch title and was unrelated to the rest of the patch:
kernel/panic.c +0 -10
@@ -233,16 +233,6 @@ unsigned long get_taint(void)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 (flag !=TAINT_CRAP && flag !=TAINT_WARN &&__debug_locks_off ()) -printk (KERN_WARNING "Disabling lock debugging due to kernel taint\n" ); -set_bit (flag, &tainted_mask);
Criticising Peter for not applying that change which the patch title did not even mention and then sulkingly asking the SOB to be removed is silly and offensive square two.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
In reply to Nick BowlerQuoted 7 lines · Nick Bowler
Quoted 4 lines
Your original patch did two things. Peter did the sensible
thing: he split out theprint_kernel_ident()changes from your patch which stand on their own and kept your authorship in place - that is what the above patch does.In which case, the changelog should have been amended to state that it's a modification of Ben's original submission.
Which is what i said in my reply to Alan:
| What Peter probably could have done is to add one more line | before his SOB: | | [ split out the patch from the original submission ] | Signed-off-by: Peter Zijlstra <[email protected]> | | Otherwise Ben Hutchings's objection here makes little sense.
No line of code was added by Peter - it's all Ben's changes.
Note that the commit in question:
fbdc4b9a6c29: lockdep, rtmutex, bug: Show taint flags on error
is actually a good one and i think even Ben actually thinks those changes are good. Nothing was added - Ben only wants *more* to be done in a single patch and is being silly about the SOB and is asking it to be removed.
Thanks,
Ingo
Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
In reply to Nick BowlerQuoted 1 line · Nick Bowler
On 2011-12-06 18:54 +0100, Ingo Molnar wrote:
[...]
Quoted 7 lines
Quoted 4 lines
Once you send it out the SOB is valid and people can (and typically will) modify it - and if you are still the main author (which you are here 100%) then keeping you as the author is the proper approach - Peter added his own SOB after yours.
Linux convention is to add a signoff *and* a description of any changes to the original submission in square brackets.
Yes, I would also be satisfied with that. (So my 'demand' was a bit overstated. Sorry about that, but *quietly* changing my code is like misquoing me and it does get me pretty angry.)
Ben.
Signature · 3 lines
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus