Messages in this thread Patch in this message | | | Date | Tue, 16 Aug 2016 17:53:54 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 4/5] bug: Provide toggle for BUG on data corruption |
| |
On Tue, 16 Aug 2016 14:42:28 -0700 Kees Cook <keescook@chromium.org> wrote:
> > OK, I will bite... Why both the WARN() and the BUG_ON()? > > Mostly because not every case of BUG(CORRUPTED_DATA_STRUCTURE) is > cleanly paired with a WARN (see the workqueue addition that wants to > dump locks too). I could rearrange things a bit, though, and create > something like: > > #ifdef CONFIG_BUG_ON_CORRUPTION > # define CORRUPTED(format...) { \ > printk(KERN_ERR format); \ > BUG(); \ > } > #else > # define CORRUPTED(format...) WARN(1, format) > #endif > > What do you think?
I prefer what you currently have.
WARN(1, "list_del corruption. next->prev should be %p, but was %p\n", entry, next->prev); BUG_ON(CORRUPTED_DATA_STRUCTURE);
Will always warn (as stated by "1") and and the BUG_ON() will bug if CORRUPTED_DATA_STRUCTURE is set. Although, I don't like that name. Can we have a:
BUG_ON(BUG_ON_CORRUPTED_DATA_STRUCTURES);
Or maybe have that as a macro:
#ifdef CONFIG_BUG_ON_CORRUPTION # define BUG_ON_CORRUPTED_DATA_STRUCTURE() BUG_ON(1) #else # define BUG_ON_CORRUPTED_DATA_STRUCTURE() do {} while (0) #endif
Then we can have:
WARN(1, "list_del corruption. next->prev should be %p, but was %p\n", entry, next->prev); BUG_ON_CORRUPTED_DATA_STRUCTURE();
??
-- Steve
> > -Kees > > > > > Thanx, Paul > > > >> --- > >> include/linux/bug.h | 7 +++++++ > >> kernel/locking/spinlock_debug.c | 1 + > >> kernel/workqueue.c | 2 ++ > >> lib/Kconfig.debug | 10 ++++++++++ > >> lib/list_debug.c | 7 +++++++ > >> 5 files changed, 27 insertions(+) > >> > >> diff --git a/include/linux/bug.h b/include/linux/bug.h > >> index e51b0709e78d..7e69758dd798 100644 > >> --- a/include/linux/bug.h > >> +++ b/include/linux/bug.h > >> @@ -118,4 +118,11 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr, > >> } > >> > >> #endif /* CONFIG_GENERIC_BUG */ > >> + > >> +#ifdef CONFIG_BUG_ON_CORRUPTION > >> +# define CORRUPTED_DATA_STRUCTURE true > >> +#else > >> +# define CORRUPTED_DATA_STRUCTURE false > >> +#endif > >> + > >> #endif /* _LINUX_BUG_H */ > >> diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c > >> index 0374a596cffa..d5f833769feb 100644 > >> --- a/kernel/locking/spinlock_debug.c > >> +++ b/kernel/locking/spinlock_debug.c > >> @@ -64,6 +64,7 @@ static void spin_dump(raw_spinlock_t *lock, const char *msg) > >> owner ? owner->comm : "<none>", > >> owner ? task_pid_nr(owner) : -1, > >> lock->owner_cpu); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> dump_stack(); > >> } > >> > >> diff --git a/kernel/workqueue.c b/kernel/workqueue.c > >> index ef071ca73fc3..ea0132b55eca 100644 > >> --- a/kernel/workqueue.c > >> +++ b/kernel/workqueue.c > >> @@ -48,6 +48,7 @@ > >> #include <linux/nodemask.h> > >> #include <linux/moduleparam.h> > >> #include <linux/uaccess.h> > >> +#include <linux/bug.h> > >> > >> #include "workqueue_internal.h" > >> > >> @@ -2108,6 +2109,7 @@ __acquires(&pool->lock) > >> current->comm, preempt_count(), task_pid_nr(current), > >> worker->current_func); > >> debug_show_held_locks(current); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> dump_stack(); > >> } > >> > >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > >> index 2307d7c89dac..d64bd6b6fd45 100644 > >> --- a/lib/Kconfig.debug > >> +++ b/lib/Kconfig.debug > >> @@ -1987,6 +1987,16 @@ config TEST_STATIC_KEYS > >> > >> If unsure, say N. > >> > >> +config BUG_ON_CORRUPTION > >> + bool "Trigger a BUG when data corruption is detected" > >> + help > >> + Select this option if the kernel should BUG when it encounters > >> + data corruption in various kernel memory structures during checks > >> + added by options like CONFIG_DEBUG_LIST, CONFIG_DEBUG_SPINLOCK, > >> + etc. > >> + > >> + If unsure, say N. > >> + > >> source "samples/Kconfig" > >> > >> source "lib/Kconfig.kgdb" > >> diff --git a/lib/list_debug.c b/lib/list_debug.c > >> index 80e2e40a6a4e..161c7e7d3478 100644 > >> --- a/lib/list_debug.c > >> +++ b/lib/list_debug.c > >> @@ -26,16 +26,19 @@ bool __list_add_debug(struct list_head *new, > >> if (unlikely(next->prev != prev)) { > >> WARN(1, "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", > >> prev, next->prev, next); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> if (unlikely(prev->next != next)) { > >> WARN(1, "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", > >> next, prev->next, prev); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> if (unlikely(new == prev || new == next)) { > >> WARN(1, "list_add double add: new=%p, prev=%p, next=%p.\n", > >> new, prev, next); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> return true; > >> @@ -52,21 +55,25 @@ bool __list_del_entry_debug(struct list_head *entry) > >> if (unlikely(next == LIST_POISON1)) { > >> WARN(1, "list_del corruption, %p->next is LIST_POISON1 (%p)\n", > >> entry, LIST_POISON1); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> if (unlikely(prev == LIST_POISON2)) { > >> WARN(1, "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", > >> entry, LIST_POISON2); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> if (unlikely(prev->next != entry)) { > >> WARN(1, "list_del corruption. prev->next should be %p, but was %p\n", > >> entry, prev->next); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> if (unlikely(next->prev != entry)) { > >> WARN(1, "list_del corruption. next->prev should be %p, but was %p\n", > >> entry, next->prev); > >> + BUG_ON(CORRUPTED_DATA_STRUCTURE); > >> return false; > >> } > >> return true; > >> -- > >> 2.7.4 > >> > > > > >
| |