All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] Clean up WARN() "cut here" handling
@ 2019-08-19 23:41 Kees Cook
  2019-08-19 23:41 ` [PATCH 1/7] bug: Refactor away warn_slowpath_fmt_taint() Kees Cook
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

Christophe Leroy noticed that the fix for missing "cut here" in the
WARN() case was adding explicit printk() calls instead of teaching the
exception handler to add it. This refactors the bug/warn infrastructure
to pass this information as a new BUGFLAG.

Longer details repeated from the last patch in the series:


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:

   text    data     bss     dec     hex filename
19691167        5134320 1646664 26472151        193eed7 vmlinux.before
19676362        5134260 1663048 26473670        193f4c6 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 <christophe.leroy@c-s.fr>
Fixes: Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures")
Signed-off-by: Kees Cook <keescook@chromium.org>


-Kees

Kees Cook (7):
  bug: Refactor away warn_slowpath_fmt_taint()
  bug: Rename __WARN_printf_taint() to __WARN_printf()
  bug: Consolidate warn_slowpath_fmt() usage
  bug: Lift "cut here" out of __warn()
  bug: Clean up helper macros to remove __WARN_TAINT()
  bug: Consolidate __WARN_FLAGS usage
  bug: Move WARN_ON() "cut here" into exception handler

 include/asm-generic/bug.h | 53 ++++++++++++++++-----------------------
 kernel/panic.c            | 34 ++++++++-----------------
 lib/bug.c                 | 11 ++++++--
 3 files changed, 40 insertions(+), 58 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/7] bug: Refactor away warn_slowpath_fmt_taint()
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-19 23:41 ` [PATCH 2/7] bug: Rename __WARN_printf_taint() to __WARN_printf() Kees Cook
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

There's no reason to have specialized helpers for passing the warn
taint down to __warn(). Consolidate and refactor helper macros, removing
__WARN_printf() and warn_slowpath_fmt_taint().

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/bug.h | 13 ++++---------
 kernel/panic.c            | 18 +++---------------
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index aa6c093d9ce9..ce466432ac3e 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -90,24 +90,19 @@ struct bug_entry {
  * Use the versions with printk format strings to provide better diagnostics.
  */
 #ifndef __WARN_TAINT
-extern __printf(3, 4)
-void warn_slowpath_fmt(const char *file, const int line,
-		       const char *fmt, ...);
 extern __printf(4, 5)
-void warn_slowpath_fmt_taint(const char *file, const int line, unsigned taint,
-			     const char *fmt, ...);
+void warn_slowpath_fmt(const char *file, const int line, unsigned taint,
+		       const char *fmt, ...);
 extern void warn_slowpath_null(const char *file, const int line);
 #define WANT_WARN_ON_SLOWPATH
 #define __WARN()		warn_slowpath_null(__FILE__, __LINE__)
-#define __WARN_printf(arg...)	warn_slowpath_fmt(__FILE__, __LINE__, arg)
 #define __WARN_printf_taint(taint, arg...)				\
-	warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
+	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_TAINT(TAINT_WARN); \
 } while (0)
-#define __WARN_printf(arg...)	__WARN_printf_taint(TAINT_WARN, arg)
 #define __WARN_printf_taint(taint, arg...)				\
 	do { __warn_printk(arg); __WARN_TAINT(taint); } while (0)
 #endif
@@ -132,7 +127,7 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
-		__WARN_printf(format);					\
+		__WARN_printf_taint(TAINT_WARN, format);		\
 	unlikely(__ret_warn_on);					\
 })
 #endif
