[PATCH 0/6] workqueue: Make the PWQ allocation and WQ enlistment atomic

[PATCH 0/6] workqueue: Make the PWQ allocation and WQ enlistment atomic

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The PWQ allocation and WQ enlistment are not within the same lock-held
critical section; therefore, their states can become out of sync when
the user modifies the unbound mask or if CPU hotplug events occur in
the interim since those operations only update the WQs that are already
in the list.

Lai Jiangshan (6):
  workqueue: Register sysfs after the whole creation of the new wq
  workqueue: Protect wq_unbound_cpumask with wq_pool_attach_mutex in
    init_rescuer()
  workqueue: Separate out destroy_rescuer()
  workqueue: Init rescuer before alloc and link pwqs
  workqueue: Move kthread_flush_worker() out of alloc_and_link_pwqs()
  workqueue: Put PWQ allocation and WQ enlistment in the same lock C.S.

Cc: Tejun Heo <tj@kernel.org>

 kernel/workqueue.c | 110 ++++++++++++++++++++++++++-------------------
 1 file changed, 63 insertions(+), 47 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/6] workqueue: Register sysfs after the whole creation of the new wq

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

workqueue creation includes adding it to the workqueue list.

Prepare for moving the whole workqueue initializing procedure into
wq_pool_mutex and cpu hotplug locks.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7a33f958dcb2..c738b3024cc2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5689,9 +5689,6 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	if (wq_online && init_rescuer(wq) < 0)
 		goto err_destroy;
 
-	if ((wq->flags & WQ_SYSFS) && workqueue_sysfs_register(wq))
-		goto err_destroy;
-
 	/*
 	 * wq_pool_mutex protects global freeze state and workqueues list.
 	 * Grab it, adjust max_active and add the new @wq to workqueues
@@ -5707,6 +5704,9 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 
 	mutex_unlock(&wq_pool_mutex);
 
+	if ((wq->flags & WQ_SYSFS) && workqueue_sysfs_register(wq))
+		goto err_destroy;
+
 	return wq;
 
 err_free_node_nr_active:
-- 
2.19.1.6.gb485710b

[PATCH 2/6] workqueue: Protect wq_unbound_cpumask with wq_pool_attach_mutex in init_rescuer()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

wq_unbound_cpumask can be possibly changed without wq_pool_attach_mutex
or wq_pool_mutex held in init_rescuer().

Use wq_pool_attach_mutex to protect it.

Fixes: 49584bb8ddbe("workqueue: Bind unbound workqueue rescuer to wq_unbound_cpumask")
Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c738b3024cc2..cf1a129eb547 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5533,6 +5533,9 @@ static int init_rescuer(struct workqueue_struct *wq)
 		return ret;
 	}
 
+	/* lock wq_pool_attach_mutex for wq_unbound_cpumask */
+	mutex_lock(&wq_pool_attach_mutex);
+
 	wq->rescuer = rescuer;
 	if (wq->flags & WQ_UNBOUND)
 		kthread_bind_mask(rescuer->task, wq_unbound_cpumask);
@@ -5540,6 +5543,8 @@ static int init_rescuer(struct workqueue_struct *wq)
 		kthread_bind_mask(rescuer->task, cpu_possible_mask);
 	wake_up_process(rescuer->task);
 
+	mutex_unlock(&wq_pool_attach_mutex);
+
 	return 0;
 }
 
-- 
2.19.1.6.gb485710b

[PATCH 3/6] workqueue: Separate out destroy_rescuer()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

Separate out destroy_rescuer() to simplify destroy_workqueue() and
prepare of reusing it in alloc_workqueue().

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index cf1a129eb547..0dd9a12befb5 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5548,6 +5548,22 @@ static int init_rescuer(struct workqueue_struct *wq)
 	return 0;
 }
 
