From: Borislav Petkov <[email protected]>
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/[email protected]
for example.
This version is an alternative of
https://lore.kernel.org/r/[email protected]
which needed to touch every caller not checking the registration
routine's return value.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
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
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
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
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
On Thu, Dec 02, 2021 at 02:36:01PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> 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/[email protected]
>
> for example.
>
> This version is an alternative of
>
> https://lore.kernel.org/r/[email protected]
>
> which needed to touch every caller not checking the registration
> routine's return value.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> 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
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
From: Borislav Petkov <[email protected]>
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/[email protected]
for example.
This version is an alternative of
https://lore.kernel.org/r/[email protected]
which needed to touch every caller not checking the registration
routine's return value.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
On 2021-12-23 16:31:01 [+0100], Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> 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/[email protected]
>
> for example.
>
> This version is an alternative of
>
> https://lore.kernel.org/r/[email protected]
>
> which needed to touch every caller not checking the registration
> routine's return value.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
Sebastian
On Thu, Dec 23, 2021 at 04:31:01PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> 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/[email protected]
>
> for example.
>
> This version is an alternative of
>
> https://lore.kernel.org/r/[email protected]
>
> which needed to touch every caller not checking the registration
> routine's return value.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
Acked-by: Alan Stern <[email protected]>
> 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
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 <[email protected]>
AuthorDate: Wed, 01 Dec 2021 22:28:14 +01:00
Committer: Borislav Petkov <[email protected]>
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/[email protected]
for example.
This version is an alternative of
https://lore.kernel.org/r/[email protected]
which needed to touch every caller not checking the registration
routine's return value.
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
Acked-by: Alan Stern <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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)