[1/2] padata: always acquire cpu_hotplug_lock before pinst->lock
diff mbox series

Message ID 20190809192857.26585-1-daniel.m.jordan@oracle.com
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series
  • [1/2] padata: always acquire cpu_hotplug_lock before pinst->lock
Related show

Commit Message

Daniel Jordan Aug. 9, 2019, 7:28 p.m. UTC
On a 5.2 kernel, lockdep complains when offlining a CPU and writing to a
parallel_cpumask sysfs file.

  echo 0 > /sys/devices/system/cpu/cpu1/online
  echo ff > /sys/kernel/pcrypt/pencrypt/parallel_cpumask

  ======================================================
  WARNING: possible circular locking dependency detected
  5.2.0-padata-base+ #19 Not tainted
  ------------------------------------------------------
  cpuhp/1/13 is trying to acquire lock:
  ...  (&pinst->lock){+.+.}, at: padata_cpu_prep_down+0x37/0x70

  but task is already holding lock:
  ...  (cpuhp_state-down){+.+.}, at: cpuhp_thread_fun+0x34/0x240

  which lock already depends on the new lock.

padata doesn't take cpu_hotplug_lock and pinst->lock in a consistent
order.  Which should be first?  CPU hotplug calls into padata with
cpu_hotplug_lock already held, so it should have priority.

Remove the cpu_hotplug_lock acquisition from __padata_stop and hoist it
up to padata_stop, before pd->lock is taken.  That fixes a
recursive acquisition of cpu_hotplug_lock in padata_remove_cpu at the
same time:

  padata_remove_cpu
    mutex_lock(&pinst->lock)
    get_online_cpus()
    __padata_remove_cpu
      __padata_stop
        get_online_cpus()

The rest is just switching the order where the two locks are taken
together.

Fixes: 6751fb3c0e0c ("padata: Use get_online_cpus/put_online_cpus")
Fixes: 65ff577e6b6e ("padata: Rearrange set_cpumask functions")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

Hello, these two patches are based on all padata fixes now in cryptodev-2.6.

 kernel/padata.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Herbert Xu Aug. 15, 2019, 5:15 a.m. UTC | #1
On Fri, Aug 09, 2019 at 03:28:56PM -0400, Daniel Jordan wrote:
> On a 5.2 kernel, lockdep complains when offlining a CPU and writing to a
> parallel_cpumask sysfs file.
> 
>   echo 0 > /sys/devices/system/cpu/cpu1/online
>   echo ff > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
> 
>   ======================================================
>   WARNING: possible circular locking dependency detected
>   5.2.0-padata-base+ #19 Not tainted
>   ------------------------------------------------------
>   cpuhp/1/13 is trying to acquire lock:
>   ...  (&pinst->lock){+.+.}, at: padata_cpu_prep_down+0x37/0x70
> 
>   but task is already holding lock:
>   ...  (cpuhp_state-down){+.+.}, at: cpuhp_thread_fun+0x34/0x240
> 
>   which lock already depends on the new lock.
> 
> padata doesn't take cpu_hotplug_lock and pinst->lock in a consistent
> order.  Which should be first?  CPU hotplug calls into padata with
> cpu_hotplug_lock already held, so it should have priority.

Yeah this is clearly a bug but I think we need tackle something
else first.
 
> diff --git a/kernel/padata.c b/kernel/padata.c
> index b60cc3dcee58..d056276a96ce 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -487,9 +487,7 @@ static void __padata_stop(struct padata_instance *pinst)
>  
>  	synchronize_rcu();
>  
> -	get_online_cpus();
>  	padata_flush_queues(pinst->pd);
> -	put_online_cpus();
>  }

As I pointed earlier, the whole concept of flushing the queues is
suspect.  So we should tackle that first and it may obviate the need
to do get_online_cpus completely if the flush call disappears.

My main worry is that you're adding an extra lock around synchronize_rcu
and that is always something that should be done only after careful
investigation.

Cheers,
Daniel Jordan Aug. 21, 2019, 4:14 a.m. UTC | #2
[sorry for late reply, moved to new place in past week]

