linux-kernel - Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Aug 2017 17:10:27 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Byungchul Park <byungchul.park@....com>
Cc:     mingo@...nel.org, linux-kernel@...r.kernel.org,
        kernel-team@....com, Arnaldo Carvalho de Melo <acme@...nel.org>,
        Dave Chinner <david@...morbit.com>, Tejun Heo <tj@...nel.org>,
        johannes@...solutions.net, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all
 part of PROVE_LOCKING

On Tue, Aug 22, 2017 at 04:46:02PM +0200, Peter Zijlstra wrote:
> I am however slightly puzzled by the need of flush_work() to take Q,
> what deadlock potential is there?
> 
> Task:			Work-W1:	Work-W2:
> 
> M(A)			AR(Q)		AR(Q)
> flush_work(W1)		A(W1)		A(W2)
>  A(W1)					  M(A)
>  R(W1)
>  AR(Q)
>  R(Q)
> 
> Spells deadlock on AQ-QA, but why? Why is flush_work() linked to any lock
> taken inside random other works. If we can get rid of flush_work()'s
> usage of Q, we can drop the recursive nature.
> 
> It was added by Oleg in commit:
> 
>   a67da70dc095 ("workqueues: lockdep annotations for flush_work()")
> 
> Which has a distinct lack of Changelog. However, that is still very much
> the old workqueue code, where I think the annotation makes sense because
> that was a single thread running the works consecutively. But I don't
> see it making sense for the current workqueue that runs works
> concurrently.
> 
> TJ, Oleg, can we agree flush_work() no longer needs the dependency on Q?

So we still need it in case of max_active==1 and that rescuer thing, and
then we still need to support calling it from a work, which then
recurses. How about the below, its a bit icky, but should work (boots
and builds a kernel).

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index e86733a8b344..c37b761f60b1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2091,7 +2091,7 @@ __acquires(&pool->lock)
 
 	spin_unlock_irq(&pool->lock);
 
-	lock_map_acquire_read(&pwq->wq->lockdep_map);
+	lock_map_acquire(&pwq->wq->lockdep_map);
 	lock_map_acquire(&lockdep_map);
 	crossrelease_hist_start(XHLOCK_PROC);
 	trace_workqueue_execute_start(work);
@@ -2783,6 +2783,32 @@ void drain_workqueue(struct workqueue_struct *wq)
 }
 EXPORT_SYMBOL_GPL(drain_workqueue);
 
+static bool need_wq_lock(struct pool_workqueue *pwq)
+{
+	struct worker *worker = current_wq_worker();
+
+	/*
+	 * If current is running a work of the same pwq, we already hold
+	 * pwq->wq->lockdep_map, no need to take it again.
+	 */
+	if (worker && worker->current_pwq == pwq)
+		return false;
+
+	/*
+	 * If @max_active is 1 or rescuer is in use, flushing another work
+	 * item on the same workqueue may lead to deadlock.  Make sure the
+	 * flusher is not running on the same workqueue by verifying write
+	 * access.
+	 */
+	if (pwq->wq->saved_max_active == 1)
+		return true;
+
+	if (pwq->wq->rescuer)
+		return true;
+
+	return false;
+}
+
 static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr)
 {
 	struct worker *worker = NULL;
@@ -2816,17 +2842,10 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr)
 	insert_wq_barrier(pwq, barr, work, worker);
 	spin_unlock_irq(&pool->lock);
 
-	/*
-	 * If @max_active is 1 or rescuer is in use, flushing another work
-	 * item on the same workqueue may lead to deadlock.  Make sure the
-	 * flusher is not running on the same workqueue by verifying write
-	 * access.
-	 */
-	if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)
+	if (need_wq_lock(pwq)) {
 		lock_map_acquire(&pwq->wq->lockdep_map);
-	else
-		lock_map_acquire_read(&pwq->wq->lockdep_map);
-	lock_map_release(&pwq->wq->lockdep_map);
+		lock_map_release(&pwq->wq->lockdep_map);
+	}
 
 	return true;
 already_gone: