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

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

[Borislav Petkov]

From: Borislav Petkov <bp@suse.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:

  https://lore.kernel.org/amd-gfx/20210512013058.6827-1-mukul.joshi@amd.com/

Which is unnecessary.

Return -EEXIST to signal that case so that callers can act accordingly.

Signed-off-by: Borislav Petkov <bp@suse.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;
 		}
 		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.
  */
 int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 		struct notifier_block *n)
@@ -235,7 +235,7 @@ NOKPROBE_SYMBOL(atomic_notifier_call_chain);
  *	Adds a notifier to a blocking notifier chain.
  *	Must be called in process context.
  *
- *	Currently always returns zero.
+ *	Returns 0 on success, !0 on error.
  */
 int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
 		struct notifier_block *n)
@@ -354,7 +354,7 @@ EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);
  *	Adds a notifier to a raw notifier chain.
  *	All locking must be provided by the caller.
  *
- *	Currently always returns zero.
+ *	Returns 0 on success, !0 on error.
  */
 int raw_notifier_chain_register(struct raw_notifier_head *nh,
 		struct notifier_block *n)
@@ -425,7 +425,7 @@ EXPORT_SYMBOL_GPL(raw_notifier_call_chain);
  *	Adds a notifier to an SRCU notifier chain.
  *	Must be called in process context.
  *
- *	Currently always returns zero.
+ *	Returns 0 on success, !0 on error.
  */
 int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 		struct notifier_block *n)
-- 
2.29.2

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

[Sean Christopherson]

On Fri, Jun 04, 2021, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.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@suse.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)

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

[Borislav Petkov]

On Fri, Jun 04, 2021 at 04:44:29PM +0000, Sean Christopherson wrote:
> 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.

Yeah, so tglx gave me a much better idea on IRC: make that function
__must_check and drop the WARN.

I'll play with a full allmodconfig build and see what complains and
whether I can make it into a kernelnewbies project. :)

Thanks for the review anyway, I'll use some of it in the new version.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette