[PATCH] notifier: Return an error when a callback has already been registered

[PATCH] notifier: Return an error when a callback has already been registered

[Borislav Petkov]

From: Borislav Petkov <bp@suse.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@suse.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;
+}
+
 static int notifier_chain_unregister(struct notifier_block **nl,
 		struct notifier_block *n)
 {
@@ -134,7 +144,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, %-EEXIST on error.
  */
 int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 		struct notifier_block *n)
@@ -216,7 +226,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, %-EEXIST on error.
  */
 int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
 		struct notifier_block *n)
@@ -335,7 +345,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, %-EEXIST on error.
  */
 int raw_notifier_chain_register(struct raw_notifier_head *nh,
 		struct notifier_block *n)
@@ -406,7 +416,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, %-EEXIST on error.
  */
 int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 		struct notifier_block *n)
-- 
2.29.2

Re: [PATCH] notifier: Return an error when a callback has already been registered

[Sebastian Andrzej Siewior]

On 2021-12-02 14:36:01 [+0100], Borislav Petkov wrote:
> --- 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;

This could be s/0/-EEXIST/ or do I miss something?
I appreciate the updated warning with %ps!

> -		}
> +		if (unlikely((*nl) == n))
> +			return -EEXIST;
>  		if (n->priority > (*nl)->priority)
>  			break;
>  		nl = &((*nl)->next);

Sebastian

Re: [PATCH] notifier: Return an error when a callback has already been registered

[Borislav Petkov]

On Thu, Dec 02, 2021 at 03:16:30PM +0100, Sebastian Andrzej Siewior wrote:
> >  	while ((*nl) != NULL) {
> > -		if (unlikely((*nl) == n)) {
> > -			WARN(1, "double register detected");
> > -			return 0;
> 
> This could be s/0/-EEXIST/ or do I miss something?

It is...

> I appreciate the updated warning with %ps!

Send peterz your thanks for that. :-)

> > -		}
> > +		if (unlikely((*nl) == n))
> > +			return -EEXIST;
			^^^^^^^^^^^^^^^

... right here.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] notifier: Return an error when a callback has already been registered

[Sebastian Andrzej Siewior]

On 2021-12-02 15:23:36 [+0100], Borislav Petkov wrote:
> On Thu, Dec 02, 2021 at 03:16:30PM +0100, Sebastian Andrzej Siewior wrote:
> > >  	while ((*nl) != NULL) {
> > > -		if (unlikely((*nl) == n)) {
> > > -			WARN(1, "double register detected");
> > > -			return 0;
> > 
> > This could be s/0/-EEXIST/ or do I miss something?
> 
> It is...
…
> > > -		}
> > > +		if (unlikely((*nl) == n))
> > > +			return -EEXIST;
> 			^^^^^^^^^^^^^^^
> 
> ... right here.

I meant without the extra function. I'm fine either way, just curious :)

Sebastian

Re: [PATCH] notifier: Return an error when a callback has already been registered

[Borislav Petkov]

On Thu, Dec 02, 2021 at 03:31:49PM +0100, Sebastian Andrzej Siewior wrote:
> I meant without the extra function. I'm fine either way, just curious :)

Extra is cleaner and simpler and you know we love that. :-)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] notifier: Return an error when a callback has already been registered

[Alan Stern]

On Thu, Dec 02, 2021 at 02:36:01PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.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@suse.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

Re: [PATCH] notifier: Return an error when a callback has already been registered

[Borislav Petkov]

On Thu, Dec 02, 2021 at 10:41:55AM -0500, Alan Stern wrote:
> 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?

Well, I like keeping things separate and prefer to keep the error
handling and warning in another function. But Sebastian asked the same
thing already so if people prefer that, I'll change it. I don't feel too
strongly about it, tbh.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH -v2] notifier: Return an error when a callback has already been registered

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Return -EEXIST when a notifier callback has already been registered on a
notifier chain.

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@suse.de>
---
 kernel/notifier.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index b8251dc0bc0f..ba005ebf4730 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -20,12 +20,13 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
  */
 
 static int notifier_chain_register(struct notifier_block **nl,
-		struct notifier_block *n)
+				   struct notifier_block *n)
 {
 	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;
@@ -134,7 +135,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, %-EEXIST on error.
  */
 int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 		struct notifier_block *n)
@@ -216,7 +217,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, %-EEXIST on error.
  */
 int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
 		struct notifier_block *n)
@@ -335,7 +336,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, %-EEXIST on error.
  */
 int raw_notifier_chain_register(struct raw_notifier_head *nh,
 		struct notifier_block *n)
@@ -406,7 +407,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, %-EEXIST on error.
  */
 int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 		struct notifier_block *n)
-- 
2.29.2


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH -v2] notifier: Return an error when a callback has already been registered

[Sebastian Andrzej Siewior]

On 2021-12-23 16:31:01 [+0100], Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Return -EEXIST when a notifier callback has already been registered on a
> notifier chain.
> 
> 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@suse.de>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [PATCH -v2] notifier: Return an error when a callback has already been registered

[Alan Stern]

On Thu, Dec 23, 2021 at 04:31:01PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Return -EEXIST when a notifier callback has already been registered on a
> notifier chain.
> 
> 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@suse.de>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  kernel/notifier.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index b8251dc0bc0f..ba005ebf4730 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -20,12 +20,13 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
>   */
>  
>  static int notifier_chain_register(struct notifier_block **nl,
> -		struct notifier_block *n)
> +				   struct notifier_block *n)
>  {
>  	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;
> @@ -134,7 +135,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, %-EEXIST on error.
>   */
>  int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
>  		struct notifier_block *n)
> @@ -216,7 +217,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, %-EEXIST on error.
>   */
>  int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
>  		struct notifier_block *n)
> @@ -335,7 +336,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, %-EEXIST on error.
>   */
>  int raw_notifier_chain_register(struct raw_notifier_head *nh,
>  		struct notifier_block *n)
> @@ -406,7 +407,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, %-EEXIST on error.
>   */
>  int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
>  		struct notifier_block *n)
> -- 
> 2.29.2
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

[tip: core/core] notifier: Return an error when a callback has already been registered

[tip-bot2 for Borislav Petkov]

The following commit has been merged into the core/core branch of tip:

Commit-ID:     5abb065dca7301de90b7c44bbcdc378e49e4d362
Gitweb:        https://git.kernel.org/tip/5abb065dca7301de90b7c44bbcdc378e49e4d362
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Wed, 01 Dec 2021 22:28:14 +01:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Wed, 29 Dec 2021 10:37:33 +01:00

notifier: Return an error when a callback has already been registered

Return -EEXIST when a notifier callback has already been registered on a
notifier chain.

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@suse.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Link: https://lore.kernel.org/r/YcSWNdUBS8A2ZB3s@zn.tnic
---
 kernel/notifier.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index b8251dc..ba005eb 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -20,12 +20,13 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
  */
 
 static int notifier_chain_register(struct notifier_block **nl,
-		struct notifier_block *n)
+				   struct notifier_block *n)
 {
 	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;
@@ -134,7 +135,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, %-EEXIST on error.
  */
 int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 		struct notifier_block *n)
@@ -216,7 +217,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, %-EEXIST on error.
  */
 int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
 		struct notifier_block *n)
@@ -335,7 +336,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, %-EEXIST on error.
  */
 int raw_notifier_chain_register(struct raw_notifier_head *nh,
 		struct notifier_block *n)
@@ -406,7 +407,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, %-EEXIST on error.
  */
 int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 		struct notifier_block *n)