[PATCH 0/4] padata lockdep, cpumask, and doc fixes

[PATCH 0/4] padata lockdep, cpumask, and doc fixes

[Daniel Jordan]

These are on top of v5.4-rc8 plus Herbert's recent padata changes:

  [PATCH] padata: Remove broken queue flushing
  https://lore.kernel.org/linux-crypto/20191119051731.yev6dcsp2znjaagz@gondor.apana.org.au/

  [PATCH] crypto: pcrypt - Fix user-after-free on module unload
  https://lore.kernel.org/linux-crypto/20191119094131.x7gkxdi5clyxk3zd@gondor.apana.org.au/

  [v2 PATCH] crypto: pcrypt - Avoid deadlock by using per-instance padata queues
  https://lore.kernel.org/linux-crypto/20191119185827.nerskpvddkcsih25@gondor.apana.org.au/

  [PATCH] padata: Remove unused padata_remove_cpu
  https://lore.kernel.org/linux-crypto/20191119223250.jaefneeatsa52nhh@gondor.apana.org.au/

I can rebase to something else if that's easier.  Thanks.

Daniel Jordan (4):
  padata: update documentation
  padata: remove reorder_objects
  padata: always acquire cpu_hotplug_lock before pinst->lock
  padata: remove cpumask change notifier

 Documentation/padata.txt | 74 ++++++++++++++--------------------------
 crypto/pcrypt.c          |  1 -
 include/linux/padata.h   | 12 -------
 kernel/padata.c          | 60 +++-----------------------------
 4 files changed, 31 insertions(+), 116 deletions(-)


base-commit: af42d3466bdc8f39806b26f593604fdc54140bcb
prerequisite-patch-id: e31e7b28eb12a2c7e1e04261f4e890f83a57bd19
prerequisite-patch-id: 00f7ca687bd9df6281e9ced0925a865b2fa7b297
prerequisite-patch-id: 0478fe82b9102aeae08f602b170eacc5cf6334de
prerequisite-patch-id: f575fef550bb0cbf5418d3ccd15724d8b4b30858
-- 
2.23.0

[PATCH 1/4] padata: update documentation

[Daniel Jordan]

Remove references to unused functions and update to reflect the new
struct padata_shell.

Fixes: 815613da6a67 ("kernel/padata.c: removed unused code")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/padata.txt | 50 +++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/Documentation/padata.txt b/Documentation/padata.txt
index b37ba1eaace3..a03afb1588f9 100644
--- a/Documentation/padata.txt
+++ b/Documentation/padata.txt
@@ -2,7 +2,7 @@
 The padata parallel execution mechanism
 =======================================
 
-:Last updated: for 2.6.36
+:Last updated: for 5.4
 
 Padata is a mechanism by which the kernel can farm work out to be done in
 parallel on multiple CPUs while retaining the ordering of tasks.  It was
@@ -53,27 +53,26 @@ 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::
+Finally, complete padata initialization by allocating a padata_shell object::
+
+   struct padata_shell *padata_alloc_shell(struct padata_instance *pinst);
+
+A padata_shell is used to submit a job to padata and allows a series of such
+jobs to be serialized independently.  A padata_instance may have one or more
+padata_shell objects associated with it, each allowing a separate series of
+jobs.
+
+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::
@@ -117,12 +116,13 @@ momentarily.
 
 The submission of work is done with::
 
-    int padata_do_parallel(struct padata_instance *pinst,
-		           struct padata_priv *padata, int cb_cpu);
+    int padata_do_parallel(struct padata_shell *ps,
+		           struct padata_priv *padata, int *cb_cpu);
 
-The pinst and padata structures must be set up as described above; cb_cpu
-specifies which CPU will be used for the final callback when the work is
-done; it must be in the current instance's CPU mask.  The return value from
+The ps and padata structures must be set up as described above; cb_cpu
+points to the preferred CPU to be used for the final callback when the work is
+done; it must be in the current instance's CPU mask (if not the cb_cpu pointer
+is updated to point to the CPU actually chosen).  The return value from
 padata_do_parallel() is zero on success, indicating that the work is in
 progress. -EBUSY means that somebody, somewhere else is messing with the
 instance's CPU mask, while -EINVAL is a complaint about cb_cpu not being
@@ -154,10 +154,12 @@ Note that this call may be deferred for a while since the padata code takes
 pains to ensure that tasks are completed in the order in which they were
 submitted.
 
