[PATCH 0/4] workqueue: simple cleanups

[PATCH 0/4] workqueue: simple cleanups

[Lai Jiangshan]

Simple cleanups found when I was reviewing the lkml patches.

Lai Jiangshan (4):
  workqueue: void unneeded requeuing the pwq in rescuer thread
  workqueue: rescuer doesn't check wq->rescuer
  workqueue: free wq->unbound_attrs earlier
  workqueue: remove useless unlock() and lock() in series

Cc: Tejun Heo <tj@kernel.org>

 kernel/workqueue.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH 1/4] workqueue: void unneeded requeuing the pwq in rescuer thread

[Lai Jiangshan]

008847f66c3 ("workqueue: allow rescuer thread to do more work.") made
the rescuer worker requeue the pwq immediately if there may be more
work items which need rescuing instead of waiting for the next mayday
timer expiration.  Unfortunately, it checks only whether the pool needs
help from rescuers, but it doesn't check whether the pwq has work items
in the pool (the real reason that this rescuer can help for the pool).

The patch adds the check and void unneeded requeuing.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7a1fc9fe6314..b2b15f1f0c8d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2529,7 +2529,7 @@ static int rescuer_thread(void *__rescuer)
 			 * being used to relieve memory pressure, don't
 			 * incur MAYDAY_INTERVAL delay inbetween.
 			 */
