[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YajpQ/dA7h3QnkCR@rowland.harvard.edu>
Date: Thu, 2 Dec 2021 10:41:55 -0500
From: Alan Stern <stern@...land.harvard.edu>
To: Borislav Petkov <bp@...en8.de>
Cc: X86 ML <x86@...nel.org>,
Valentin Schneider <valentin.schneider@....com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Steven Rostedt <rostedt@...dmis.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] notifier: Return an error when a callback has already
been registered
On Thu, Dec 02, 2021 at 02:36:01PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@...e.de>
>
> Add another indirection to the notifiers callback registration path
> which does only the error checking and propagates the proper error value
> to the callers instead of returning only 0.
>
> This should avoid any homegrown registration tracking at the callsite
> like
>
> https://lore.kernel.org/amd-gfx/20210512013058.6827-1-mukul.joshi@amd.com
>
> for example.
>
> This version is an alternative of
>
> https://lore.kernel.org/r/20211108101157.15189-1-bp@alien8.de
>
> which needed to touch every caller not checking the registration
> routine's return value.
>
> Signed-off-by: Borislav Petkov <bp@...e.de>
> ---
> kernel/notifier.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index b8251dc0bc0f..0820a156ce83 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -19,14 +19,12 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
> * are layered on top of these, with appropriate locking added.
> */
>
> -static int notifier_chain_register(struct notifier_block **nl,
> - struct notifier_block *n)
> +static int __notifier_chain_register(struct notifier_block **nl,
> + struct notifier_block *n)
> {
> while ((*nl) != NULL) {
> - if (unlikely((*nl) == n)) {
> - WARN(1, "double register detected");
> - return 0;
> - }
> + if (unlikely((*nl) == n))
> + return -EEXIST;
> if (n->priority > (*nl)->priority)
> break;
> nl = &((*nl)->next);
> @@ -36,6 +34,18 @@ static int notifier_chain_register(struct notifier_block **nl,
> return 0;
> }
>
> +static int notifier_chain_register(struct notifier_block **nl,
> + struct notifier_block *n)
> +{
> + int ret = __notifier_chain_register(nl, n);
> +
> + if (ret == -EEXIST)
> + WARN(1, "notifier callback %ps already registered",
> + n->notifier_call);
> +
> + return ret;
> +}
How about doing this instead?
@@ -24,8 +24,9 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
> - WARN(1, "double register detected");
> - return 0;
> + WARN(1, "notifier callback %ps already registered",
> + n->notifier_call);
> + return -EEXIST;
}
if (n->priority > (*nl)->priority)
break;
A patch that adds three new lines of code has got to be simpler than and
preferable to a patch that adds about eleven lines (including a whole new
function), right?
Alan Stern
Powered by blists - more mailing lists