-The one remaining function in the padata API should be called to clean up
-when a padata instance is no longer needed::
+Cleaning up a padata instance predictably involves calling the three free
+functions that correspond to the allocation in reverse:
 
+    void padata_free_shell(struct padata_shell *ps);
+    void padata_stop(struct padata_instance *pinst);
     void padata_free(struct padata_instance *pinst);
 
-This function will busy-wait while any remaining tasks are completed, so it
-might be best not to call it while there is work outstanding.
+It is the user's responsibility to ensure all outstanding jobs are complete
+before any of the above are called.
-- 
2.23.0

Re: [PATCH 1/4] padata: update documentation

[Jonathan Corbet]

On Wed, 20 Nov 2019 13:54:09 -0500
Daniel Jordan <daniel.m.jordan@oracle.com> wrote:

> Remove references to unused functions and update to reflect the new
> struct padata_shell.
> 
> Fixes: 815613da6a67 ("kernel/padata.c: removed unused code")
> Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  Documentation/padata.txt | 50 +++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 24 deletions(-)

This all seems fine - it's better than not doing it - but can I put in a
request or two?

 - This document is already formatted as RST, and your changes continue
   that.  Can we please move it to Documentation/core-api/padata.rst and
   add it to the TOC tree there?  Then it can become part of our formatted
   docs.

 - The padata code seems to be nicely equipped with kerneldoc comments; it
   would be awfully nice to pull them into the document directly rather
   than replicating the API there.  (Why does the document do that now?
   Blame the bozo who originally wrote it :)  That would make the document
   more complete and easier to maintain going forward.

For added goodness we could stick in an SPDX tag while we're at it.

Thanks,

jon

Re: [PATCH 1/4] padata: update documentation

[Daniel Jordan]

On Wed, Nov 20, 2019 at 12:16:34PM -0700, Jonathan Corbet wrote:
> This all seems fine - it's better than not doing it - but can I put in a
> request or two?
> 
>  - This document is already formatted as RST, and your changes continue
>    that.  Can we please move it to Documentation/core-api/padata.rst and
>    add it to the TOC tree there?  Then it can become part of our formatted
>    docs.
> 
>  - The padata code seems to be nicely equipped with kerneldoc comments; it
>    would be awfully nice to pull them into the document directly rather
>    than replicating the API there.  (Why does the document do that now?
>    Blame the bozo who originally wrote it :)  That would make the document
>    more complete and easier to maintain going forward.

Ok.  It would be nice to preserve the how-to aspect of the original doc as
well, in other words, the order the interfaces should be called in.  Will do
both.

> For added goodness we could stick in an SPDX tag while we're at it.

I'll use the license from padata.c/h.

[PATCH 2/4] padata: remove reorder_objects

