[PATCH] kernel/notifier.c: remove notifier_chain_register

[PATCH] kernel/notifier.c: remove notifier_chain_register

[Xiaoming Ni]

Registering the same notifier to a hook repeatedly can cause the hook
list to form a ring or lose other members of the list.

case1: An infinite loop in notifier_chain_register can cause soft lockup
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier2);

case2: An infinite loop in notifier_chain_register can cause soft lockup
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
	atomic_notifier_call_chain(&test_notifier_list, 0, NULL);

case3: lose other hook "test_notifier2"
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier2);
	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);

case4: Unregister returns 0, but the hook is still in the linked list,
	and it is not really registered. If you call notifier_call_chain
	after ko is unloaded, it will trigger oops.

If the system is configured with softlockup_panic and the same
hook is repeatedly registered on the panic_notifier_list, it
will cause a loop panic.

The only difference between notifier_chain_cond_register and
notifier_chain_register is that a check is added in order to
avoid registering the same notifier multiple times to the same hook.
So consider removing notifier_chain_register and replacing it
with notifier_chain_cond_register.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 kernel/notifier.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index d9f5081..56efd54 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -19,20 +19,6 @@
  *	are layered on top of these, with appropriate locking added.
  */
 
-static int notifier_chain_register(struct notifier_block **nl,
-		struct notifier_block *n)
-{
-	while ((*nl) != NULL) {
-		WARN_ONCE(((*nl) == n), "double register detected");
-		if (n->priority > (*nl)->priority)
-			break;
-		nl = &((*nl)->next);
-	}
-	n->next = *nl;
-	rcu_assign_pointer(*nl, n);
-	return 0;
-}
-
 static int notifier_chain_cond_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
@@ -127,7 +113,7 @@ int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 	int ret;
 
 	spin_lock_irqsave(&nh->lock, flags);
-	ret = notifier_chain_register(&nh->head, n);
+	ret = notifier_chain_cond_register(&nh->head, n);
 	spin_unlock_irqrestore(&nh->lock, flags);
 	return ret;
 }
@@ -223,10 +209,10 @@ int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
 	 * such times we must not call down_write().
 	 */
 	if (unlikely(system_state == SYSTEM_BOOTING))
-		return notifier_chain_register(&nh->head, n);
+		return notifier_chain_cond_register(&nh->head, n);
 
 	down_write(&nh->rwsem);
-	ret = notifier_chain_register(&nh->head, n);
+	ret = notifier_chain_cond_register(&nh->head, n);
 	up_write(&nh->rwsem);
 	return ret;
 }
@@ -349,7 +335,7 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
 int raw_notifier_chain_register(struct raw_notifier_head *nh,
 		struct notifier_block *n)
 {
-	return notifier_chain_register(&nh->head, n);
+	return notifier_chain_cond_register(&nh->head, n);
 }
 EXPORT_SYMBOL_GPL(raw_notifier_chain_register);
 
@@ -431,10 +417,10 @@ int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 	 * such times we must not call mutex_lock().
 	 */
 	if (unlikely(system_state == SYSTEM_BOOTING))
-		return notifier_chain_register(&nh->head, n);
+		return notifier_chain_cond_register(&nh->head, n);
 
 	mutex_lock(&nh->mutex);
-	ret = notifier_chain_register(&nh->head, n);
+	ret = notifier_chain_cond_register(&nh->head, n);
 	mutex_unlock(&nh->mutex);
 	return ret;
 }
-- 
1.8.5.6

Re: [PATCH] kernel/notifier.c: remove notifier_chain_register

[Andrew Morton]

On Thu, 13 Jun 2019 22:07:44 +0800 Xiaoming Ni <nixiaoming@huawei.com> wrote:

> Registering the same notifier to a hook repeatedly can cause the hook
> list to form a ring or lose other members of the list.
> 
> case1: An infinite loop in notifier_chain_register can cause soft lockup
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier2);
> 
> case2: An infinite loop in notifier_chain_register can cause soft lockup
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
> 	atomic_notifier_call_chain(&test_notifier_list, 0, NULL);
> 
> case3: lose other hook "test_notifier2"
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier2);
> 	atomic_notifier_chain_register(&test_notifier_list, &test_notifier1);
> 
> case4: Unregister returns 0, but the hook is still in the linked list,
> 	and it is not really registered. If you call notifier_call_chain
> 	after ko is unloaded, it will trigger oops.
> 
> If the system is configured with softlockup_panic and the same
> hook is repeatedly registered on the panic_notifier_list, it
> will cause a loop panic.
> 
> The only difference between notifier_chain_cond_register and
> notifier_chain_register is that a check is added in order to
> avoid registering the same notifier multiple times to the same hook.
> So consider removing notifier_chain_register and replacing it
> with notifier_chain_cond_register.
>
> ...
> 
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index d9f5081..56efd54 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -19,20 +19,6 @@
>   *	are layered on top of these, with appropriate locking added.
>   */
>  
> -static int notifier_chain_register(struct notifier_block **nl,
> -		struct notifier_block *n)
> -{
> -	while ((*nl) != NULL) {
> -		WARN_ONCE(((*nl) == n), "double register detected");
> -		if (n->priority > (*nl)->priority)
> -			break;
> -		nl = &((*nl)->next);
> -	}
> -	n->next = *nl;
> -	rcu_assign_pointer(*nl, n);
> -	return 0;
> -}

Registering an already-registered notifier is a bug (except for in
net/sunrpc/rpc_pipe.c, apparently).  The effect of this change is to
remove the warning about the presence of the bug, so the bug is less
likely to get fixed.

I think it would be better to remove notifier_chain_cond_register() and
blocking_notifier_chain_cond_register() and to figure out why
net/sunrpc/rpc_pipe.c is using it and to redo the rpc code so it no
longer has that need.

RE: [PATCH] kernel/notifier.c: remove notifier_chain_register

[Nixiaoming]

On Fri, 14 Jun 2019 03:38 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>On Thu, 13 Jun 2019 22:07:44 +0800 Xiaoming Ni <nixiaoming@huawei.com> wrote:
>
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>> .....
>> 
>> diff --git a/kernel/notifier.c b/kernel/notifier.c
>> index d9f5081..56efd54 100644
>> --- a/kernel/notifier.c
>> +++ b/kernel/notifier.c
>> @@ -19,20 +19,6 @@
>>   *	are layered on top of these, with appropriate locking added.
>>   */
>>  
>> -static int notifier_chain_register(struct notifier_block **nl,
>> -		struct notifier_block *n)
>> -{
>> -	while ((*nl) != NULL) {
>> -		WARN_ONCE(((*nl) == n), "double register detected");
>> -		if (n->priority > (*nl)->priority)
>> -			break;
>> -		nl = &((*nl)->next);
>> -	}
>> -	n->next = *nl;
>> -	rcu_assign_pointer(*nl, n);
>> -	return 0;
>> -}
>
>Registering an already-registered notifier is a bug (except for in
>net/sunrpc/rpc_pipe.c, apparently).  The effect of this change is to
>remove the warning about the presence of the bug, so the bug is less
>likely to get fixed.
>
thanks for your guidance,

Should I modify this way 
   1 notifier_chain_cond_register() and notifier_chain_register() should be combined into one function.
   2 The warning information needs to be displayed while prohibiting duplicate registration.
		@@ -23,7 +23,10 @@ static int notifier_chain_register(struct notifier_block **nl,
						struct notifier_block *n)
		 {
				while ((*nl) != NULL) {
		-           WARN_ONCE(((*nl) == n), "double register detected");
		+         if (unlikely((*nl) == n)) {
		+                 WARN(1, "double register detected");
		+                 return 0;
		+         }
						if (n->priority > (*nl)->priority)
								break;

>I think it would be better to remove notifier_chain_cond_register() and
>blocking_notifier_chain_cond_register() and to figure out why
>net/sunrpc/rpc_pipe.c is using it and to redo the rpc code so it no
>longer has that need.
>
thanks for your guidance,
I re-examine the submission record and analyze it as follows

notifier_chain_cond_register() was introduced by commit 6546bc4279241e8fa43
 ("ipc: re-enable msgmni automatic recomputing msgmni if ​​set to negative")
From the patch description information, it should be done to avoid repeated registrations,
 but I don't know why not directly modify notifier_chain_cond_register().
 
notifier_chain_cond_register() is only called by blocking_notifier_chain_cond_register()
blocking_notifier_chain_cond_register() has less processing of the SYSTEM_BOOTING state 
than blocking_notifier_chain_egister().
may also be a bug.

ipc/ipcns_notifier.c and the call to blocking_notifier_chain_cond_register() are removed 
in commit 0050ee059f7fc86b1df252 ("ipc/msg: increase MSGMNI, remove scaling").

now blocking_notifier_chain_cond_register() is only used in net/sunrpc/rpc_pipe.c, 
commit 2d00131acc641b2cb6 ("SUNRPC: send notification events on pipefs sb creation and destruction")
Using blocking_notifier_chain_cond_register() may also be to avoid duplicate registrations??

thanks