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

[Andrew Morton]

On Thu, 13 Jun 2019 22:07:44 +0800 Xiaoming Ni <nixiaoming@xxxxxxxxxx> 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.