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] [day] [month] [year] [list]

Date:   Tue, 29 Aug 2017 19:51:06 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Byungchul Park <byungchul.park@....com>, 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
Subject: Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all
 part of PROVE_LOCKING

On Tue, Aug 29, 2017 at 05:52:05PM +0200, Oleg Nesterov wrote:

> The problem is that start_flush_work() does not do acquire/release
> unconditionally, it does this only if it is going to wait, and I am not
> sure this is right...

Right, I think you're right, we can move it earlier, once we have the
pwq.

> Plus process_one_work() does lock_map_acquire_read(), I don't really
> understand this too.

Right, so aside from recursive-reads being broken, I think that was a
mistake.

> > And the analogous:
> >
> > flush_workqueue(wq)	process_one_work(wq, work)
> >   A(wq)			  A(wq)
> >   R(wq)			  work->func(work);
> > 			  R(wq)
> >
> >
> > The thing I puzzled over was flush_work() (really start_flush_work())
> > doing:
> >
> >         if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)
> >                 lock_map_acquire(&pwq->wq->lockdep_map);
> >         else
> >                 lock_map_acquire_read(&pwq->wq->lockdep_map);
> >         lock_map_release(&pwq->wq->lockdep_map);
> >
> > Why does flush_work() care about the wq->lockdep_map?
> >
> > The answer is because, for single-threaded workqueues, doing
> > flush_work() from a work is a potential deadlock:
> 
> Yes, but the simple answer is that flush_work() doesn't really differ
> from flush_workqueue() in this respect?

Right, and I think that the new code (aside from maybe placing it
earlier) does that. If single-threaded we use wq->lockdep_map, otherwise
we (also) use work->lockdep_map.

> If nothing else, if some WORK is the last queued work on WQ, then
> flush_work(WORK) is the same thing as flush_workqueuw(WQ), more or less.
> Again, I am talking about single-threaded workqueues.

Right, so single-threaded workqueues are special and are what we need
this extra bit for, but only for single-threaded.

> > workqueue-thread:
> >
> > 	work-n:
> > 	  flush_work(work-n+1);
> >
> > 	work-n+1:
> >
> >
> > Will not be going anywhere fast..
> 
> Or another example,
> 
> 	lock(LOCK);
> 	flush_work(WORK);
> 	unlock(LOCK);
> 
> 	workqueue-thread:
> 		another_pending_work:
> 			LOCK(LOCK);
> 			UNLOCK(LOCK);
> 
> 		WORK:
> 
> In this case we do not care about WORK->lockdep_map, but
> taking the wq->lockdep_map from flush_work() (if single-threaded) allows
> to report the deadlock.

Right. And the new code does so.