[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: Eric Biggers <ebiggers@kernel.org>
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
---
 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 fd38897e1b91..9e0be23bd9ff 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -102,7 +102,6 @@ struct padata_cpumask {
  * @sh: padata_shell object.
  * @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.
  * @max_seq_nr:  Maximal used sequence number.
  * @processed: Number of already processed objects.
@@ -115,7 +114,6 @@ struct parallel_data {
 	struct padata_shell		*ps;
 	struct padata_parallel_queue	__percpu *pqueue;
 	struct padata_serial_queue	__percpu *squeue;
-	atomic_t			reorder_objects;
 	atomic_t			refcnt;
 	atomic_t			seq_nr;
 	unsigned int			processed;
diff --git a/kernel/padata.c b/kernel/padata.c
index 8fd188fa017d..5c9bd8df0f0f 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -202,7 +202,6 @@ static struct padata_priv *padata_find_next(struct parallel_data *pd,
 
 	if (remove_object) {
 		list_del_init(&padata->list);
-		atomic_dec(&pd->reorder_objects);
 		++pd->processed;
 		pd->cpu = cpumask_next_wrap(cpu, pd->cpumask.pcpu, -1, false);
 	}
@@ -336,7 +335,6 @@ void padata_do_serial(struct padata_priv *padata)
 		if (cur->seq_nr < padata->seq_nr)
 			break;
 	list_add(&padata->list, &cur->list);
-	atomic_inc(&pd->reorder_objects);
 	spin_unlock(&pqueue->reorder.lock);
 
 	/*
@@ -452,7 +450,6 @@ static struct parallel_data *padata_alloc_pd(struct padata_shell *ps)
 	padata_init_pqueues(pd);
 	padata_init_squeues(pd);
 	atomic_set(&pd->seq_nr, -1);
-	atomic_set(&pd->reorder_objects, 0);
 	atomic_set(&pd->refcnt, 1);
 	spin_lock_init(&pd->lock);
 	pd->cpu = cpumask_first(pd->cpumask.pcpu);
-- 
2.23.0

[PATCH 3/4] padata: always acquire cpu_hotplug_lock before pinst->lock

[Daniel Jordan]

lockdep complains when padata's paths to update cpumasks via CPU hotplug
and sysfs are both taken:

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

  ======================================================
  WARNING: possible circular locking dependency detected
  5.4.0-rc8-padata-cpuhp-v3+ #1 Not tainted
  ------------------------------------------------------
  bash/205 is trying to acquire lock:
  ffffffff8286bcd0 (cpu_hotplug_lock.rw_sem){++++}, at: padata_set_cpumask+0x2b/0x120

  but task is already holding lock:
  ffff8880001abfa0 (&pinst->lock){+.+.}, at: padata_set_cpumask+0x26/0x120

  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: Eric Biggers <ebiggers@kernel.org>
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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index 5c9bd8df0f0f..cd9ae6822460 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -623,8 +623,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:
@@ -642,8 +642,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

[PATCH 4/4] padata: remove cpumask change notifier

[Daniel Jordan]

Since commit 63d3578892dc ("crypto: pcrypt - remove padata cpumask
notifier") this feature is unused and may deliver unexpected data with
the introduction of struct padata_shell (a notification mask that
includes -ENOMEM), so get rid of it.

Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/padata.txt | 24 ------------------
 crypto/pcrypt.c          |  1 -
 include/linux/padata.h   | 10 --------
 kernel/padata.c          | 53 +++-------------------------------------
 4 files changed, 3 insertions(+), 85 deletions(-)

diff --git a/Documentation/padata.txt b/Documentation/padata.txt
index a03afb1588f9..82e42dbaab65 100644
--- a/Documentation/padata.txt
+++ b/Documentation/padata.txt
@@ -74,30 +74,6 @@ 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::
-
-    int padata_register_cpumask_notifier(struct padata_instance *pinst,
-					 struct notifier_block *nblock);
-
-To unregister from that notifier::
-
-    int padata_unregister_cpumask_notifier(struct padata_instance *pinst,
-					   struct notifier_block *nblock);
-
-The padata cpumask change notifier notifies about changes of the usable
-cpumasks, i.e. the subset of active CPUs in the user supplied cpumask.
-
-Padata calls the notifier chain with::
-
-    blocking_notifier_call_chain(&pinst->cpumask_change_notifier,
-				 notification_mask,
-				 &pd_new->cpumask);
-
-Here cpumask_change_notifier is registered notifier, notification_mask
-is one of PADATA_CPU_SERIAL, PADATA_CPU_PARALLEL and cpumask is a pointer
-to a struct padata_cpumask that contains the new cpumask information.
-
 Actually submitting work to the padata instance requires the creation of a
 padata_priv structure::
 
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 3e026e7a7e75..af60016d25c4 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -13,7 +13,6 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/notifier.h>
 #include <linux/kobject.h>
 #include <linux/cpu.h>
 #include <crypto/pcrypt.h>
diff --git a/include/linux/padata.h b/include/linux/padata.h
index 9e0be23bd9ff..efdd0b129c53 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -13,7 +13,6 @@
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
-#include <linux/notifier.h>
 #include <linux/kobject.h>
 
 #define PADATA_CPU_SERIAL   0x01
@@ -141,14 +140,10 @@ struct padata_shell {
 /**
  * struct padata_instance - The overall control structure.
  *
- * @cpu_notifier: cpu hotplug notifier.
  * @parallel_wq: The workqueue used for parallel work.
  * @serial_wq: The workqueue used for serial work.
  * @pslist: List of padata_shell objects attached to this instance.
  * @cpumask: User supplied cpumasks for parallel and serial works.
- * @cpumask_change_notifier: Notifiers chain for user-defined notify
- *            callbacks that will be called when either @pcpu or @cbcpu
- *            or both cpumasks change.
  * @kobj: padata instance kernel object.
  * @lock: padata instance lock.
  * @flags: padata flags.
@@ -159,7 +154,6 @@ struct padata_instance {
 	struct workqueue_struct		*serial_wq;
 	struct list_head		pslist;
 	struct padata_cpumask		cpumask;
-	struct blocking_notifier_head	 cpumask_change_notifier;
 	struct kobject                   kobj;
 	struct mutex			 lock;
 	u8				 flags;
@@ -179,8 +173,4 @@ extern int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 			      cpumask_var_t cpumask);
 extern int padata_start(struct padata_instance *pinst);
 extern void padata_stop(struct padata_instance *pinst);
-extern int padata_register_cpumask_notifier(struct padata_instance *pinst,
-					    struct notifier_block *nblock);
-extern int padata_unregister_cpumask_notifier(struct padata_instance *pinst,
-					      struct notifier_block *nblock);
 #endif
diff --git a/kernel/padata.c b/kernel/padata.c
index cd9ae6822460..7305e81b9bee 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -492,81 +492,35 @@ static void __padata_stop(struct padata_instance *pinst)
 }
 
 /* Replace the internal control structure with a new one. */
-static int padata_replace_one(struct padata_shell *ps)
+static void padata_replace_one(struct padata_shell *ps)
 {
 	struct parallel_data *pd_old = rcu_dereference_protected(ps->pd, 1);
 	struct parallel_data *pd_new;
-	int notification_mask = 0;
 
 	pd_new = padata_alloc_pd(ps);
 	if (!pd_new)
-		return -ENOMEM;
+		return;
 
 	rcu_assign_pointer(ps->pd, pd_new);
 
-	if (!cpumask_equal(pd_old->cpumask.pcpu, pd_new->cpumask.pcpu))
-		notification_mask |= PADATA_CPU_PARALLEL;
-	if (!cpumask_equal(pd_old->cpumask.cbcpu, pd_new->cpumask.cbcpu))
-		notification_mask |= PADATA_CPU_SERIAL;
-
 	if (atomic_dec_and_test(&pd_old->refcnt))
 		padata_free_pd(pd_old);
-
-	return notification_mask;
 }
 
 static void padata_replace(struct padata_instance *pinst)
 {
-	int notification_mask = 0;
 	struct padata_shell *ps;
 
 	pinst->flags |= PADATA_RESET;
 
 	list_for_each_entry(ps, &pinst->pslist, list)
-		notification_mask |= padata_replace_one(ps);
+		padata_replace_one(ps);
 
 	synchronize_rcu();
 
-	if (notification_mask)
-		blocking_notifier_call_chain(&pinst->cpumask_change_notifier,
-					     notification_mask,
-					     &pinst->cpumask);
-
 	pinst->flags &= ~PADATA_RESET;
 }
 
-/**
- * padata_register_cpumask_notifier - Registers a notifier that will be called
- *                             if either pcpu or cbcpu or both cpumasks change.
- *
- * @pinst: A poineter to padata instance
- * @nblock: A pointer to notifier block.
- */
-int padata_register_cpumask_notifier(struct padata_instance *pinst,
-				     struct notifier_block *nblock)
-{
-	return blocking_notifier_chain_register(&pinst->cpumask_change_notifier,
-						nblock);
-}
-EXPORT_SYMBOL(padata_register_cpumask_notifier);
-
-/**
- * padata_unregister_cpumask_notifier - Unregisters cpumask notifier
- *        registered earlier  using padata_register_cpumask_notifier
- *
- * @pinst: A pointer to data instance.
- * @nlock: A pointer to notifier block.
- */
-int padata_unregister_cpumask_notifier(struct padata_instance *pinst,
-				       struct notifier_block *nblock)
-{
-	return blocking_notifier_chain_unregister(
-		&pinst->cpumask_change_notifier,
-		nblock);
-}
-EXPORT_SYMBOL(padata_unregister_cpumask_notifier);
-
-
 /* If cpumask contains no active cpu, we mark the instance as invalid. */
 static bool padata_validate_cpumask(struct padata_instance *pinst,
 				    const struct cpumask *cpumask)
@@ -944,7 +898,6 @@ static struct padata_instance *padata_alloc(const char *name,
 
 	pinst->flags = 0;
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&pinst->cpumask_change_notifier);
 	kobject_init(&pinst->kobj, &padata_attr_type);
 	mutex_init(&pinst->lock);
 
-- 
2.23.0