linux-kernel - Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

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]

Message-ID: <20170824140240.t4imrpvussebfimm@hirez.programming.kicks-ass.net>
Date:   Thu, 24 Aug 2017 16:02:40 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Byungchul Park <byungchul.park@....com>
Cc:     mingo@...nel.org, tj@...nel.org, boqun.feng@...il.com,
        david@...morbit.com, johannes@...solutions.net, oleg@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

On Thu, Aug 24, 2017 at 11:18:40AM +0900, Byungchul Park wrote:
> On Wed, Aug 23, 2017 at 01:58:47PM +0200, Peter Zijlstra wrote:

> > Also, unconditinoally switching to recursive-read here would fail to
> > detect the actual deadlock on single-threaded workqueues, which do
> 
> Do you mean it's true even in case having fixed lockdep properly?
> Could you explain why if so? IMHO, I don't think so.

I'm saying that if lockdep is fixed it should be:

	if (wq->saved_max_active == 1 || wq->rescuer) {
		lock_map_acquire(wq->lockdep_map);
		lock_map_acquire(lockdep_map);
	} else {
		lock_map_acquire_read(wq->lockdep_map);
		lock_map_acquire_read(lockdep_map);
	}

or something like that, because for a single-threaded workqueue, the
following _IS_ a deadlock:

	work-n:
		wait_for_completion(C);

	work-n+1:
		complete(C);

And that is the only case we now fail to catch.

> > +void crossrelease_hist_start(enum xhlock_context_t c, bool force)
> >  {
> >  	struct task_struct *cur = current;
> >  
> > -	if (cur->xhlocks) {
> > -		cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> > -		cur->hist_id_save[c] = cur->hist_id;
> > +	if (!cur->xhlocks)
> > +		return;
> > +
> > +	/*
> > +	 * We call this at an invariant point, no current state, no history.
> > +	 */
> 
> This very work-around code _must_ be removed after fixing read-recursive
> thing in lockdep. I think it would be better to add a tag(comment)
> saying it.
> 
> > +	if (c == XHLOCK_PROC) {
> > +		/* verified the former, ensure the latter */
> > +		WARN_ON_ONCE(!force && cur->lockdep_depth);
> > +		invalidate_xhlock(&xhlock(cur->xhlock_idx));
> >  	}

No, this is not a work around, this is fundamentally so. It's not going
away. The only thing that should go away is the .force argument.