diff --git a/kernel/panic.c b/kernel/panic.c
index 057540b6eee9..4d8b7577c82c 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -592,20 +592,8 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
-void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...)
-{
-	struct warn_args args;
-
-	args.fmt = fmt;
-	va_start(args.args, fmt);
-	__warn(file, line, __builtin_return_address(0), TAINT_WARN, NULL,
-	       &args);
-	va_end(args.args);
-}
-EXPORT_SYMBOL(warn_slowpath_fmt);
-
-void warn_slowpath_fmt_taint(const char *file, int line,
-			     unsigned taint, const char *fmt, ...)
+void warn_slowpath_fmt(const char *file, int line, unsigned taint,
+		       const char *fmt, ...)
 {
 	struct warn_args args;
 
@@ -614,7 +602,7 @@ void warn_slowpath_fmt_taint(const char *file, int line,
 	__warn(file, line, __builtin_return_address(0), taint, NULL, &args);
 	va_end(args.args);
 }
-EXPORT_SYMBOL(warn_slowpath_fmt_taint);
+EXPORT_SYMBOL(warn_slowpath_fmt);
 
 void warn_slowpath_null(const char *file, int line)
 {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/7] bug: Rename __WARN_printf_taint() to __WARN_printf()
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
  2019-08-19 23:41 ` [PATCH 1/7] bug: Refactor away warn_slowpath_fmt_taint() Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-19 23:41 ` [PATCH 3/7] bug: Consolidate warn_slowpath_fmt() usage Kees Cook
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

This just renames the helper to improve readability.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/bug.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index ce466432ac3e..e9166a24b336 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -96,14 +96,14 @@ void warn_slowpath_fmt(const char *file, const int line, unsigned taint,
 extern void warn_slowpath_null(const char *file, const int line);
 #define WANT_WARN_ON_SLOWPATH
 #define __WARN()		warn_slowpath_null(__FILE__, __LINE__)
-#define __WARN_printf_taint(taint, arg...)				\
+#define __WARN_printf(taint, arg...)					\
 	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_TAINT(TAINT_WARN); \
 } while (0)
-#define __WARN_printf_taint(taint, arg...)				\
+#define __WARN_printf(taint, arg...)					\
 	do { __warn_printk(arg); __WARN_TAINT(taint); } while (0)
 #endif
 
@@ -127,7 +127,7 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
-		__WARN_printf_taint(TAINT_WARN, format);		\
+		__WARN_printf(TAINT_WARN, format);			\
 	unlikely(__ret_warn_on);					\
 })
 #endif
@@ -135,7 +135,7 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 #define WARN_TAINT(condition, taint, format...) ({			\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
-		__WARN_printf_taint(taint, format);			\
+		__WARN_printf(taint, format);				\
 	unlikely(__ret_warn_on);					\
 })
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/7] bug: Consolidate warn_slowpath_fmt() usage
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
  2019-08-19 23:41 ` [PATCH 1/7] bug: Refactor away warn_slowpath_fmt_taint() Kees Cook
  2019-08-19 23:41 ` [PATCH 2/7] bug: Rename __WARN_printf_taint() to __WARN_printf() Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-19 23:41 ` [PATCH 4/7] bug: Lift "cut here" out of __warn() Kees Cook
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

Instead of having a separate helper for no printk output, just
consolidate the logic into warn_slowpath_fmt().

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/bug.h |  3 +--
 kernel/panic.c            | 14 +++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index e9166a24b336..f76efbcbe3b5 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -93,9 +93,8 @@ struct bug_entry {
 extern __printf(4, 5)
 void warn_slowpath_fmt(const char *file, const int line, unsigned taint,
 		       const char *fmt, ...);
-extern void warn_slowpath_null(const char *file, const int line);
 #define WANT_WARN_ON_SLOWPATH
-#define __WARN()		warn_slowpath_null(__FILE__, __LINE__)
+#define __WARN()		__WARN_printf(TAINT_WARN, NULL)
 #define __WARN_printf(taint, arg...)					\
 	warn_slowpath_fmt(__FILE__, __LINE__, taint, arg)
 #else
diff --git a/kernel/panic.c b/kernel/panic.c
index 4d8b7577c82c..51efdeb2558e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -597,19 +597,19 @@ void warn_slowpath_fmt(const char *file, int line, unsigned taint,
 {
 	struct warn_args args;
 
+	if (!fmt) {
+		pr_warn(CUT_HERE);
+		__warn(file, line, __builtin_return_address(0), taint,
+		       NULL, NULL);
+		return;
+	}
+
 	args.fmt = fmt;
 	va_start(args.args, fmt);
 	__warn(file, line, __builtin_return_address(0), taint, NULL, &args);
 	va_end(args.args);
 }
 EXPORT_SYMBOL(warn_slowpath_fmt);
-
-void warn_slowpath_null(const char *file, int line)
-{
-	pr_warn(CUT_HERE);
-	__warn(file, line, __builtin_return_address(0), TAINT_WARN, NULL, NULL);
-}
-EXPORT_SYMBOL(warn_slowpath_null);
 #else
 void __warn_printk(const char *fmt, ...)
 {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 4/7] bug: Lift "cut here" out of __warn()
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
                   ` (2 preceding siblings ...)
  2019-08-19 23:41 ` [PATCH 3/7] bug: Consolidate warn_slowpath_fmt() usage Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-19 23:41 ` [PATCH 5/7] bug: Clean up helper macros to remove __WARN_TAINT() Kees Cook
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