+static void destroy_rescuer(struct workqueue_struct *wq)
+{
+	struct worker *rescuer = wq->rescuer;
+
+	if (rescuer) {
+		/* this prevents new queueing */
+		raw_spin_lock_irq(&wq_mayday_lock);
+		wq->rescuer = NULL;
+		raw_spin_unlock_irq(&wq_mayday_lock);
+
+		/* rescuer will empty maydays list before exiting */
+		kthread_stop(rescuer->task);
+		kfree(rescuer);
+	}
+}
+
 /**
  * wq_adjust_max_active - update a wq's max_active to the current setting
  * @wq: target workqueue
@@ -5772,18 +5788,7 @@ void destroy_workqueue(struct workqueue_struct *wq)
 	drain_workqueue(wq);
 
 	/* kill rescuer, if sanity checks fail, leave it w/o rescuer */
-	if (wq->rescuer) {
-		struct worker *rescuer = wq->rescuer;
-
-		/* this prevents new queueing */
-		raw_spin_lock_irq(&wq_mayday_lock);
-		wq->rescuer = NULL;
-		raw_spin_unlock_irq(&wq_mayday_lock);
-
-		/* rescuer will empty maydays list before exiting */
-		kthread_stop(rescuer->task);
-		kfree(rescuer);
-	}
+	destroy_rescuer(wq);
 
 	/*
 	 * Sanity checks - grab all the locks so that we wait for all
-- 
2.19.1.6.gb485710b

[PATCH 4/6] workqueue: Init rescuer before alloc and link pwqs

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

Swap the order of the allocations for rescuer and pwqs to prepare for
making alloc_and_link_pwqs() and the wq enlistment into the same
wq_pool_mutex and cpu hotplug locks protection.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0dd9a12befb5..810ea55c0ac9 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5704,11 +5704,11 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 			goto err_unreg_lockdep;
 	}
 
-	if (alloc_and_link_pwqs(wq) < 0)
+	if (wq_online && init_rescuer(wq) < 0)
 		goto err_free_node_nr_active;
 
-	if (wq_online && init_rescuer(wq) < 0)
-		goto err_destroy;
+	if (alloc_and_link_pwqs(wq) < 0)
+		goto err_free_rescuer;
 
 	/*
 	 * wq_pool_mutex protects global freeze state and workqueues list.
@@ -5730,6 +5730,8 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 
 	return wq;
 
+err_free_rescuer:
+	destroy_rescuer(wq);
 err_free_node_nr_active:
 	if (wq->flags & WQ_UNBOUND)
 		free_node_nr_active(wq->node_nr_active);
-- 
2.19.1.6.gb485710b

[PATCH 5/6] workqueue: Move kthread_flush_worker() out of alloc_and_link_pwqs()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

kthread_flush_worker() can't be called with wq_pool_mutex held.

Prepare for moving wq_pool_mutex and cpu hotplug lock out of
alloc_and_link_pwqs().

Cc: Zqiang <qiang.zhang1211@gmail.com>
Link: https://lore.kernel.org/lkml/20230920060704.24981-1-qiang.zhang1211@gmail.com/
Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 810ea55c0ac9..3203c67ec4cb 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5472,12 +5472,6 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 	}
 	cpus_read_unlock();
 
-	/* for unbound pwq, flush the pwq_release_worker ensures that the
-	 * pwq_release_workfn() completes before calling kfree(wq).
-	 */
-	if (ret)
-		kthread_flush_worker(pwq_release_worker);
-
 	return ret;
 
 enomem:
@@ -5731,6 +5725,14 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	return wq;
 
 err_free_rescuer:
+	/*
+	 * Failed alloc_and_link_pwqs() may leave pending pwq->release_work,
+	 * flushing the pwq_release_worker ensures that the pwq_release_workfn()
+	 * completes before calling kfree(wq).
+	 */
+	if (wq->flags & WQ_UNBOUND)
+		kthread_flush_worker(pwq_release_worker);
+
 	destroy_rescuer(wq);
 err_free_node_nr_active:
 	if (wq->flags & WQ_UNBOUND)
