linux-kernel - Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between printk.h and dynamic

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <996a7cf5-b047-5038-c86b-f10820364465@rasmusvillemoes.dk>
Date:   Tue, 11 Jan 2022 17:01:35 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Petr Mladek <pmladek@...e.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     John Ogness <john.ogness@...utronix.de>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Chris Down <chris@...isdown.name>,
        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

On 11/01/2022 15.30, Petr Mladek wrote:
> `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.
> 
> 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'.


Urgh, this doesn't look like the right approach.

>  
>  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;

It looks like this has only one caller, kernel/module.c. I suggest
simply moving the match logic into unknown_module_param_cb(), making it
on par with the other "generic" module parameter async_probe. That is,
do something like


  if (strstr(param, "dyndbg")) {
    if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
      return ddebug_dyndbg_module_param_cb(param, val, modname)
    }
    pr_warn("dyndbg param is supported only in ...");
    return 0; /* allow and ignore */
  }

  pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
  return 0;

That makes it simpler to add more magic/generic module parameters in
unknown_module_param_cb(). No need for a static inline stub, and no need
for conditionally declaring ddebug_dyndbg_module_param_cb(). So all that
is needed in dynamic_debug.h is to remove the static inline definition,
and pull the declaration outside #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
protection.

What's with the strstr, btw? Shouldn't it just be !strcmp()?

> @@ -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;
>  }

And for this one I think the solution is even simpler, as I can't find
any in-tree callers. Perhaps just nuke it entirely?

Rasmus