linux-kernel - Re: [PATCH] kernel/notifier.c: remove notifier_chain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20190613123823.bf75e7305e22dd1dcab04fb8@linux-foundation.org>
Date:   Thu, 13 Jun 2019 12:38:23 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Xiaoming Ni <nixiaoming@...wei.com>
Cc:     <vvs@...tuozzo.com>, <adobriyan@...ru>, <adobriyan@...il.com>,
        <tglx@...utronix.de>, <gregkh@...uxfoundation.org>,
        <mingo@...nel.org>, <viresh.kumar@...aro.org>, <luto@...nel.org>,
        <arjan@...ux.intel.com>, <Nadia.Derbey@...l.net>,
        <linux-kernel@...r.kernel.org>, <torvalds@...ux-foundation.org>,
        <stern@...land.harvard.edu>, <paulmck@...ux.vnet.ibm.com>,
        <masami.hiramatsu.pt@...achi.com>, <alex.huangjianhui@...wei.com>,
        <dylix.dailei@...wei.com>,
        Stanislav Kinsbursky <skinsbursky@...allels.com>,
        Trond Myklebust <Trond.Myklebust@...app.com>
Subject: Re: [PATCH] kernel/notifier.c: remove notifier_chain_register

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