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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sun, 16 Jun 2019 13:56:47 +0000
From:   Nixiaoming <nixiaoming@...wei.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        "skinsbursky@...allels.com" <skinsbursky@...allels.com>
CC:     "vvs@...tuozzo.com" <vvs@...tuozzo.com>,
        "adobriyan@...il.com" <adobriyan@...il.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "arjan@...ux.intel.com" <arjan@...ux.intel.com>,
        "Nadia.Derbey@...l.net" <Nadia.Derbey@...l.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "paulmck@...ux.vnet.ibm.com" <paulmck@...ux.vnet.ibm.com>,
        "Huangjianhui (Alex)" <alex.huangjianhui@...wei.com>,
        Dailei <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 Fri, 14 Jun 2019 03:38 AM Andrew Morton <akpm@...ux-foundation.org> wrote:
>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.
>> .....
>> 
>> 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