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]
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)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