On 8/15/19 1:15 AM, Herbert Xu wrote:
> On Fri, Aug 09, 2019 at 03:28:56PM -0400, Daniel Jordan wrote:
>> padata doesn't take cpu_hotplug_lock and pinst->lock in a consistent
>> order.  Which should be first?  CPU hotplug calls into padata with
>> cpu_hotplug_lock already held, so it should have priority.
> 
> Yeah this is clearly a bug but I think we need tackle something
> else first.
>   
>> diff --git a/kernel/padata.c b/kernel/padata.c
>> index b60cc3dcee58..d056276a96ce 100644
>> --- a/kernel/padata.c
>> +++ b/kernel/padata.c
>> @@ -487,9 +487,7 @@ static void __padata_stop(struct padata_instance *pinst)
>>   
>>   	synchronize_rcu();
>>   
>> -	get_online_cpus();
>>   	padata_flush_queues(pinst->pd);
>> -	put_online_cpus();
>>   }
> 
> As I pointed earlier, the whole concept of flushing the queues is
> suspect.  So we should tackle that first and it may obviate the need
> to do get_online_cpus completely if the flush call disappears.
>
> My main worry is that you're adding an extra lock around synchronize_rcu
> and that is always something that should be done only after careful
> investigation.

Agreed, padata_stop may not need to do get_online_cpus() if we stop an instance in a way that plays well with async crypto.

I'll try fixing the flushing with Steffen's refcounting idea assuming he hasn't already started on that.  So we're on the same page, the problem is that if padata's ->parallel() punts to a cryptd thread, flushing the parallel work will return immediately without necessarily indicating the parallel job is finished, so flushing is pointless and padata_replace needs to wait till the instance's refcount drops to 0.  Did I get it right?

Daniel
Herbert Xu Aug. 21, 2019, 6:43 a.m. UTC | #3
On Wed, Aug 21, 2019 at 12:14:19AM -0400, Daniel Jordan wrote:
>
> I'll try fixing the flushing with Steffen's refcounting idea assuming he hasn't already started on that.  So we're on the same page, the problem is that if padata's ->parallel() punts to a cryptd thread, flushing the parallel work will return immediately without necessarily indicating the parallel job is finished, so flushing is pointless and padata_replace needs to wait till the instance's refcount drops to 0.  Did I get it right?

Yeah you can never flush an async crypto job.  You have to wait
for it to finish.

Cheers,

Patch
diff mbox series

diff --git a/kernel/padata.c b/kernel/padata.c
index b60cc3dcee58..d056276a96ce 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -487,9 +487,7 @@  static void __padata_stop(struct padata_instance *pinst)
 
 	synchronize_rcu();
 
-	get_online_cpus();
 	padata_flush_queues(pinst->pd);
-	put_online_cpus();
 }
 
 /* Replace the internal control structure with a new one. */
@@ -614,8 +612,8 @@  int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 	struct cpumask *serial_mask, *parallel_mask;
 	int err = -EINVAL;
 
-	mutex_lock(&pinst->lock);
 	get_online_cpus();
+	mutex_lock(&pinst->lock);
 
 	switch (cpumask_type) {
 	case PADATA_CPU_PARALLEL:
@@ -633,8 +631,8 @@  int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 	err =  __padata_set_cpumasks(pinst, parallel_mask, serial_mask);
 
 out:
-	put_online_cpus();
 	mutex_unlock(&pinst->lock);
+	put_online_cpus();
 
 	return err;
 }
@@ -669,9 +667,11 @@  EXPORT_SYMBOL(padata_start);
  */
 void padata_stop(struct padata_instance *pinst)
 {
+	get_online_cpus();
 	mutex_lock(&pinst->lock);
 	__padata_stop(pinst);
 	mutex_unlock(&pinst->lock);
+	put_online_cpus();
 }
 EXPORT_SYMBOL(padata_stop);
 
@@ -739,18 +739,18 @@  int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask)
 	if (!(mask & (PADATA_CPU_SERIAL | PADATA_CPU_PARALLEL)))
 		return -EINVAL;
 
+	get_online_cpus();
 	mutex_lock(&pinst->lock);
 
-	get_online_cpus();
 	if (mask & PADATA_CPU_SERIAL)
 		cpumask_clear_cpu(cpu, pinst->cpumask.cbcpu);
 	if (mask & PADATA_CPU_PARALLEL)
 		cpumask_clear_cpu(cpu, pinst->cpumask.pcpu);
 
 	err = __padata_remove_cpu(pinst, cpu);
-	put_online_cpus();
 
 	mutex_unlock(&pinst->lock);
+	put_online_cpus();
 
 	return err;
 }