In preparation for cleaning up "cut here", move the "cut here" logic
up out of __warn() and into callers that pass non-NULL args. For anyone
looking closely, there are two callers that pass NULL args: one already
explicitly prints "cut here". The remaining case is covered by how a
WARN is built, which will be cleaned up in the next patch.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/panic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 51efdeb2558e..dc2243429903 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -551,9 +551,6 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 {
 	disable_trace_on_warning();
 
-	if (args)
-		pr_warn(CUT_HERE);
-
 	if (file)
 		pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n",
 			raw_smp_processor_id(), current->pid, file, line,
@@ -597,8 +594,9 @@ void warn_slowpath_fmt(const char *file, int line, unsigned taint,
 {
 	struct warn_args args;
 
+	pr_warn(CUT_HERE);
+
 	if (!fmt) {
-		pr_warn(CUT_HERE);
 		__warn(file, line, __builtin_return_address(0), taint,
 		       NULL, NULL);
 		return;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 5/7] bug: Clean up helper macros to remove __WARN_TAINT()
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
                   ` (3 preceding siblings ...)
  2019-08-19 23:41 ` [PATCH 4/7] bug: Lift "cut here" out of __warn() Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-19 23:41 ` [PATCH 6/7] bug: Consolidate __WARN_FLAGS usage Kees Cook
  2019-08-19 23:41 ` [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler Kees Cook
  6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

In preparation for cleaning up "cut here" even more, this removes the
__WARN_*TAINT() helpers, as they limit the ability to add new BUGFLAG_*
flags to call sites. They are removed by expanding them into full
__WARN_FLAGS() calls.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/bug.h | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index f76efbcbe3b5..6ea8d258cb96 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -62,13 +62,11 @@ struct bug_entry {
 #endif
 
 #ifdef __WARN_FLAGS
-#define __WARN_TAINT(taint)		__WARN_FLAGS(BUGFLAG_TAINT(taint))
-#define __WARN_ONCE_TAINT(taint)	__WARN_FLAGS(BUGFLAG_ONCE|BUGFLAG_TAINT(taint))
-
 #define WARN_ON_ONCE(condition) ({				\
 	int __ret_warn_on = !!(condition);			\
 	if (unlikely(__ret_warn_on))				\
-		__WARN_ONCE_TAINT(TAINT_WARN);			\
+		__WARN_FLAGS(BUGFLAG_ONCE |			\
+			     BUGFLAG_TAINT(TAINT_WARN));	\
 	unlikely(__ret_warn_on);				\
 })
 #endif
@@ -89,7 +87,7 @@ struct bug_entry {
  *
  * Use the versions with printk format strings to provide better diagnostics.
  */
-#ifndef __WARN_TAINT
+#ifndef __WARN_FLAGS
 extern __printf(4, 5)
 void warn_slowpath_fmt(const char *file, const int line, unsigned taint,
 		       const char *fmt, ...);
@@ -99,11 +97,14 @@ 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_TAINT(TAINT_WARN); \
-} while (0)
-#define __WARN_printf(taint, arg...)					\
-	do { __warn_printk(arg); __WARN_TAINT(taint); } while (0)
+#define __WARN() do {							\
+		printk(KERN_WARNING CUT_HERE);				\
+		__WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN));		\
+	} while (0)
+#define __WARN_printf(taint, arg...) do {				\
+		__warn_printk(arg);					\
+		__WARN_FLAGS(BUGFLAG_TAINT(taint));			\
+	} while (0)
 #endif
 
 /* used internally by panic.c */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 6/7] bug: Consolidate __WARN_FLAGS usage
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
                   ` (4 preceding siblings ...)
  2019-08-19 23:41 ` [PATCH 5/7] bug: Clean up helper macros to remove __WARN_TAINT() Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-19 23:41 ` [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler Kees Cook
  6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

Instead of having separate tests for __WARN_FLAGS, merge the two #ifdef
blocks and replace the synonym WANT_WARN_ON_SLOWPATH macro.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/bug.h | 18 +++++++-----------
 kernel/panic.c            |  2 +-
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 6ea8d258cb96..588dd59a5b72 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -61,16 +61,6 @@ struct bug_entry {
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
-#ifdef __WARN_FLAGS
-#define WARN_ON_ONCE(condition) ({				\
-	int __ret_warn_on = !!(condition);			\
-	if (unlikely(__ret_warn_on))				\
-		__WARN_FLAGS(BUGFLAG_ONCE |			\
-			     BUGFLAG_TAINT(TAINT_WARN));	\
-	unlikely(__ret_warn_on);				\
-})
-#endif
-
 /*
  * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
  * significant kernel issues that need prompt attention if they should ever
@@ -91,7 +81,6 @@ struct bug_entry {
 extern __printf(4, 5)
 void warn_slowpath_fmt(const char *file, const int line, unsigned taint,
 		       const char *fmt, ...);
-#define WANT_WARN_ON_SLOWPATH
 #define __WARN()		__WARN_printf(TAINT_WARN, NULL)
 #define __WARN_printf(taint, arg...)					\
 	warn_slowpath_fmt(__FILE__, __LINE__, taint, arg)
@@ -105,6 +94,13 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
 		__warn_printk(arg);					\
 		__WARN_FLAGS(BUGFLAG_TAINT(taint));			\
 	} while (0)
+#define WARN_ON_ONCE(condition) ({				\
+	int __ret_warn_on = !!(condition);			\
+	if (unlikely(__ret_warn_on))				\
+		__WARN_FLAGS(BUGFLAG_ONCE |			\
+			     BUGFLAG_TAINT(TAINT_WARN));	\
+	unlikely(__ret_warn_on);				\
+})
 #endif
 
 /* used internally by panic.c */
diff --git a/kernel/panic.c b/kernel/panic.c
index dc2243429903..233219b3fb34 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -588,7 +588,7 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 	add_taint(taint, LOCKDEP_STILL_OK);
 }
 
-#ifdef WANT_WARN_ON_SLOWPATH
+#ifndef __WARN_FLAGS
 void warn_slowpath_fmt(const char *file, int line, unsigned taint,
 		       const char *fmt, ...)
 {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
                   ` (5 preceding siblings ...)
  2019-08-19 23:41 ` [PATCH 6/7] bug: Consolidate __WARN_FLAGS usage Kees Cook
@ 2019-08-19 23:41 ` Kees Cook
  2019-08-20 10:06   ` Peter Zijlstra
  6 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2019-08-19 23:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Christophe Leroy, Drew Davenport, Peter Zijlstra,
	Arnd Bergmann, Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

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:

   text    data     bss     dec     hex filename
19691167        5134320 1646664 26472151        193eed7 vmlinux.before
19676362        5134260 1663048 26473670        193f4c6 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 <christophe.leroy@c-s.fr>
Fixes: Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-19 23:41 ` [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler Kees Cook
@ 2019-08-20 10:06   ` Peter Zijlstra
  2019-08-20 10:58     ` Christophe Leroy
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2019-08-20 10:06 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Christophe Leroy, Drew Davenport, Arnd Bergmann,
	Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-20 10:06   ` Peter Zijlstra
@ 2019-08-20 10:58     ` Christophe Leroy
  2019-08-20 16:33       ` Kees Cook
  2019-08-21  1:14       ` Steven Rostedt
  0 siblings, 2 replies; 14+ messages in thread
From: Christophe Leroy @ 2019-08-20 10:58 UTC (permalink / raw)
  To: Peter Zijlstra, Kees Cook
  Cc: Andrew Morton, Drew Davenport, Arnd Bergmann,
	Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel



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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-20 10:58     ` Christophe Leroy
@ 2019-08-20 16:33       ` Kees Cook
  2019-08-21  0:50         ` Steven Rostedt
  2019-08-21  1:14       ` Steven Rostedt
  1 sibling, 1 reply; 14+ messages in thread
From: Kees Cook @ 2019-08-20 16:33 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Peter Zijlstra, Andrew Morton, Drew Davenport, Arnd Bergmann,
	Steven Rostedt (VMware),
	Feng Tang, Petr Mladek, Mauro Carvalho Chehab, Borislav Petkov,
	YueHaibing, linux-arch, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-20 16:33       ` Kees Cook
@ 2019-08-21  0:50         ` Steven Rostedt
  0 siblings, 0 replies; 14+ messages in thread
From: Steven Rostedt @ 2019-08-21  0:50 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christophe Leroy, Peter Zijlstra, Andrew Morton, Drew Davenport,
	Arnd Bergmann, Feng Tang, Petr Mladek, Mauro Carvalho Chehab,
	Borislav Petkov, YueHaibing, linux-arch, linux-kernel

On Tue, 20 Aug 2019 09:33:24 -0700
Kees Cook <keescook@chromium.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-20 10:58     ` Christophe Leroy
  2019-08-20 16:33       ` Kees Cook
@ 2019-08-21  1:14       ` Steven Rostedt
  2019-08-24 17:17         ` Kees Cook
  1 sibling, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2019-08-21  1:14 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Peter Zijlstra, Kees Cook, Andrew Morton, Drew Davenport,
	Arnd Bergmann, Feng Tang, Petr Mladek, Mauro Carvalho Chehab,
	Borislav Petkov, YueHaibing, linux-arch, linux-kernel

On Tue, 20 Aug 2019 12:58:49 +0200
Christophe Leroy <christophe.leroy@c-s.fr> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler
  2019-08-21  1:14       ` Steven Rostedt
@ 2019-08-24 17:17         ` Kees Cook
  0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2019-08-24 17:17 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Christophe Leroy, Peter Zijlstra, Andrew Morton, Drew Davenport,
	Arnd Bergmann, Feng Tang, Petr Mladek, Mauro Carvalho Chehab,
	Borislav Petkov, YueHaibing, linux-arch, linux-kernel

On Tue, Aug 20, 2019 at 09:14:29PM -0400, Steven Rostedt wrote:
> On Tue, 20 Aug 2019 12:58:49 +0200
> Christophe Leroy <christophe.leroy@c-s.fr> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-08-28 17:36 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 23:41 [PATCH 0/7] Clean up WARN() "cut here" handling Kees Cook
2019-08-19 23:41 ` [PATCH 1/7] bug: Refactor away warn_slowpath_fmt_taint() Kees Cook
2019-08-19 23:41 ` [PATCH 2/7] bug: Rename __WARN_printf_taint() to __WARN_printf() Kees Cook
2019-08-19 23:41 ` [PATCH 3/7] bug: Consolidate warn_slowpath_fmt() usage Kees Cook
2019-08-19 23:41 ` [PATCH 4/7] bug: Lift "cut here" out of __warn() Kees Cook
2019-08-19 23:41 ` [PATCH 5/7] bug: Clean up helper macros to remove __WARN_TAINT() Kees Cook
2019-08-19 23:41 ` [PATCH 6/7] bug: Consolidate __WARN_FLAGS usage Kees Cook
2019-08-19 23:41 ` [PATCH 7/7] bug: Move WARN_ON() "cut here" into exception handler Kees Cook
2019-08-20 10:06   ` Peter Zijlstra
2019-08-20 10:58     ` Christophe Leroy
2019-08-20 16:33       ` Kees Cook
2019-08-21  0:50         ` Steven Rostedt
2019-08-21  1:14       ` Steven Rostedt
2019-08-24 17:17         ` Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.