-- 
2.19.1.6.gb485710b

[PATCH 6/6] workqueue: Put PWQ allocation and WQ enlistment in the same lock C.S.

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

The PWQ allocation and WQ enlistment are not within the same lock-held
critical section; therefore, their states can become out of sync when
the user modifies the unbound mask or if CPU hotplug events occur in
the interim since those operations only update the WQs that are already
in the list.

Make the PWQ allocation and WQ enlistment atomic.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 54 ++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 3203c67ec4cb..c910f3c28664 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5114,6 +5114,19 @@ static struct pool_workqueue *alloc_unbound_pwq(struct workqueue_struct *wq,
 	return pwq;
 }
 
+static void apply_wqattrs_lock(void)
+{
+	/* CPUs should stay stable across pwq creations and installations */
+	cpus_read_lock();
+	mutex_lock(&wq_pool_mutex);
+}
+
+static void apply_wqattrs_unlock(void)
+{
+	mutex_unlock(&wq_pool_mutex);
+	cpus_read_unlock();
+}
+
 /**
  * wq_calc_pod_cpumask - calculate a wq_attrs' cpumask for a pod
  * @attrs: the wq_attrs of the default pwq of the target workqueue
@@ -5425,6 +5438,9 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 	bool highpri = wq->flags & WQ_HIGHPRI;
 	int cpu, ret;
 
+	lockdep_assert_cpus_held();
+	lockdep_assert_held(&wq_pool_mutex);
+
 	wq->cpu_pwq = alloc_percpu(struct pool_workqueue *);
 	if (!wq->cpu_pwq)
 		goto enomem;
@@ -5457,20 +5473,18 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 		return 0;
 	}
 
-	cpus_read_lock();
 	if (wq->flags & __WQ_ORDERED) {
 		struct pool_workqueue *dfl_pwq;
 
-		ret = apply_workqueue_attrs(wq, ordered_wq_attrs[highpri]);
+		ret = apply_workqueue_attrs_locked(wq, ordered_wq_attrs[highpri]);
 		/* there should only be single pwq for ordering guarantee */
 		dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
 		WARN(!ret && (wq->pwqs.next != &dfl_pwq->pwqs_node ||
 			      wq->pwqs.prev != &dfl_pwq->pwqs_node),
 		     "ordering guarantee broken for workqueue %s\n", wq->name);
 	} else {
-		ret = apply_workqueue_attrs(wq, unbound_std_wq_attrs[highpri]);
+		ret = apply_workqueue_attrs_locked(wq, unbound_std_wq_attrs[highpri]);
 	}
-	cpus_read_unlock();
 
 	return ret;
 
@@ -5701,15 +5715,15 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	if (wq_online && init_rescuer(wq) < 0)
 		goto err_free_node_nr_active;
 
-	if (alloc_and_link_pwqs(wq) < 0)
-		goto err_free_rescuer;
-
 	/*
-	 * wq_pool_mutex protects global freeze state and workqueues list.
-	 * Grab it, adjust max_active and add the new @wq to workqueues
-	 * list.
+	 * wq_pool_mutex protects the workqueues list, allocations of PWQs,
+	 * and the global freeze state.  alloc_and_link_pwqs() also requires
+	 * cpus_read_lock() for PWQs' affinities.
 	 */
-	mutex_lock(&wq_pool_mutex);
+	apply_wqattrs_lock();
+
+	if (alloc_and_link_pwqs(wq) < 0)
+		goto err_unlock_free_rescuer;
 
 	mutex_lock(&wq->mutex);
 	wq_adjust_max_active(wq);
@@ -5717,14 +5731,15 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 
 	list_add_tail_rcu(&wq->list, &workqueues);
 
