linux-kernel - Re: [PATCH -v2] notifier: Return non-null when callback is already registered

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: <YLpYbQnswxZUB5sW@google.com>
Date:   Fri, 4 Jun 2021 16:44:29 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -v2] notifier: Return non-null when callback is already
 registered

On Fri, Jun 04, 2021, Borislav Petkov wrote:
> From: Borislav Petkov <bp@...e.de>
>
> The notifier registration routine doesn't return a proper error value
> when a callback has already been registered, leading people to track
> whether that regisration has happened at the call site:
               ^^^^^^^^^^^
               registration
>
>   https://lore.kernel.org/amd-gfx/20210512013058.6827-1-mukul.joshi@amd.com/
>
> Which is unnecessary.

The WARN is still going to make that "necessary", and the vast number of callers
and variations that don't check the return value means that WARN isn't going
anywhere for quite some time.  Returning an error code still makes sense, but
the changelog is misleading in that it implies callers can blindly register
without any repercussions.

> Return -EEXIST to signal that case so that callers can act accordingly.
>
> Signed-off-by: Borislav Petkov <bp@...e.de>
> ---
>  kernel/notifier.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index 1b019cbca594..5a31bc9b24b4 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -25,7 +25,7 @@ static int notifier_chain_register(struct notifier_block **nl,
>       while ((*nl) != NULL) {
>               if (unlikely((*nl) == n)) {
>                       WARN(1, "double register detected");
> -                     return 0;
> +                     return -EEXIST;

Opportunistically squish the WARN into the if?

                if (WARN((*nl) == n, "double register detected"))
                        return -EEXIST;

>               }
>               if (n->priority > (*nl)->priority)
>                       break;
> @@ -134,7 +134,7 @@ static int notifier_call_chain_robust(struct notifier_block **nl,
>   *
>   *	Adds a notifier to an atomic notifier chain.
>   *
> - *	Currently always returns zero.
> + *	Returns 0 on success, !0 on error.

Maybe explicitly call out %-EEXIST to be consistent with the unregister wrappers?
Those are tightly coupled to the notifier_chain_unregister() behavior.

  Returns zero on success or %-ENOENT on failure.

If that's unpalatable, it's probably a good idea to at least clarify that it
returns a -errno, there's at least one call site that explicitly checks for a
negative return value.

  static int __init gic_clocksource_of_init(struct device_node *node)
  {
	...

	ret = gic_clockevent_init();
	if (!ret && !IS_ERR(clk)) {
		if (clk_notifier_register(clk, &gic_clk_nb) < 0)  <-------
			pr_warn("Unable to register clock notifier\n");
	}

>   */
>  int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
>  		struct notifier_block *n)