-			if (need_to_create_worker(pool)) {
+			if (pwq->nr_active && need_to_create_worker(pool)) {
 				spin_lock(&wq_mayday_lock);
 				/*
 				 * Queue iff we aren't racing destruction
-- 
2.20.1

Re: [PATCH 1/4] workqueue: void unneeded requeuing the pwq in rescuer thread

[Tejun Heo]

On Fri, May 29, 2020 at 06:58:59AM +0000, Lai Jiangshan wrote:
> 008847f66c3 ("workqueue: allow rescuer thread to do more work.") made
> the rescuer worker requeue the pwq immediately if there may be more
> work items which need rescuing instead of waiting for the next mayday
> timer expiration.  Unfortunately, it checks only whether the pool needs
> help from rescuers, but it doesn't check whether the pwq has work items
> in the pool (the real reason that this rescuer can help for the pool).
> 
> The patch adds the check and void unneeded requeuing.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>

Applied to wq/for-5.8.

Thanks.

-- 
tejun

[PATCH 2/4] workqueue: don't check wq->rescuer in rescuer

[Lai Jiangshan]

Now rescuer checks pwq->nr_active before requeues the pwq,
it is a more robust check and the rescuer must be still valid.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index b2b15f1f0c8d..8d017727bfbc 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -248,7 +248,7 @@ struct workqueue_struct {
 	struct list_head	flusher_overflow; /* WQ: flush overflow list */
 
 	struct list_head	maydays;	/* MD: pwqs requesting rescue */
-	struct worker		*rescuer;	/* MD: rescue worker */
+	struct worker		*rescuer;	/* I: rescue worker */
 
 	int			nr_drainers;	/* WQ: drain in progress */
 	int			saved_max_active; /* WQ: saved pwq max_active */
@@ -2532,12 +2532,13 @@ static int rescuer_thread(void *__rescuer)
 			if (pwq->nr_active && need_to_create_worker(pool)) {
 				spin_lock(&wq_mayday_lock);
 				/*
-				 * Queue iff we aren't racing destruction
-				 * and somebody else hasn't queued it already.
+				 * Queue iff somebody else hasn't queued it
+				 * already.
 				 */
-				if (wq->rescuer && list_empty(&pwq->mayday_node)) {
+				if (list_empty(&pwq->mayday_node)) {
 					get_pwq(pwq);
-					list_add_tail(&pwq->mayday_node, &wq->maydays);
+					list_add_tail(&pwq->mayday_node,
+						      &wq->maydays);
 				}
 				spin_unlock(&wq_mayday_lock);
 			}
@@ -4356,16 +4357,10 @@ void destroy_workqueue(struct workqueue_struct *wq)
 
 	/* kill rescuer, if sanity checks fail, leave it w/o rescuer */
 	if (wq->rescuer) {
-		struct worker *rescuer = wq->rescuer;
-
-		/* this prevents new queueing */
-		spin_lock_irq(&wq_mayday_lock);
-		wq->rescuer = NULL;
-		spin_unlock_irq(&wq_mayday_lock);
-
 		/* rescuer will empty maydays list before exiting */
-		kthread_stop(rescuer->task);
-		kfree(rescuer);
+		kthread_stop(wq->rescuer->task);
+		kfree(wq->rescuer);
+		wq->rescuer = NULL;
 	}
 
 	/*
-- 
2.20.1

Re: [PATCH 2/4] workqueue: don't check wq->rescuer in rescuer

[Tejun Heo]

On Fri, May 29, 2020 at 06:59:00AM +0000, Lai Jiangshan wrote:
> Now rescuer checks pwq->nr_active before requeues the pwq,
> it is a more robust check and the rescuer must be still valid.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>  kernel/workqueue.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index b2b15f1f0c8d..8d017727bfbc 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -248,7 +248,7 @@ struct workqueue_struct {
>  	struct list_head	flusher_overflow; /* WQ: flush overflow list */
>  
>  	struct list_head	maydays;	/* MD: pwqs requesting rescue */
> -	struct worker		*rescuer;	/* MD: rescue worker */
> +	struct worker		*rescuer;	/* I: rescue worker */
>  
>  	int			nr_drainers;	/* WQ: drain in progress */
>  	int			saved_max_active; /* WQ: saved pwq max_active */
> @@ -2532,12 +2532,13 @@ static int rescuer_thread(void *__rescuer)
>  			if (pwq->nr_active && need_to_create_worker(pool)) {
>  				spin_lock(&wq_mayday_lock);
>  				/*
> -				 * Queue iff we aren't racing destruction
> -				 * and somebody else hasn't queued it already.
> +				 * Queue iff somebody else hasn't queued it
> +				 * already.
>  				 */
> -				if (wq->rescuer && list_empty(&pwq->mayday_node)) {
> +				if (list_empty(&pwq->mayday_node)) {
>  					get_pwq(pwq);
> -					list_add_tail(&pwq->mayday_node, &wq->maydays);
> +					list_add_tail(&pwq->mayday_node,
> +						      &wq->maydays);

send_mayday() also checks for wq->rescuer, so when sanity check fails,
scenarios which would have leaked a workqueue after destroying its rescuer
can lead to use-after-free after the patch. I'm not quite sure why the patch
is an improvement.

Thanks.

-- 
tejun

Re: [PATCH 2/4] workqueue: don't check wq->rescuer in rescuer

[Lai Jiangshan]

On Fri, May 29, 2020 at 10:14 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Fri, May 29, 2020 at 06:59:00AM +0000, Lai Jiangshan wrote:
> > Now rescuer checks pwq->nr_active before requeues the pwq,
> > it is a more robust check and the rescuer must be still valid.
> >
> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> > ---
> >  kernel/workqueue.c | 23 +++++++++--------------
> >  1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index b2b15f1f0c8d..8d017727bfbc 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -248,7 +248,7 @@ struct workqueue_struct {
> >       struct list_head        flusher_overflow; /* WQ: flush overflow list */
> >
> >       struct list_head        maydays;        /* MD: pwqs requesting rescue */
> > -     struct worker           *rescuer;       /* MD: rescue worker */
> > +     struct worker           *rescuer;       /* I: rescue worker */
> >
> >       int                     nr_drainers;    /* WQ: drain in progress */
> >       int                     saved_max_active; /* WQ: saved pwq max_active */
> > @@ -2532,12 +2532,13 @@ static int rescuer_thread(void *__rescuer)
> >                       if (pwq->nr_active && need_to_create_worker(pool)) {
> >                               spin_lock(&wq_mayday_lock);
> >                               /*
> > -                              * Queue iff we aren't racing destruction
> > -                              * and somebody else hasn't queued it already.
> > +                              * Queue iff somebody else hasn't queued it
> > +                              * already.
> >                                */
> > -                             if (wq->rescuer && list_empty(&pwq->mayday_node)) {
> > +                             if (list_empty(&pwq->mayday_node)) {
> >                                       get_pwq(pwq);
> > -                                     list_add_tail(&pwq->mayday_node, &wq->maydays);
> > +                                     list_add_tail(&pwq->mayday_node,
> > +                                                   &wq->maydays);
>
> send_mayday() also checks for wq->rescuer, so when sanity check fails,
> scenarios which would have leaked a workqueue after destroying its rescuer
> can lead to use-after-free after the patch. I'm not quite sure why the patch
> is an improvement.
>

Hi

I'm not sure I understood your words. And I'm not
sure which function may use freed object in "use-after-free".
Is it "send_mayday() may use a freed rescuer"?

This patch relies on
def98c84b6 ("workqueue: Fix spurious sanity check failures in
destroy_workqueue()")
to move the kthread_stop() before the sanity check and the work
of drain_workqueue() which guarantees there is no work item
in the workqueue. If send_mayday() still goes wrong after
drain_workqueue(), the user must have queued work items and
invoked destroy_workqueue() concurrently. It is excellent
if the sanity check can find this case out, but it is not possible
that the sanity check can always live through it since it is
not worqueue's internal fault. We hope the sanity check can
find all the internal fault, but not to the extend that
it can always work when any user uses it in a very wrong way.

Thanks
Lai.

Re: [PATCH 2/4] workqueue: don't check wq->rescuer in rescuer

[Tejun Heo]

Hello,

On Fri, May 29, 2020 at 10:58:46PM +0800, Lai Jiangshan wrote:
> I'm not sure I understood your words. And I'm not
> sure which function may use freed object in "use-after-free".
> Is it "send_mayday() may use a freed rescuer"?
> 
> This patch relies on
> def98c84b6 ("workqueue: Fix spurious sanity check failures in
> destroy_workqueue()")
> to move the kthread_stop() before the sanity check and the work
> of drain_workqueue() which guarantees there is no work item
> in the workqueue. If send_mayday() still goes wrong after
> drain_workqueue(), the user must have queued work items and
> invoked destroy_workqueue() concurrently. It is excellent
> if the sanity check can find this case out, but it is not possible
> that the sanity check can always live through it since it is
> not worqueue's internal fault. We hope the sanity check can
> find all the internal fault, but not to the extend that
> it can always work when any user uses it in a very wrong way.

Yeah, it's not fool proof but it's difficult for me to see what is better
after the patch. What does the patch actually improve?

Thanks.

-- 
tejun

[PATCH 3/4] workqueue: free wq->unbound_attrs earlier

[Lai Jiangshan]

wq->unbound_attrs is never accessed in rcu read site, so that
it can be freed earlier and relieves memory pressure earlier,
although slightly.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8d017727bfbc..c0cbe0de95d0 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3489,8 +3489,6 @@ static void rcu_free_wq(struct rcu_head *rcu)
 
 	if (!(wq->flags & WQ_UNBOUND))
 		free_percpu(wq->cpu_pwqs);
-	else
-		free_workqueue_attrs(wq->unbound_attrs);
 
 	kfree(wq);
 }
@@ -3678,6 +3676,7 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
 	 */
 	if (is_last) {
 		wq_unregister_lockdep(wq);
+		free_workqueue_attrs(wq->unbound_attrs);
 		call_rcu(&wq->rcu, rcu_free_wq);
 	}
 }
-- 
2.20.1

Re: [PATCH 3/4] workqueue: free wq->unbound_attrs earlier

[Tejun Heo]

On Fri, May 29, 2020 at 06:59:01AM +0000, Lai Jiangshan wrote:
> wq->unbound_attrs is never accessed in rcu read site, so that
> it can be freed earlier and relieves memory pressure earlier,
> although slightly.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>

I don't think this is gonna make a material difference in terms of memory
pressure and would much prefer keep freeing together so that we don't
complicate the lifetime rules.

Thanks.

-- 
tejun

[PATCH 4/4] workqueue: remove useless unlock() and lock() in series

[Lai Jiangshan]

This is no point to unlock() and then lock() the same mutex
back to back.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c0cbe0de95d0..415893cfb074 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4383,13 +4383,11 @@ void destroy_workqueue(struct workqueue_struct *wq)
 		spin_unlock_irq(&pwq->pool->lock);
 	}
 	mutex_unlock(&wq->mutex);
-	mutex_unlock(&wq_pool_mutex);
 
 	/*
 	 * wq list is used to freeze wq, remove from list after
 	 * flushing is complete in case freeze races us.
 	 */
-	mutex_lock(&wq_pool_mutex);
 	list_del_rcu(&wq->list);
 	mutex_unlock(&wq_pool_mutex);
 
-- 
2.20.1

Re: [PATCH 4/4] workqueue: remove useless unlock() and lock() in series

[Tejun Heo]

On Fri, May 29, 2020 at 06:59:02AM +0000, Lai Jiangshan wrote:
> This is no point to unlock() and then lock() the same mutex
> back to back.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>

Applied to wq/for-5.8.

Thanks.

-- 
tejun