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 <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Steffen Klassert <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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(-)
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;
}
--
2.22.0
Configuring an instance's parallel mask without any online CPUs...
echo 2 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
echo 0 > /sys/devices/system/cpu/cpu1/online
...crashes like this:
divide error: 0000 [#1] SMP PTI
CPU: 4 PID: 281 Comm: modprobe Not tainted 5.2.0-padata-base+ #25
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-<snip>
RIP: 0010:padata_do_parallel+0xf1/0x270
...
Call Trace:
pcrypt_do_parallel+0xed/0x1e0 [pcrypt]
pcrypt_aead_encrypt+0xbf/0xd0 [pcrypt]
do_mult_aead_op+0x68/0x112 [tcrypt]
test_mb_aead_speed.constprop.0.cold+0x21a/0x55a [tcrypt]
do_test+0x2280/0x4ca2 [tcrypt]
tcrypt_mod_init+0x55/0x1000 [tcrypt]
...
The cpumask_weight call in padata_cpu_hash returns 0, causing the
division error, because the mask has no CPUs, which is expected in this
situation. The problem is __padata_remove_cpu doesn't mark the instance
PADATA_INVALID as expected, which would have made padata_do_parallel
return error before doing the division, because it checks for valid
masks too early.
Fix by moving the checks after the masks have been adjusted for the
offlined CPU. Only do the second check if the first succeeded to avoid
inadvertently clearing PADATA_INVALID.
Fixes: 33e54450683c ("padata: Handle empty padata cpumasks")
Signed-off-by: Daniel Jordan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Steffen Klassert <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
kernel/padata.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index d056276a96ce..2ab3b7402643 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -702,10 +702,7 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
struct parallel_data *pd = NULL;
if (cpumask_test_cpu(cpu, cpu_online_mask)) {
-
- if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
- !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
- __padata_stop(pinst);
+ __padata_stop(pinst);
pd = padata_alloc_pd(pinst, pinst->cpumask.pcpu,
pinst->cpumask.cbcpu);
@@ -716,6 +713,8 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
cpumask_clear_cpu(cpu, pd->cpumask.cbcpu);
cpumask_clear_cpu(cpu, pd->cpumask.pcpu);
+ if (padata_validate_cpumask(pinst, pd->cpumask.pcpu))
+ padata_validate_cpumask(pinst, pd->cpumask.cbcpu);
}
return 0;
--
2.22.0