Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock
@ 2019-08-09 19:28 Daniel Jordan
  2019-08-09 19:28 ` [PATCH 2/2] padata: validate cpumask without removed CPU during offline Daniel Jordan
  2019-08-15  5:15 ` [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock Herbert Xu
  0 siblings, 2 replies; 5+ messages in thread
From: Daniel Jordan @ 2019-08-09 19:28 UTC (permalink / raw)
  To: Herbert Xu, Steffen Klassert; +Cc: Daniel Jordan, linux-crypto, linux-kernel

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(-)

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] padata: validate cpumask without removed CPU during offline
  2019-08-09 19:28 [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock Daniel Jordan
@ 2019-08-09 19:28 ` Daniel Jordan
  2019-08-09 21:06   ` [PATCH v2] " Daniel Jordan
  2019-08-15  5:15 ` [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock Herbert Xu
  1 sibling, 1 reply; 5+ messages in thread
From: Daniel Jordan @ 2019-08-09 19:28 UTC (permalink / raw)
  To: Herbert Xu, Steffen Klassert; +Cc: Daniel Jordan, linux-crypto, linux-kernel

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 <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
---
 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] padata: validate cpumask without removed CPU during offline
  2019-08-09 19:28 ` [PATCH 2/2] padata: validate cpumask without removed CPU during offline Daniel Jordan
@ 2019-08-09 21:06   ` " Daniel Jordan
  2019-08-12 21:02     ` [PATCH 3/2] padata: initialize usable masks to reflect offlined CPU Daniel Jordan
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Jordan @ 2019-08-09 21:06 UTC (permalink / raw)
  To: Herbert Xu, Steffen Klassert; +Cc: Daniel Jordan, linux-crypto, linux-kernel

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.

Stop the instance unconditionally and start again if the masks are
valid.  Stopping the instance only after an invalid mask is found risks
this div-by-0 crash since a padata_do_parallel call in another task
could happen between cpumask_clear_cpu and padata_validate_cpumask.

Fixes: 33e54450683c ("padata: Handle empty padata cpumasks")
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
---

v2: Don't leave the instance stopped if the masks are valid.

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

diff --git a/kernel/padata.c b/kernel/padata.c
index d056276a96ce..01460ea1d160 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,9 @@ 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))
+			__padata_start(pinst);
 	}
 
 	return 0;
-- 
2.22.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/2] padata: initialize usable masks to reflect offlined CPU
  2019-08-09 21:06   ` [PATCH v2] " Daniel Jordan
@ 2019-08-12 21:02     ` Daniel Jordan
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Jordan @ 2019-08-12 21:02 UTC (permalink / raw)
  To: Herbert Xu, Steffen Klassert; +Cc: Daniel Jordan, linux-crypto, linux-kernel

__padata_remove_cpu clears the offlined CPU from the usable masks after
padata_alloc_pd has initialized pd->cpu, which means pd->cpu could be
initialized to this CPU, causing padata to wait indefinitely for the
next job in padata_get_next.

Make the usable masks reflect the offline CPU when they're established
in padata_setup_cpumasks so pd->cpu is initialized properly.

Fixes: 6fc4dbcf0276 ("padata: Replace delayed timer with immediate workqueue in padata_reorder")
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
---

Hi, one more edge case.  All combinations of CPUs among
parallel_cpumask, serial_cpumask, and CPU hotplug have now been tested
in a 4-CPU VM, and an 8-CPU VM has run with random combinations of these
settings for over an hour.

 kernel/padata.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index 01460ea1d160..c1002ac4720c 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -702,17 +702,27 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
 	struct parallel_data *pd = NULL;
 
 	if (cpumask_test_cpu(cpu, cpu_online_mask)) {
+		cpumask_var_t pcpu, cbcpu;
+
 		__padata_stop(pinst);
 
-		pd = padata_alloc_pd(pinst, pinst->cpumask.pcpu,
-				     pinst->cpumask.cbcpu);
+		/*
+		 * padata_alloc_pd uses cpu_online_mask to get the usable
+		 * masks, but @cpu hasn't been removed from it yet, so use
+		 * temporary masks that exclude @cpu so the usable masks show
+		 * @cpu as offline for pd->cpu's initialization.
+		 */
+		cpumask_copy(pcpu, pinst->cpumask.pcpu);
+		cpumask_copy(cbcpu, pinst->cpumask.cbcpu);
+		cpumask_clear_cpu(cpu, cbcpu);
+		cpumask_clear_cpu(cpu, pcpu);
+
+		pd = padata_alloc_pd(pinst, pcpu, cbcpu);
 		if (!pd)
 			return -ENOMEM;
 
 		padata_replace(pinst, pd);
 
-		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))
 			__padata_start(pinst);
-- 
2.22.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock
  2019-08-09 19:28 [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock Daniel Jordan
  2019-08-09 19:28 ` [PATCH 2/2] padata: validate cpumask without removed CPU during offline Daniel Jordan
@ 2019-08-15  5:15 ` Herbert Xu
  1 sibling, 0 replies; 5+ messages in thread
From: Herbert Xu @ 2019-08-15  5:15 UTC (permalink / raw)
  To: Daniel Jordan; +Cc: Steffen Klassert, linux-crypto, linux-kernel

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 19:28 [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock Daniel Jordan
2019-08-09 19:28 ` [PATCH 2/2] padata: validate cpumask without removed CPU during offline Daniel Jordan
2019-08-09 21:06   ` [PATCH v2] " Daniel Jordan
2019-08-12 21:02     ` [PATCH 3/2] padata: initialize usable masks to reflect offlined CPU Daniel Jordan
2019-08-15  5:15 ` [PATCH 1/2] padata: always acquire cpu_hotplug_lock before pinst->lock Herbert Xu

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org linux-crypto@archiver.kernel.org
	public-inbox-index linux-crypto


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/ public-inbox