| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yd2Z0NrG1+Yhmseg@chrisdown.name>
Date: Tue, 11 Jan 2022 14:53:04 +0000
From: Chris Down <chris@...isdown.name>
To: Petr Mladek <pmladek@...e.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
John Ogness <john.ogness@...utronix.de>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Marc Zyngier <maz@...nel.org>,
Andrew Scull <ascull@...gle.com>,
Will Deacon <will@...nel.org>, Jason Baron <jbaron@...mai.com>,
Peter Zijlstra <peterz@...radead.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
linux-kernel@...r.kernel.org,
Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between
printk.h and dynamic_debug.h
Hey Petr,
Petr Mladek writes:
>`make headerdep` reports many circular dependencies with the same
>pattern:
>
>In file included from linux/printk.h,
> from linux/dynamic_debug.h:188
> from linux/printk.h:559 <-- here
>
>It looks like false positive:
>
> + printk.h includes dynamic_debug.h when CONFIG_DYNAMIC_DEBUG_CORE
> + dynamic_debug.h includes printk.h when !CONFIG_DYNAMIC_DEBUG_CORE
>
>Anyway, it would be great to get rid of this dependency because it is
>tricky and it might hit us in the future. Also it might hide another
>more complicated cyclic dependencies.
Sounds reasonable.
>One solution would be to move the inlined ddebug_dyndbg_module_param_cb()
>and dynamic_debug_exec_queries() from 'dynamic_debug.h' into some .c so
>that it will not be needed to inline printk() in 'dynamic_debug.h'.
>
>The obvious location would be 'lib/dynamic_debug.c'. But it is built
>only when CONFIG_DYNAMIC_DEBUG_CORE is set. And the problematic
>inline variants are used only when this config option is _not_ set.
>So that it is not that easy.
>
>Instead, this patch adds 'include/linux/printk_core.h' and moves some
>lowlevel printk API there. Then the raw _printk() can be called from
>the inlined code in 'dynamic_debug.h'.
>
>Note that it is not enough to declare _printk() in 'dynamic_debug.h'.
>It would break build with CONFIG_PRINTK where it is an inlined nope.
s/nope/nop/, and you mean !CONFIG_PRINTK instead of CONFIG_PRINTK. Or did I
misunderstand?
>
>The drawback of this approach is that _printk() does not add metadata for
>printk indexing and the message is not listed in <debugfs>/printk/index/.
>But it should be acceptable here. The strings are used only for debugging.
Agreed that these are fine to omit from a printk indexing perspective.
>The advantage is that this approach might be used to solve other cyclic
>dependencies, for example in bug.h.
>
>Reported-by: Andy Shevchenko <andy.shevchenko@...il.com>
>Signed-off-by: Petr Mladek <pmladek@...e.com>
Thanks! Looks good to me with changelog fixes.
Acked-by: Chris Down <chris@...isdown.name>
>---
> MAINTAINERS | 1 +
> include/linux/dynamic_debug.h | 10 ++---
> include/linux/printk.h | 57 +-------------------------
> include/linux/printk_core.h | 76 +++++++++++++++++++++++++++++++++++
> 4 files changed, 83 insertions(+), 61 deletions(-)
> create mode 100644 include/linux/printk_core.h
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index fb18ce7168aa..eadb77da01db 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -15340,6 +15340,7 @@ R: Steven Rostedt <rostedt@...dmis.org>
> R: John Ogness <john.ogness@...utronix.de>
> S: Maintained
> F: include/linux/printk.h
>+F: include/linux/printk_core.h
> F: kernel/printk/
>
> PRINTK INDEXING
>diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
>index dce631e678dd..63f6ebd6a14c 100644
>--- a/include/linux/dynamic_debug.h
>+++ b/include/linux/dynamic_debug.h
>@@ -185,7 +185,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>
> #include <linux/string.h>
> #include <linux/errno.h>
>-#include <linux/printk.h>
>+#include <linux/printk_core.h>
>
> static inline int ddebug_add_module(struct _ddebug *tab, unsigned int n,
> const char *modname)
>@@ -202,9 +202,8 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
> const char *modname)
> {
> if (strstr(param, "dyndbg")) {
>- /* avoid pr_warn(), which wants pr_fmt() fully defined */
>- printk(KERN_WARNING "dyndbg param is supported only in "
>- "CONFIG_DYNAMIC_DEBUG builds\n");
>+ /* Use raw _printk() to avoid cyclic dependency. */
>+ _printk(KERN_WARNING "dyndbg param is supported only in CONFIG_DYNAMIC_DEBUG builds\n");
> return 0; /* allow and ignore */
> }
> return -EINVAL;
>@@ -223,7 +222,8 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
>
> static inline int dynamic_debug_exec_queries(const char *query, const char *modname)
> {
>- pr_warn("kernel not built with CONFIG_DYNAMIC_DEBUG_CORE\n");
>+ /* Use raw _printk() to avoid cyclic dependency. */
>+ _printk(KERN_WARNING "kernel not built with CONFIG_DYNAMIC_DEBUG_CORE\n");
> return 0;
> }
>
>diff --git a/include/linux/printk.h b/include/linux/printk.h
>index 9497f6b98339..c20f55df7fa6 100644
>--- a/include/linux/printk.h
>+++ b/include/linux/printk.h
>@@ -5,6 +5,7 @@
> #include <linux/stdarg.h>
> #include <linux/init.h>
> #include <linux/kern_levels.h>
>+#include <linux/printk_core.h>
> #include <linux/linkage.h>
> #include <linux/cache.h>
> #include <linux/ratelimit_types.h>
>@@ -144,32 +145,6 @@ void early_printk(const char *s, ...) { }
> struct dev_printk_info;
>
> #ifdef CONFIG_PRINTK
>-asmlinkage __printf(4, 0)
>-int vprintk_emit(int facility, int level,
>- const struct dev_printk_info *dev_info,
>- const char *fmt, va_list args);
>-
>-asmlinkage __printf(1, 0)
>-int vprintk(const char *fmt, va_list args);
>-
>-asmlinkage __printf(1, 2) __cold
>-int _printk(const char *fmt, ...);
>-
>-/*
>- * Special printk facility for scheduler/timekeeping use only, _DO_NOT_USE_ !
>- */
>-__printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
>-
>-extern void __printk_safe_enter(void);
>-extern void __printk_safe_exit(void);
>-/*
>- * The printk_deferred_enter/exit macros are available only as a hack for
>- * some code paths that need to defer all printk console printing. Interrupts
>- * must be disabled for the deferred duration.
>- */
>-#define printk_deferred_enter __printk_safe_enter
>-#define printk_deferred_exit __printk_safe_exit
>-
> /*
> * Please don't use printk_ratelimit(), because it shares ratelimiting state
> * with all other unrelated printk_ratelimit() callsites. Instead use
>@@ -189,7 +164,6 @@ devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void *buf,
>
> extern void wake_up_klogd(void);
>
>-char *log_buf_addr_get(void);
> u32 log_buf_len_get(void);
> void log_buf_vmcoreinfo_setup(void);
> void __init setup_log_buf(int early);
>@@ -200,30 +174,6 @@ extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
> extern asmlinkage void dump_stack(void) __cold;
> void printk_trigger_flush(void);
> #else
>-static inline __printf(1, 0)
>-int vprintk(const char *s, va_list args)
>-{
>- return 0;
>-}
>-static inline __printf(1, 2) __cold
>-int _printk(const char *s, ...)
>-{
>- return 0;
>-}
>-static inline __printf(1, 2) __cold
>-int _printk_deferred(const char *s, ...)
>-{
>- return 0;
>-}
>-
>-static inline void printk_deferred_enter(void)
>-{
>-}
>-
>-static inline void printk_deferred_exit(void)
>-{
>-}
>-
> static inline int printk_ratelimit(void)
> {
> return 0;
>@@ -238,11 +188,6 @@ static inline void wake_up_klogd(void)
> {
> }
>
>-static inline char *log_buf_addr_get(void)
>-{
>- return NULL;
>-}
>-
> static inline u32 log_buf_len_get(void)
> {
> return 0;
>diff --git a/include/linux/printk_core.h b/include/linux/printk_core.h
>new file mode 100644
>index 000000000000..a2b8727a2873
>--- /dev/null
>+++ b/include/linux/printk_core.h
>@@ -0,0 +1,76 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+#ifndef __KERNEL_PRINTK_CORE__
>+#define __KERNEL_PRINTK_CORE__
>+
>+#include <linux/stdarg.h>
>+#include <linux/kern_levels.h>
>+#include <linux/linkage.h>
>+
>+/* Low level printk API. Use carefully! */
>+
>+#ifdef CONFIG_PRINTK
>+
>+struct dev_printk_info;
>+
>+asmlinkage __printf(4, 0)
>+int vprintk_emit(int facility, int level,
>+ const struct dev_printk_info *dev_info,
>+ const char *fmt, va_list args);
>+
>+asmlinkage __printf(1, 0)
>+int vprintk(const char *fmt, va_list args);
>+
>+asmlinkage __printf(1, 2) __cold
>+int _printk(const char *fmt, ...);
>+
>+/*
>+ * Special printk facility for scheduler/timekeeping use only, _DO_NOT_USE_ !
>+ */
>+__printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
>+
>+extern void __printk_safe_enter(void);
>+extern void __printk_safe_exit(void);
>+/*
>+ * The printk_deferred_enter/exit macros are available only as a hack for
>+ * some code paths that need to defer all printk console printing. Interrupts
>+ * must be disabled for the deferred duration.
>+ */
>+#define printk_deferred_enter __printk_safe_enter
>+#define printk_deferred_exit __printk_safe_exit
>+
>+char *log_buf_addr_get(void);
>+
>+#else /* CONFIG_PRINTK */
>+
>+static inline __printf(1, 0)
>+int vprintk(const char *s, va_list args)
>+{
>+ return 0;
>+}
>+static inline __printf(1, 2) __cold
>+int _printk(const char *s, ...)
>+{
>+ return 0;
>+}
>+static inline __printf(1, 2) __cold
>+int _printk_deferred(const char *s, ...)
>+{
>+ return 0;
>+}
>+
>+static inline void printk_deferred_enter(void)
>+{
>+}
>+
>+static inline void printk_deferred_exit(void)
>+{
>+}
>+
>+static inline char *log_buf_addr_get(void)
>+{
>+ return NULL;
>+}
>+
>+#endif /* CONFING_PRINTK */
>+
>+#endif
>--
>2.26.2
>
Powered by blists - more mailing lists