[Devel] net/core/flow.c: Fix CPU hotplug callback registration

Submitted by Stanislav Kinsburskiy on July 18, 2017, 1:51 p.m.

Details

Message ID 20170718135145.16777.86748.stgit@localhost.localdomain
State New
Series "net/core/flow.c: Fix CPU hotplug callback registration"
Headers show

Commit Message

Stanislav Kinsburskiy July 18, 2017, 1:51 p.m.
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Backport of upstream commit: e30a293e8ad7e6048d6d88bcc114094f964bd67b

https://jira.sw.ru/browse/PSBM-68620

Fixes race between CPU add/del and per-cpu notifier add/del.

Below is original commit message:

Patch hide | download patch | download mbox

=========================================================================

Subsystems that want to register CPU hotplug callbacks, as well as perform
initialization for the CPUs that are already online, often do it as shown
below:

	get_online_cpus();

	for_each_online_cpu(cpu)
		init_cpu(cpu);

	register_cpu_notifier(&foobar_cpu_notifier);

	put_online_cpus();

This is wrong, since it is prone to ABBA deadlocks involving the
cpu_add_remove_lock and the cpu_hotplug.lock (when running concurrently
with CPU hotplug operations).

Instead, the correct and race-free way of performing the callback
registration is:

	cpu_notifier_register_begin();

	for_each_online_cpu(cpu)
		init_cpu(cpu);

	/* Note the use of the double underscored version of the API */
	__register_cpu_notifier(&foobar_cpu_notifier);

	cpu_notifier_register_done();

Fix the code in net/core/flow.c by using this latter form of callback
registration.



Cc: Li RongQing <roy.qing.li@gmail.com>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Ingo Molnar <mingo@kernel.org>
Acked-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
---
 net/core/flow.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/core/flow.c b/net/core/flow.c
index e2664a2..20b3ac3 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -450,6 +450,8 @@  int flow_cache_init(struct net *net)
 	if (!fc->percpu)
 		return -ENOMEM;
 
+	cpu_notifier_register_begin();
+
 	for_each_online_cpu(i) {
 		if (flow_cache_cpu_prepare(fc, i))
 			goto err;
@@ -457,7 +459,9 @@  int flow_cache_init(struct net *net)
 	fc->hotcpu_notifier = (struct notifier_block){
 		.notifier_call = flow_cache_cpu,
 	};
-	register_hotcpu_notifier(&fc->hotcpu_notifier);
+	__register_hotcpu_notifier(&fc->hotcpu_notifier);
+
+	cpu_notifier_register_done();
 
 	setup_timer(&fc->rnd_timer, flow_cache_new_hashrnd,
 		    (unsigned long) fc);
@@ -473,6 +477,8 @@  int flow_cache_init(struct net *net)
 		fcp->hash_table = NULL;
 	}
 
+	cpu_notifier_register_done();
+
 	free_percpu(fc->percpu);
 	fc->percpu = NULL;
 

Comments

Kirill Tkhai July 18, 2017, 2:55 p.m.
On 18.07.2017 16:51, Stanislav Kinsburskiy wrote:
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> Backport of upstream commit: e30a293e8ad7e6048d6d88bcc114094f964bd67b
> 
> https://jira.sw.ru/browse/PSBM-68620
> 
> Fixes race between CPU add/del and per-cpu notifier add/del.
> 
> Below is original commit message:
> 
> =========================================================================
> 
> Subsystems that want to register CPU hotplug callbacks, as well as perform
> initialization for the CPUs that are already online, often do it as shown
> below:
> 
> 	get_online_cpus();
> 
> 	for_each_online_cpu(cpu)
> 		init_cpu(cpu);
> 
> 	register_cpu_notifier(&foobar_cpu_notifier);
> 
> 	put_online_cpus();
> 
> This is wrong, since it is prone to ABBA deadlocks involving the
> cpu_add_remove_lock and the cpu_hotplug.lock (when running concurrently
> with CPU hotplug operations).
> 
> Instead, the correct and race-free way of performing the callback
> registration is:
> 
> 	cpu_notifier_register_begin();
> 
> 	for_each_online_cpu(cpu)
> 		init_cpu(cpu);
> 
> 	/* Note the use of the double underscored version of the API */
> 	__register_cpu_notifier(&foobar_cpu_notifier);
> 
> 	cpu_notifier_register_done();
> 
> Fix the code in net/core/flow.c by using this latter form of callback
> registration.

Just for clearness.
The original description confuses, as it's possible to think we're fixing deadlock here.
The patch is about: "We iterate over online cpu list, but we have not prohibited its
modifications before. Fix that".

> Cc: Li RongQing <roy.qing.li@gmail.com>
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Chris Metcalf <cmetcalf@tilera.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Acked-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  net/core/flow.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/flow.c b/net/core/flow.c
> index e2664a2..20b3ac3 100644
> --- a/net/core/flow.c
> +++ b/net/core/flow.c
> @@ -450,6 +450,8 @@ int flow_cache_init(struct net *net)
>  	if (!fc->percpu)
>  		return -ENOMEM;
>  
> +	cpu_notifier_register_begin();
> +
>  	for_each_online_cpu(i) {
>  		if (flow_cache_cpu_prepare(fc, i))
>  			goto err;
> @@ -457,7 +459,9 @@ int flow_cache_init(struct net *net)
>  	fc->hotcpu_notifier = (struct notifier_block){
>  		.notifier_call = flow_cache_cpu,
>  	};
> -	register_hotcpu_notifier(&fc->hotcpu_notifier);
> +	__register_hotcpu_notifier(&fc->hotcpu_notifier);
> +
> +	cpu_notifier_register_done();
>  
>  	setup_timer(&fc->rnd_timer, flow_cache_new_hashrnd,
>  		    (unsigned long) fc);
> @@ -473,6 +477,8 @@ int flow_cache_init(struct net *net)
>  		fcp->hash_table = NULL;
>  	}
>  
> +	cpu_notifier_register_done();
> +
>  	free_percpu(fc->percpu);
>  	fc->percpu = NULL;
>  
>