* [PATCH v2 1/5] padata: make flushing work with async users
2019-08-28 22:14 [PATCH v2 0/5] padata flushing and CPU hotplug fixes Daniel Jordan
@ 2019-08-28 22:14 ` Daniel Jordan
2019-09-05 4:17 ` Herbert Xu
2019-08-28 22:14 ` [PATCH v2 2/5] padata: remove reorder_objects Daniel Jordan
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Daniel Jordan @ 2019-08-28 22:14 UTC (permalink / raw)
To: Herbert Xu, Steffen Klassert
Cc: Sebastian Andrzej Siewior, Thomas Gleixner, linux-crypto,
linux-kernel, Daniel Jordan
padata_flush_queues() is broken for an async ->parallel() function
because flush_work() can't wait on it:
# modprobe tcrypt alg="pcrypt(cryptd(rfc4106(gcm_base(ctr(aes-generic),ghash-generic))))" type=3
# modprobe tcrypt mode=215 sec=1 &
# sleep 5; echo 7 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
kernel BUG at kernel/padata.c:473!
invalid opcode: 0000 [#1] SMP PTI
CPU: 0 PID: 282 Comm: sh Not tainted 5.3.0-rc5-padata-base+ #3
RIP: 0010:padata_flush_queues+0xa7/0xb0
Call Trace:
padata_replace+0xa1/0x110
padata_set_cpumask+0xae/0x130
store_cpumask+0x75/0xa0
padata_sysfs_store+0x20/0x30
...
Wait instead for the parallel_data (pd) object's refcount to drop to
zero.
Simplify by taking an initial reference on a pd when allocating an
instance. That ref is dropped during flushing, which allows calling
wait_for_completion() unconditionally.
If the initial ref weren't used, the only other alternative I could
think of is that complete() would be conditional on !PADATA_INIT or
PADATA_REPLACE (in addition to zero pd->refcnt), and
wait_for_completion() on nonzero pd->refcnt. But then complete() could
be called without a matching wait:
completer waiter
--------- ------
DEC pd->refcnt // 0
pinst->flags |= PADATA_REPLACE
LOAD pinst->flags // REPLACE
LOAD pd->refcnt // 0
/* return without wait_for_completion() */
complete()
No more flushing per-CPU work items, so no more CPU hotplug lock in
__padata_stop.
Fixes: 2b73b07ab8a4 ("padata: Flush the padata queues actively")
Reported-by: Herbert Xu <herbert@gondor.apana.org.au>
Suggested-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
include/linux/padata.h | 3 +++
kernel/padata.c | 32 ++++++++++++--------------------
2 files changed, 15 insertions(+), 20 deletions(-)
diff --git a/include/linux/padata.h b/include/linux/padata.h
index 8da673861d99..1c73f9cc7b72 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -14,6 +14,7 @@
#include <linux/list.h>
#include <linux/notifier.h>
#include <linux/kobject.h>
+#include <linux/completion.h>
#define PADATA_CPU_SERIAL 0x01
#define PADATA_CPU_PARALLEL 0x02
@@ -104,6 +105,7 @@ struct padata_cpumask {
* @squeue: percpu padata queues used for serialuzation.
* @reorder_objects: Number of objects waiting in the reorder queues.
* @refcnt: Number of objects holding a reference on this parallel_data.
+ * @flushing_done: Wait for all objects to be sent out.
* @max_seq_nr: Maximal used sequence number.
* @cpu: Next CPU to be processed.
* @cpumask: The cpumasks in use for parallel and serial workers.
@@ -116,6 +118,7 @@ struct parallel_data {
struct padata_serial_queue __percpu *squeue;
atomic_t reorder_objects;
atomic_t refcnt;
+ struct completion flushing_done;
atomic_t seq_nr;
int cpu;
struct padata_cpumask cpumask;
diff --git a/kernel/padata.c b/kernel/padata.c
index b60cc3dcee58..958166e23123 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -301,7 +301,8 @@ static void padata_serial_worker(struct work_struct *serial_work)
list_del_init(&padata->list);
padata->serial(padata);
- atomic_dec(&pd->refcnt);
+ if (atomic_dec_return(&pd->refcnt) == 0)
+ complete(&pd->flushing_done);
}
local_bh_enable();
}
@@ -423,7 +424,9 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
padata_init_squeues(pd);
atomic_set(&pd->seq_nr, -1);
atomic_set(&pd->reorder_objects, 0);
- atomic_set(&pd->refcnt, 0);
+ /* Initial ref dropped in padata_flush_queues. */
+ atomic_set(&pd->refcnt, 1);
+ init_completion(&pd->flushing_done);
pd->pinst = pinst;
spin_lock_init(&pd->lock);
pd->cpu = cpumask_first(pd->cpumask.pcpu);
@@ -453,24 +456,15 @@ static void padata_free_pd(struct parallel_data *pd)
/* Flush all objects out of the padata queues. */
static void padata_flush_queues(struct parallel_data *pd)
{
- int cpu;
- struct padata_parallel_queue *pqueue;
- struct padata_serial_queue *squeue;
-
- for_each_cpu(cpu, pd->cpumask.pcpu) {
- pqueue = per_cpu_ptr(pd->pqueue, cpu);
- flush_work(&pqueue->work);
- }
-
- if (atomic_read(&pd->reorder_objects))
- padata_reorder(pd);
+ if (!(pd->pinst->flags & PADATA_INIT))
+ return;
- for_each_cpu(cpu, pd->cpumask.cbcpu) {
- squeue = per_cpu_ptr(pd->squeue, cpu);
- flush_work(&squeue->work);
- }
+ if (atomic_dec_return(&pd->refcnt) == 0)
+ complete(&pd->flushing_done);
- BUG_ON(atomic_read(&pd->refcnt) != 0);
+ wait_for_completion(&pd->flushing_done);
+ reinit_completion(&pd->flushing_done);
+ atomic_set(&pd->refcnt, 1);
}
static void __padata_start(struct padata_instance *pinst)
@@ -487,9 +481,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. */
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/5] padata: make flushing work with async users
2019-08-28 22:14 ` [PATCH v2 1/5] padata: make flushing work with async users Daniel Jordan
@ 2019-09-05 4:17 ` Herbert Xu
2019-09-05 22:37 ` Daniel Jordan
0 siblings, 1 reply; 9+ messages in thread
From: Herbert Xu @ 2019-09-05 4:17 UTC (permalink / raw)
To: Daniel Jordan
Cc: Steffen Klassert, Sebastian Andrzej Siewior, Thomas Gleixner,
linux-crypto, linux-kernel
On Wed, Aug 28, 2019 at 06:14:21PM -0400, Daniel Jordan wrote:
>
> @@ -453,24 +456,15 @@ static void padata_free_pd(struct parallel_data *pd)
> /* Flush all objects out of the padata queues. */
> static void padata_flush_queues(struct parallel_data *pd)
> {
> - int cpu;
> - struct padata_parallel_queue *pqueue;
> - struct padata_serial_queue *squeue;
> -
> - for_each_cpu(cpu, pd->cpumask.pcpu) {
> - pqueue = per_cpu_ptr(pd->pqueue, cpu);
> - flush_work(&pqueue->work);
> - }
> -
> - if (atomic_read(&pd->reorder_objects))
> - padata_reorder(pd);
> + if (!(pd->pinst->flags & PADATA_INIT))
> + return;
>
> - for_each_cpu(cpu, pd->cpumask.cbcpu) {
> - squeue = per_cpu_ptr(pd->squeue, cpu);
> - flush_work(&squeue->work);
> - }
> + if (atomic_dec_return(&pd->refcnt) == 0)
> + complete(&pd->flushing_done);
>
> - BUG_ON(atomic_read(&pd->refcnt) != 0);
> + wait_for_completion(&pd->flushing_done);
> + reinit_completion(&pd->flushing_done);
> + atomic_set(&pd->refcnt, 1);
> }
I don't think waiting is an option. In a pathological case the
hardware may not return at all. We cannot and should not hold off
CPU hotplug for an arbitrary amount of time when the event we are
waiting for isn't even occuring on that CPU.
I don't think flushing is needed at all. All we need to do is
maintain consistency before and after the CPU hotplug event.
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] 9+ messages in thread* Re: [PATCH v2 1/5] padata: make flushing work with async users
2019-09-05 4:17 ` Herbert Xu
@ 2019-09-05 22:37 ` Daniel Jordan
2019-09-18 20:37 ` Daniel Jordan
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Jordan @ 2019-09-05 22:37 UTC (permalink / raw)
To: Herbert Xu
Cc: Daniel Jordan, Steffen Klassert, Sebastian Andrzej Siewior,
Thomas Gleixner, linux-crypto, linux-kernel
On Thu, Sep 05, 2019 at 02:17:35PM +1000, Herbert Xu wrote:
> On Wed, Aug 28, 2019 at 06:14:21PM -0400, Daniel Jordan wrote:
> >
> > @@ -453,24 +456,15 @@ static void padata_free_pd(struct parallel_data *pd)
> > /* Flush all objects out of the padata queues. */
> > static void padata_flush_queues(struct parallel_data *pd)
> > {
> > - int cpu;
> > - struct padata_parallel_queue *pqueue;
> > - struct padata_serial_queue *squeue;
> > -
> > - for_each_cpu(cpu, pd->cpumask.pcpu) {
> > - pqueue = per_cpu_ptr(pd->pqueue, cpu);
> > - flush_work(&pqueue->work);
> > - }
> > -
> > - if (atomic_read(&pd->reorder_objects))
> > - padata_reorder(pd);
> > + if (!(pd->pinst->flags & PADATA_INIT))
> > + return;
> >
> > - for_each_cpu(cpu, pd->cpumask.cbcpu) {
> > - squeue = per_cpu_ptr(pd->squeue, cpu);
> > - flush_work(&squeue->work);
> > - }
> > + if (atomic_dec_return(&pd->refcnt) == 0)
> > + complete(&pd->flushing_done);
> >
> > - BUG_ON(atomic_read(&pd->refcnt) != 0);
> > + wait_for_completion(&pd->flushing_done);
> > + reinit_completion(&pd->flushing_done);
> > + atomic_set(&pd->refcnt, 1);
> > }
>
> I don't think waiting is an option. In a pathological case the
> hardware may not return at all. We cannot and should not hold off
> CPU hotplug for an arbitrary amount of time when the event we are
> waiting for isn't even occuring on that CPU.
Ok, I hadn't considered hardware not returning.
> I don't think flushing is needed at all. All we need to do is
> maintain consistency before and after the CPU hotplug event.
I could imagine not flushing would work for replacing a pd. The old pd could
be freed by whatever drops the last reference and the new pd could be
installed, all without flushing.
In the case of freeing an instance, though, padata needs to wait for all the
jobs to complete so they don't use the instance's data after it's been freed.
Holding the CPU hotplug lock isn't necessary for this, though, so I think we're
ok to wait here.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/5] padata: make flushing work with async users
2019-09-05 22:37 ` Daniel Jordan
@ 2019-09-18 20:37 ` Daniel Jordan
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Jordan @ 2019-09-18 20:37 UTC (permalink / raw)
To: Herbert Xu
Cc: Daniel Jordan, Steffen Klassert, Sebastian Andrzej Siewior,
Thomas Gleixner, linux-crypto, linux-kernel
On Thu, Sep 05, 2019 at 06:37:56PM -0400, Daniel Jordan wrote:
> On Thu, Sep 05, 2019 at 02:17:35PM +1000, Herbert Xu wrote:
> > I don't think waiting is an option. In a pathological case the
> > hardware may not return at all. We cannot and should not hold off
> > CPU hotplug for an arbitrary amount of time when the event we are
> > waiting for isn't even occuring on that CPU.
>
> Ok, I hadn't considered hardware not returning.
>
> > I don't think flushing is needed at all. All we need to do is
> > maintain consistency before and after the CPU hotplug event.
>
> I could imagine not flushing would work for replacing a pd. The old pd could
> be freed by whatever drops the last reference and the new pd could be
> installed, all without flushing.
>
> In the case of freeing an instance, though, padata needs to wait for all the
> jobs to complete so they don't use the instance's data after it's been freed.
> Holding the CPU hotplug lock isn't necessary for this, though, so I think we're
> ok to wait here.
[FYI, I'm currently on leave until mid-October and will return to this series
then.]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/5] padata: remove reorder_objects
2019-08-28 22:14 [PATCH v2 0/5] padata flushing and CPU hotplug fixes Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 1/5] padata: make flushing work with async users Daniel Jordan
@ 2019-08-28 22:14 ` Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 3/5] padata: get rid of padata_remove_cpu() for real Daniel Jordan
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Jordan @ 2019-08-28 22:14 UTC (permalink / raw)
To: Herbert Xu, Steffen Klassert
Cc: Sebastian Andrzej Siewior, Thomas Gleixner, linux-crypto,
linux-kernel, Daniel Jordan
reorder_objects is unused since the rework of padata's flushing, so
remove it.
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
include/linux/padata.h | 2 --
kernel/padata.c | 3 ---
2 files changed, 5 deletions(-)
diff --git a/include/linux/padata.h b/include/linux/padata.h
index 1c73f9cc7b72..720e970cda0a 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -103,7 +103,6 @@ struct padata_cpumask {
* @pinst: padata instance.
* @pqueue: percpu padata queues used for parallelization.
* @squeue: percpu padata queues used for serialuzation.
- * @reorder_objects: Number of objects waiting in the reorder queues.
* @refcnt: Number of objects holding a reference on this parallel_data.
* @flushing_done: Wait for all objects to be sent out.
* @max_seq_nr: Maximal used sequence number.
@@ -116,7 +115,6 @@ struct parallel_data {
struct padata_instance *pinst;
struct padata_parallel_queue __percpu *pqueue;
struct padata_serial_queue __percpu *squeue;
- atomic_t reorder_objects;
atomic_t refcnt;
struct completion flushing_done;
atomic_t seq_nr;
diff --git a/kernel/padata.c b/kernel/padata.c
index 958166e23123..2bfce01c5b85 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -179,7 +179,6 @@ static struct padata_priv *padata_get_next(struct parallel_data *pd)
struct padata_priv, list);
list_del_init(&padata->list);
- atomic_dec(&pd->reorder_objects);
pd->cpu = cpumask_next_wrap(cpu, pd->cpumask.pcpu, -1,
false);
@@ -323,7 +322,6 @@ void padata_do_serial(struct padata_priv *padata)
spin_lock(&pqueue->reorder.lock);
list_add_tail(&padata->list, &pqueue->reorder.list);
- atomic_inc(&pd->reorder_objects);
spin_unlock(&pqueue->reorder.lock);
/*
@@ -423,7 +421,6 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
padata_init_pqueues(pd);
padata_init_squeues(pd);
atomic_set(&pd->seq_nr, -1);
- atomic_set(&pd->reorder_objects, 0);
/* Initial ref dropped in padata_flush_queues. */
atomic_set(&pd->refcnt, 1);
init_completion(&pd->flushing_done);
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 3/5] padata: get rid of padata_remove_cpu() for real
2019-08-28 22:14 [PATCH v2 0/5] padata flushing and CPU hotplug fixes Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 1/5] padata: make flushing work with async users Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 2/5] padata: remove reorder_objects Daniel Jordan
@ 2019-08-28 22:14 ` Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 4/5] padata: always acquire cpu_hotplug_lock before pinst->lock Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 5/5] padata: validate cpumask without removed CPU during offline Daniel Jordan
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Jordan @ 2019-08-28 22:14 UTC (permalink / raw)
To: Herbert Xu, Steffen Klassert
Cc: Sebastian Andrzej Siewior, Thomas Gleixner, linux-crypto,
linux-kernel, Daniel Jordan, Jonathan Corbet, linux-doc
A later patch is going to address a lock ordering issue involving
pinst->mutex and the CPU hotplug lock. padata_remove_cpu() needs fixing
but it has no callers, so just delete it rather than maintaining unused
code. The Fixes commit forgot to do it anyway.
While at it remove Documentation references to other unused functions.
Fixes: 815613da6a67 ("kernel/padata.c: removed unused code")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Documentation/padata.txt | 18 ++++--------------
kernel/padata.c | 35 -----------------------------------
2 files changed, 4 insertions(+), 49 deletions(-)
diff --git a/Documentation/padata.txt b/Documentation/padata.txt
index b103d0c82000..43ca928da713 100644
--- a/Documentation/padata.txt
+++ b/Documentation/padata.txt
@@ -51,27 +51,17 @@ padata cpumask contains no active CPU (flag not set).
padata_stop clears the flag and blocks until the padata instance
is unused.
-The list of CPUs to be used can be adjusted with these functions::
+The list of CPUs to be used can be adjusted with this function::
- int padata_set_cpumasks(struct padata_instance *pinst,
- cpumask_var_t pcpumask,
- cpumask_var_t cbcpumask);
int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
cpumask_var_t cpumask);
- int padata_add_cpu(struct padata_instance *pinst, int cpu, int mask);
- int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask);
Changing the CPU masks are expensive operations, though, so it should not be
done with great frequency.
-It's possible to change both cpumasks of a padata instance with
-padata_set_cpumasks by specifying the cpumasks for parallel execution (pcpumask)
-and for the serial callback function (cbcpumask). padata_set_cpumask is used to
-change just one of the cpumasks. Here cpumask_type is one of PADATA_CPU_SERIAL,
-PADATA_CPU_PARALLEL and cpumask specifies the new cpumask to use.
-To simply add or remove one CPU from a certain cpumask the functions
-padata_add_cpu/padata_remove_cpu are used. cpu specifies the CPU to add or
-remove and mask is one of PADATA_CPU_SERIAL, PADATA_CPU_PARALLEL.
+padata_set_cpumask is used to change just one of the cpumasks. Here cpumask_type
+is one of PADATA_CPU_SERIAL or PADATA_CPU_PARALLEL, and cpumask specifies the
+new cpumask to use.
If a user is interested in padata cpumask changes, he can register to
the padata cpumask change notifier::
diff --git a/kernel/padata.c b/kernel/padata.c
index 2bfce01c5b85..6adce3b203fe 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -710,41 +710,6 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
return 0;
}
- /**
- * padata_remove_cpu - remove a cpu from the one or both(serial and parallel)
- * padata cpumasks.
- *
- * @pinst: padata instance
- * @cpu: cpu to remove
- * @mask: bitmask specifying from which cpumask @cpu should be removed
- * The @mask may be any combination of the following flags:
- * PADATA_CPU_SERIAL - serial cpumask
- * PADATA_CPU_PARALLEL - parallel cpumask
- */
-int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask)
-{
- int err;
-
- if (!(mask & (PADATA_CPU_SERIAL | PADATA_CPU_PARALLEL)))
- return -EINVAL;
-
- 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);
-
- return err;
-}
-EXPORT_SYMBOL(padata_remove_cpu);
-
static inline int pinst_has_cpu(struct padata_instance *pinst, int cpu)
{
return cpumask_test_cpu(cpu, pinst->cpumask.pcpu) ||
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 4/5] padata: always acquire cpu_hotplug_lock before pinst->lock
2019-08-28 22:14 [PATCH v2 0/5] padata flushing and CPU hotplug fixes Daniel Jordan
` (2 preceding siblings ...)
2019-08-28 22:14 ` [PATCH v2 3/5] padata: get rid of padata_remove_cpu() for real Daniel Jordan
@ 2019-08-28 22:14 ` Daniel Jordan
2019-08-28 22:14 ` [PATCH v2 5/5] padata: validate cpumask without removed CPU during offline Daniel Jordan
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Jordan @ 2019-08-28 22:14 UTC (permalink / raw)
To: Herbert Xu, Steffen Klassert
Cc: Sebastian Andrzej Siewior, Thomas Gleixner, linux-crypto,
linux-kernel, Daniel Jordan
lockdep complains when...
# echo 0 > /sys/devices/system/cpu/cpu1/online
# echo ff > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
======================================================
WARNING: possible circular locking dependency detected
5.3.0-rc5-padata-base+ #6 Not tainted
------------------------------------------------------
bash/258 is trying to acquire lock:
00000000c43f7f29 (cpu_hotplug_lock.rw_sem){++++}, at: padata_set_cpumask+0x30/0x130
but task is already holding lock:
00000000676aa31d (&pinst->lock){+.+.}, at: padata_set_cpumask+0x2b/0x130
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.
Fixes: 6751fb3c0e0c ("padata: Use get_online_cpus/put_online_cpus")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
kernel/padata.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index 6adce3b203fe..75e668fedd8d 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -603,8 +603,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:
@@ -622,8 +622,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;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 5/5] padata: validate cpumask without removed CPU during offline
2019-08-28 22:14 [PATCH v2 0/5] padata flushing and CPU hotplug fixes Daniel Jordan
` (3 preceding siblings ...)
2019-08-28 22:14 ` [PATCH v2 4/5] padata: always acquire cpu_hotplug_lock before pinst->lock Daniel Jordan
@ 2019-08-28 22:14 ` Daniel Jordan
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Jordan @ 2019-08-28 22:14 UTC (permalink / raw)
To: Herbert Xu, Steffen Klassert
Cc: Sebastian Andrzej Siewior, Thomas Gleixner, linux-crypto,
linux-kernel, Daniel Jordan
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
...makes tcrypt mode=215 crash like this:
divide error: 0000 [#1] SMP PTI
CPU: 4 PID: 287 Comm: modprobe Not tainted 5.3.0-rc5-padata-base+ #7
RIP: 0010:padata_do_parallel+0xfd/0x290
Call Trace:
pcrypt_do_parallel+0xed/0x1e0 [pcrypt]
pcrypt_aead_encrypt+0xbf/0xd0 [pcrypt]
crypto_aead_encrypt+0x1f/0x30
do_mult_aead_op+0x4e/0xdf [tcrypt]
test_mb_aead_speed.constprop.0.cold+0x226/0x564 [tcrypt]
do_test+0x2280/0x4c16 [tcrypt]
tcrypt_mod_init+0x55/0x1000 [tcrypt]
...
cpumask_weight() in padata_cpu_hash() returns 0 because the mask has no
CPUs. The problem is __padata_remove_cpu() checks for valid masks too
early and so doesn't mark the instance PADATA_INVALID as expected, which
would have made padata_do_parallel() return error before doing the
division.
Fix by introducing a second padata CPU hotplug state before
CPUHP_BRINGUP_CPU so that __padata_remove_cpu() sees the online mask
without @cpu. No need for the cpumask_clear_cpu()s since @cpu is
already missing from those masks.
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: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
include/linux/cpuhotplug.h | 1 +
kernel/padata.c | 21 ++++++++++++++-------
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 068793a619ca..2d55cee638fc 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -59,6 +59,7 @@ enum cpuhp_state {
CPUHP_IOMMU_INTEL_DEAD,
CPUHP_LUSTRE_CFS_DEAD,
CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
+ CPUHP_PADATA_DEAD,
CPUHP_WORKQUEUE_PREP,
CPUHP_POWER_NUMA_PREPARE,
CPUHP_HRTIMERS_PREPARE,
diff --git a/kernel/padata.c b/kernel/padata.c
index 75e668fedd8d..5325e5978ee4 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -690,7 +690,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 (!cpumask_test_cpu(cpu, cpu_online_mask)) {
if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
!padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
@@ -702,9 +702,6 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
return -ENOMEM;
padata_replace(pinst, pd);
-
- cpumask_clear_cpu(cpu, pd->cpumask.cbcpu);
- cpumask_clear_cpu(cpu, pd->cpumask.pcpu);
}
return 0;
@@ -731,7 +728,7 @@ static int padata_cpu_online(unsigned int cpu, struct hlist_node *node)
return ret;
}
-static int padata_cpu_prep_down(unsigned int cpu, struct hlist_node *node)
+static int padata_cpu_dead(unsigned int cpu, struct hlist_node *node)
{
struct padata_instance *pinst;
int ret;
@@ -752,6 +749,7 @@ static enum cpuhp_state hp_online;
static void __padata_free(struct padata_instance *pinst)
{
#ifdef CONFIG_HOTPLUG_CPU
+ cpuhp_state_remove_instance_nocalls(CPUHP_PADATA_DEAD, &pinst->node);
cpuhp_state_remove_instance_nocalls(hp_online, &pinst->node);
#endif
@@ -938,6 +936,8 @@ static struct padata_instance *padata_alloc(struct workqueue_struct *wq,
#ifdef CONFIG_HOTPLUG_CPU
cpuhp_state_add_instance_nocalls_cpuslocked(hp_online, &pinst->node);
+ cpuhp_state_add_instance_nocalls_cpuslocked(CPUHP_PADATA_DEAD,
+ &pinst->node);
#endif
return pinst;
@@ -984,17 +984,24 @@ static __init int padata_driver_init(void)
int ret;
ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, "padata:online",
- padata_cpu_online,
- padata_cpu_prep_down);
+ padata_cpu_online, NULL);
if (ret < 0)
return ret;
hp_online = ret;
+
+ ret = cpuhp_setup_state_multi(CPUHP_PADATA_DEAD, "padata:dead",
+ NULL, padata_cpu_dead);
+ if (ret < 0) {
+ cpuhp_remove_multi_state(hp_online);
+ return ret;
+ }
return 0;
}
module_init(padata_driver_init);
static __exit void padata_driver_exit(void)
{
+ cpuhp_remove_multi_state(CPUHP_PADATA_DEAD);
cpuhp_remove_multi_state(hp_online);
}
module_exit(padata_driver_exit);
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread