Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
On Tue, Aug 20, 2019 at 09:14:29PM -0400, Steven Rostedt wrote: > On Tue, 20 Aug 2019 12:58:49 +0200 > Christophe Leroy wrote: > > > >> index 1077366f496b..6c22e8a6f9de 100644 > > >> --- a/lib/bug.c > > >> +++ b/lib/bug.c > > >> @@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long > > >> bugaddr, struct pt_regs *regs) > > >> } > > >> } > > >> > > >> +/* > > >> + * BUG() and WARN_ON() families don't print a custom debug > > >> message > > >> + * before triggering the exception handler, so we must add the > > >> + * "cut here" line now. WARN() issues its own "cut here" before > > >> the > > >> + * extra debugging message it writes before triggering the > > >> handler. > > >> + */ > > >> +if ((bug->flags & BUGFLAG_PRINTK) == 0) > > >> +printk(KERN_DEFAULT CUT_HERE); > > > > > > I'm not loving that BUGFLAG_PRINTK name, BUGFLAG_CUT_HERE makes more > > > sense to me. > > > > > > > Actually it would be BUGFLAG_NO_CUT_HERE then, otherwise all arches not > > using the generic macros will have to add the flag to get the "cut here" > > line. > > > > Perhaps they all should be audited to see if they don't have the same > problem? As far as I could tell, all the other combinations end up either using the slow path bug helpers or the common exception handler. warn-with-a-fmt-string is the only case that does the "early cut here". -- Kees Cook
Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
On Tue, 20 Aug 2019 12:58:49 +0200 Christophe Leroy wrote: > >> index 1077366f496b..6c22e8a6f9de 100644 > >> --- a/lib/bug.c > >> +++ b/lib/bug.c > >> @@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long bugaddr, > >> struct pt_regs *regs) > >>} > >>} > >> > >> + /* > >> + * BUG() and WARN_ON() families don't print a custom debug message > >> + * before triggering the exception handler, so we must add the > >> + * "cut here" line now. WARN() issues its own "cut here" before the > >> + * extra debugging message it writes before triggering the handler. > >> + */ > >> + if ((bug->flags & BUGFLAG_PRINTK) == 0) > >> + printk(KERN_DEFAULT CUT_HERE); > > > > I'm not loving that BUGFLAG_PRINTK name, BUGFLAG_CUT_HERE makes more > > sense to me. > > > > Actually it would be BUGFLAG_NO_CUT_HERE then, otherwise all arches not > using the generic macros will have to add the flag to get the "cut here" > line. > Perhaps they all should be audited to see if they don't have the same problem? -- Steve
Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
On Tue, 20 Aug 2019 09:33:24 -0700 Kees Cook wrote: > > > > diff --git a/lib/bug.c b/lib/bug.c > > > > index 1077366f496b..6c22e8a6f9de 100644 > > > > --- a/lib/bug.c > > > > +++ b/lib/bug.c > > > > @@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long > > > > bugaddr, struct pt_regs *regs) > > > > } > > > > } > > > > + /* > > > > +* BUG() and WARN_ON() families don't print a custom debug > > > > message > > > > +* before triggering the exception handler, so we must add the > > > > +* "cut here" line now. WARN() issues its own "cut here" before > > > > the > > > > +* extra debugging message it writes before triggering the > > > > handler. > > > > +*/ > > > > + if ((bug->flags & BUGFLAG_PRINTK) == 0) > > > > + printk(KERN_DEFAULT CUT_HERE); > > > > > > I'm not loving that BUGFLAG_PRINTK name, BUGFLAG_CUT_HERE makes more > > > sense to me. > > That's fine -- easy rename. :) > > > Actually it would be BUGFLAG_NO_CUT_HERE then, otherwise all arches not > > using the generic macros will have to add the flag to get the "cut here" > > line. > > I am testing for the lack of the flag (so that only the > CONFIG_GENERIC_BUG with __WARN_FLAGS case needs to set it). I was > thinking of the flag to mean "this reporting flow has already issued > cut-here". It sounds like it would be more logical to have it named > BUGFLAG_NO_CUT_HERE to mean "do not issue a cut-here; it has already > happened"? I will update the patch. > BUGFLAG_HAS_CUT_HERE ? As it shows it was already done? -- Steve
Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
On Tue, Aug 20, 2019 at 12:58:49PM +0200, Christophe Leroy wrote: > Le 20/08/2019 à 12:06, Peter Zijlstra a écrit : > > On Mon, Aug 19, 2019 at 04:41:11PM -0700, Kees Cook wrote: > > > > > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h > > > index 588dd59a5b72..da471fcc5487 100644 > > > --- a/include/asm-generic/bug.h > > > +++ b/include/asm-generic/bug.h > > > @@ -10,6 +10,7 @@ > > > #define BUGFLAG_WARNING (1 << 0) > > > #define BUGFLAG_ONCE(1 << 1) > > > #define BUGFLAG_DONE(1 << 2) > > > +#define BUGFLAG_PRINTK (1 << 3) > > > #define BUGFLAG_TAINT(taint)((taint) << 8) > > > #define BUG_GET_TAINT(bug) ((bug)->flags >> 8) > > > #endif > > > > > diff --git a/lib/bug.c b/lib/bug.c > > > index 1077366f496b..6c22e8a6f9de 100644 > > > --- a/lib/bug.c > > > +++ b/lib/bug.c > > > @@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long bugaddr, > > > struct pt_regs *regs) > > > } > > > } > > > + /* > > > + * BUG() and WARN_ON() families don't print a custom debug message > > > + * before triggering the exception handler, so we must add the > > > + * "cut here" line now. WARN() issues its own "cut here" before the > > > + * extra debugging message it writes before triggering the handler. > > > + */ > > > + if ((bug->flags & BUGFLAG_PRINTK) == 0) > > > + printk(KERN_DEFAULT CUT_HERE); > > > > I'm not loving that BUGFLAG_PRINTK name, BUGFLAG_CUT_HERE makes more > > sense to me. That's fine -- easy rename. :) > Actually it would be BUGFLAG_NO_CUT_HERE then, otherwise all arches not > using the generic macros will have to add the flag to get the "cut here" > line. I am testing for the lack of the flag (so that only the CONFIG_GENERIC_BUG with __WARN_FLAGS case needs to set it). I was thinking of the flag to mean "this reporting flow has already issued cut-here". It sounds like it would be more logical to have it named BUGFLAG_NO_CUT_HERE to mean "do not issue a cut-here; it has already happened"? I will update the patch. Thanks! -- Kees Cook
Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
Le 20/08/2019 à 12:06, Peter Zijlstra a écrit : On Mon, Aug 19, 2019 at 04:41:11PM -0700, Kees Cook wrote: diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 588dd59a5b72..da471fcc5487 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -10,6 +10,7 @@ #define BUGFLAG_WARNING (1 << 0) #define BUGFLAG_ONCE (1 << 1) #define BUGFLAG_DONE (1 << 2) +#define BUGFLAG_PRINTK (1 << 3) #define BUGFLAG_TAINT(taint) ((taint) << 8) #define BUG_GET_TAINT(bug)((bug)->flags >> 8) #endif diff --git a/lib/bug.c b/lib/bug.c index 1077366f496b..6c22e8a6f9de 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) } } + /* +* BUG() and WARN_ON() families don't print a custom debug message +* before triggering the exception handler, so we must add the +* "cut here" line now. WARN() issues its own "cut here" before the +* extra debugging message it writes before triggering the handler. +*/ + if ((bug->flags & BUGFLAG_PRINTK) == 0) + printk(KERN_DEFAULT CUT_HERE); I'm not loving that BUGFLAG_PRINTK name, BUGFLAG_CUT_HERE makes more sense to me. Actually it would be BUGFLAG_NO_CUT_HERE then, otherwise all arches not using the generic macros will have to add the flag to get the "cut here" line. Christophe
Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
On Mon, Aug 19, 2019 at 04:41:11PM -0700, Kees Cook wrote: > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h > index 588dd59a5b72..da471fcc5487 100644 > --- a/include/asm-generic/bug.h > +++ b/include/asm-generic/bug.h > @@ -10,6 +10,7 @@ > #define BUGFLAG_WARNING (1 << 0) > #define BUGFLAG_ONCE (1 << 1) > #define BUGFLAG_DONE (1 << 2) > +#define BUGFLAG_PRINTK (1 << 3) > #define BUGFLAG_TAINT(taint) ((taint) << 8) > #define BUG_GET_TAINT(bug) ((bug)->flags >> 8) > #endif > diff --git a/lib/bug.c b/lib/bug.c > index 1077366f496b..6c22e8a6f9de 100644 > --- a/lib/bug.c > +++ b/lib/bug.c > @@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long bugaddr, > struct pt_regs *regs) > } > } > > + /* > + * BUG() and WARN_ON() families don't print a custom debug message > + * before triggering the exception handler, so we must add the > + * "cut here" line now. WARN() issues its own "cut here" before the > + * extra debugging message it writes before triggering the handler. > + */ > + if ((bug->flags & BUGFLAG_PRINTK) == 0) > + printk(KERN_DEFAULT CUT_HERE); I'm not loving that BUGFLAG_PRINTK name, BUGFLAG_CUT_HERE makes more sense to me.
[PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
The original clean up of "cut here" missed the WARN_ON() case (that
does not have a printk message), which was fixed recently by adding
an explicit printk of "cut here". This had the downside of adding a
printk() to every WARN_ON() caller, which reduces the utility of using
an instruction exception to streamline the resulting code. By making
this a new BUGFLAG, all of these can be removed and "cut here" can be
handled by the exception handler.
This was very pronounced on PowerPC, but the effect can be seen on
x86 as well. The resulting text size of a defconfig build shows some
small savings from this patch:
textdata bss dec hex filename
196911675134320 1646664 26472151193eed7 vmlinux.before
196763625134260 1663048 26473670193f4c6 vmlinux.after
This change also opens the door for creating something like BUG_MSG(),
where a custom printk() before issuing BUG(), without confusing the "cut
here" line.
Reported-by: Christophe Leroy
Fixes: Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for
WARN_ON for __WARN_TAINT architectures")
Signed-off-by: Kees Cook
---
include/asm-generic/bug.h | 8 +++-
lib/bug.c | 11 +--
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 588dd59a5b72..da471fcc5487 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -10,6 +10,7 @@
#define BUGFLAG_WARNING(1 << 0)
#define BUGFLAG_ONCE (1 << 1)
#define BUGFLAG_DONE (1 << 2)
+#define BUGFLAG_PRINTK (1 << 3)
#define BUGFLAG_TAINT(taint) ((taint) << 8)
#define BUG_GET_TAINT(bug) ((bug)->flags >> 8)
#endif
@@ -86,13 +87,10 @@ void warn_slowpath_fmt(const char *file, const int line,
unsigned taint,
warn_slowpath_fmt(__FILE__, __LINE__, taint, arg)
#else
extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
-#define __WARN() do { \
- printk(KERN_WARNING CUT_HERE); \
- __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN));\
- } while (0)
+#define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN))
#define __WARN_printf(taint, arg...) do { \
__warn_printk(arg); \
- __WARN_FLAGS(BUGFLAG_TAINT(taint)); \
+ __WARN_FLAGS(BUGFLAG_PRINTK | BUGFLAG_TAINT(taint));\
} while (0)
#define WARN_ON_ONCE(condition) ({ \
int __ret_warn_on = !!(condition); \
diff --git a/lib/bug.c b/lib/bug.c
index 1077366f496b..6c22e8a6f9de 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -181,6 +181,15 @@ enum bug_trap_type report_bug(unsigned long bugaddr,
struct pt_regs *regs)
}
}
+ /*
+* BUG() and WARN_ON() families don't print a custom debug message
+* before triggering the exception handler, so we must add the
+* "cut here" line now. WARN() issues its own "cut here" before the
+* extra debugging message it writes before triggering the handler.
+*/
+ if ((bug->flags & BUGFLAG_PRINTK) == 0)
+ printk(KERN_DEFAULT CUT_HERE);
+
if (warning) {
/* this is a WARN_ON rather than BUG/BUG_ON */
__warn(file, line, (void *)bugaddr, BUG_GET_TAINT(bug), regs,
@@ -188,8 +197,6 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct
pt_regs *regs)
return BUG_TRAP_TYPE_WARN;
}
- printk(KERN_DEFAULT CUT_HERE);
-
if (file)
pr_crit("kernel BUG at %s:%u!\n", file, line);
else
--
2.17.1
