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: <9f44aae6-ec00-7ede-ec19-6e67ceb74510@huawei.com>
Date:   Thu, 28 Apr 2022 09:01:13 +0800
From:   Xiaoming Ni <nixiaoming@...wei.com>
To:     "Guilherme G. Piccoli" <gpiccoli@...lia.com>,
        <akpm@...ux-foundation.org>, <bhe@...hat.com>, <pmladek@...e.com>,
        <kexec@...ts.infradead.org>
CC:     <linux-kernel@...r.kernel.org>,
        <bcm-kernel-feedback-list@...adcom.com>,
        <coresight@...ts.linaro.org>, <linuxppc-dev@...ts.ozlabs.org>,
        <linux-alpha@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-edac@...r.kernel.org>, <linux-hyperv@...r.kernel.org>,
        <linux-leds@...r.kernel.org>, <linux-mips@...r.kernel.org>,
        <linux-parisc@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-s390@...r.kernel.org>,
        <linux-tegra@...r.kernel.org>, <linux-um@...ts.infradead.org>,
        <linux-xtensa@...ux-xtensa.org>, <netdev@...r.kernel.org>,
        <openipmi-developer@...ts.sourceforge.net>, <rcu@...r.kernel.org>,
        <sparclinux@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
        <x86@...nel.org>, <kernel-dev@...lia.com>, <kernel@...ccoli.net>,
        <halves@...onical.com>, <fabiomirmar@...il.com>,
        <alejandro.j.jimenez@...cle.com>,
        <andriy.shevchenko@...ux.intel.com>, <arnd@...db.de>,
        <bp@...en8.de>, <corbet@....net>, <d.hatayama@...fujitsu.com>,
        <dave.hansen@...ux.intel.com>, <dyoung@...hat.com>,
        <feng.tang@...el.com>, <gregkh@...uxfoundation.org>,
        <mikelley@...rosoft.com>, <hidehiro.kawai.ez@...achi.com>,
        <jgross@...e.com>, <john.ogness@...utronix.de>,
        <keescook@...omium.org>, <luto@...nel.org>, <mhiramat@...nel.org>,
        <mingo@...hat.com>, <paulmck@...nel.org>, <peterz@...radead.org>,
        <rostedt@...dmis.org>, <senozhatsky@...omium.org>,
        <stern@...land.harvard.edu>, <tglx@...utronix.de>,
        <vgoyal@...hat.com>, <vkuznets@...hat.com>, <will@...nel.org>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Valentin Schneider <valentin.schneider@....com>
Subject: Re: [PATCH 18/30] notifier: Show function names on notifier routines
 if DEBUG_NOTIFIERS is set

On 2022/4/28 6:49, Guilherme G. Piccoli wrote:
> Currently we have a debug infrastructure in the notifiers file, but
> it's very simple/limited. This patch extends it by:
> 
> (a) Showing all registered/unregistered notifiers' callback names;
> 
> (b) Adding a dynamic debug tuning to allow showing called notifiers'
> function names. Notice that this should be guarded as a tunable since
> it can flood the kernel log buffer.
> 
> Cc: Arjan van de Ven <arjan@...ux.intel.com>
> Cc: Cong Wang <xiyou.wangcong@...il.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Cc: Valentin Schneider <valentin.schneider@....com>
> Cc: Xiaoming Ni <nixiaoming@...wei.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@...lia.com>
> ---
> 
> We have some design decisions that worth discussing here:
> 
> (a) First of call, using C99 helps a lot to write clear and concise code, but
> due to commit 4d94f910e79a ("Kbuild: use -Wdeclaration-after-statement") we
> have a warning if mixing variable declarations with code. For this patch though,
> doing that makes the code way clear, so decision was to add the debug code
> inside brackets whenever this warning pops up. We can change that, but that'll
> cause more ifdefs in the same function.
> 
> (b) In the symbol lookup helper function, we modify the parameter passed but
> even more, we return it as well! This is unusual and seems unnecessary, but was
> the strategy taken to allow embedding such function in the pr_debug() call.
> 
> Not doing that would likely requiring 3 symbol_name variables to avoid
> concurrency (registering notifier A while calling notifier B) - we rely in
> local variables as a serialization mechanism.
> 
> We're open for suggestions in case this design is not appropriate;
> thanks in advance!
> 
>   kernel/notifier.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index ba005ebf4730..21032ebcde57 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -7,6 +7,22 @@
>   #include <linux/vmalloc.h>
>   #include <linux/reboot.h>
>   
> +#ifdef CONFIG_DEBUG_NOTIFIERS
> +#include <linux/kallsyms.h>
> +
> +/*
> + *	Helper to get symbol names in case DEBUG_NOTIFIERS is set.
> + *	Return the modified parameter is a strategy used to achieve
> + *	the pr_debug() functionality - with this, function is only
> + *	executed if the dynamic debug tuning is effectively set.
> + */
> +static inline char *notifier_name(struct notifier_block *nb, char *sym_name)
> +{
> +	lookup_symbol_name((unsigned long)(nb->notifier_call), sym_name);
> +	return sym_name;
> +}
> +#endif
> +
>   /*
>    *	Notifier list for kernel code which wants to be called
>    *	at shutdown. This is used to stop any idling DMA operations
> @@ -34,20 +50,41 @@ static int notifier_chain_register(struct notifier_block **nl,
>   	}
>   	n->next = *nl;
>   	rcu_assign_pointer(*nl, n);
> +
> +#ifdef CONFIG_DEBUG_NOTIFIERS
> +	{
> +		char sym_name[KSYM_NAME_LEN];
> +
> +		pr_info("notifiers: registered %s()\n",
> +			notifier_name(n, sym_name));
> +	}

Duplicate Code.

Is it better to use __func__ and %pS?

pr_info("%s: %pS\n", __func__, n->notifier_call);


> +#endif
>   	return 0;
>   }
>   
>   static int notifier_chain_unregister(struct notifier_block **nl,
>   		struct notifier_block *n)
>   {
> +	int ret = -ENOENT;
> +
>   	while ((*nl) != NULL) {
>   		if ((*nl) == n) {
>   			rcu_assign_pointer(*nl, n->next);
> -			return 0;
> +			ret = 0;
> +			break;
>   		}
>   		nl = &((*nl)->next);
>   	}
> -	return -ENOENT;
> +
> +#ifdef CONFIG_DEBUG_NOTIFIERS
> +	if (!ret) {
> +		char sym_name[KSYM_NAME_LEN];
> +
> +		pr_info("notifiers: unregistered %s()\n",
> +			notifier_name(n, sym_name));
> +	}
Duplicate Code.

Is it better to use __func__ and %pS?

pr_info("%s: %pS\n", __func__, n->notifier_call);
> +#endif
> +	return ret;
>   }
>   
>   /**
> @@ -80,6 +117,13 @@ static int notifier_call_chain(struct notifier_block **nl,
>   			nb = next_nb;
>   			continue;
>   		}
> +
Is the "#ifdef" missing here?
> +		{
> +			char sym_name[KSYM_NAME_LEN];
> +
> +			pr_debug("notifiers: calling %s()\n",
> +				 notifier_name(nb, sym_name));
Duplicate Code.

Is it better to use __func__ and %pS?

pr_info("%s: %pS\n", __func__, n->notifier_call);
> +		}
>   #endif
>   		ret = nb->notifier_call(nb, val, v);
>   
> 

Thanks
Xiaoming Ni

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