-	mutex_unlock(&wq_pool_mutex);
+	apply_wqattrs_unlock();
 
 	if ((wq->flags & WQ_SYSFS) && workqueue_sysfs_register(wq))
 		goto err_destroy;
 
 	return wq;
 
-err_free_rescuer:
+err_unlock_free_rescuer:
+	apply_wqattrs_unlock();
 	/*
 	 * Failed alloc_and_link_pwqs() may leave pending pwq->release_work,
 	 * flushing the pwq_release_worker ensures that the pwq_release_workfn()
@@ -7005,19 +7020,6 @@ static struct attribute *wq_sysfs_attrs[] = {
 };
 ATTRIBUTE_GROUPS(wq_sysfs);
 
-static void apply_wqattrs_lock(void)
-{
-	/* CPUs should stay stable across pwq creations and installations */
-	cpus_read_lock();
-	mutex_lock(&wq_pool_mutex);
-}
-
-static void apply_wqattrs_unlock(void)
-{
-	mutex_unlock(&wq_pool_mutex);
-	cpus_read_unlock();
-}
-
 static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-- 
2.19.1.6.gb485710b

Re: [PATCH 2/6] workqueue: Protect wq_unbound_cpumask with wq_pool_attach_mutex in init_rescuer()

[Waiman Long]

On 7/2/24 23:38, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
>
> wq_unbound_cpumask can be possibly changed without wq_pool_attach_mutex
> or wq_pool_mutex held in init_rescuer().
>
> Use wq_pool_attach_mutex to protect it.
>
> Fixes: 49584bb8ddbe("workqueue: Bind unbound workqueue rescuer to wq_unbound_cpumask")
> Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> ---
>   kernel/workqueue.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c738b3024cc2..cf1a129eb547 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -5533,6 +5533,9 @@ static int init_rescuer(struct workqueue_struct *wq)
>   		return ret;
>   	}
>   
> +	/* lock wq_pool_attach_mutex for wq_unbound_cpumask */
> +	mutex_lock(&wq_pool_attach_mutex);
> +
>   	wq->rescuer = rescuer;
>   	if (wq->flags & WQ_UNBOUND)
>   		kthread_bind_mask(rescuer->task, wq_unbound_cpumask);
> @@ -5540,6 +5543,8 @@ static int init_rescuer(struct workqueue_struct *wq)
>   		kthread_bind_mask(rescuer->task, cpu_possible_mask);
>   	wake_up_process(rescuer->task);
>   
> +	mutex_unlock(&wq_pool_attach_mutex);
> +
>   	return 0;
>   }
>   
Reviewed-by: Waiman Long <longman@redhat.com>

Thanks!

Re: [PATCH 2/6] workqueue: Protect wq_unbound_cpumask with wq_pool_attach_mutex in init_rescuer()

[Tejun Heo]

Hello, Lai.

On Wed, Jul 03, 2024 at 11:38:51AM +0800, Lai Jiangshan wrote:
> @@ -5533,6 +5533,9 @@ static int init_rescuer(struct workqueue_struct *wq)
>  		return ret;
>  	}
>  
> +	/* lock wq_pool_attach_mutex for wq_unbound_cpumask */
> +	mutex_lock(&wq_pool_attach_mutex);
> +
>  	wq->rescuer = rescuer;
>  	if (wq->flags & WQ_UNBOUND)
>  		kthread_bind_mask(rescuer->task, wq_unbound_cpumask);
> @@ -5540,6 +5543,8 @@ static int init_rescuer(struct workqueue_struct *wq)
>  		kthread_bind_mask(rescuer->task, cpu_possible_mask);
>  	wake_up_process(rescuer->task);
>  
> +	mutex_unlock(&wq_pool_attach_mutex);
> +

Isn't that just protecting the reads on wq_unbound_cpumask? I don't
understand what this protects against. Shouldn't the interlocking be
something like "either new rescuer reads the updated cpumask or the
workqueue is already on the workqueue list"?

Thanks.

-- 
tejun