linux-kernel - Re: [PATCH] locking/lockdep: Fix expected depth value in __lock

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: <20151020130729.GE17308@twins.programming.kicks-ass.net>
Date:	Tue, 20 Oct 2015 15:07:29 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Jerome Glisse <j.glisse@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Jérôme Glisse <jglisse@...hat.com>,
	Ingo Molnar <mingo@...nel.org>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: [PATCH] locking/lockdep: Fix expected depth value in
 __lock_release()

On Tue, Oct 20, 2015 at 08:42:19AM -0400, Jerome Glisse wrote:
> On Tue, Oct 20, 2015 at 02:18:53PM +0200, Peter Zijlstra wrote:

> > What code did you find that triggered this? That is, what code is taking
> > nested locks with other locks in the middle? (Not wrong per-se, just
> > curious how that would come about).
> 
> Well i am not able to reproduce myself but it happens as part of
> mm_drop_all_locks() as to which lock does trigger i am unsure as
> all the i_mmap_rwsem are taken one after the other and same for
> anon_vma rwsem so they should already coalesce properly. My guess
> is that code calling all lock also have a mutex and once all vma
> lock are drop the mutex coalesce with mm_all_locks_mutex.

Ah yes, mm_all_locks_mutex looks like a likely candidate.

Curious, this code is ancient, and I've never seen a report of this
triggering.

> > > This patch adjust the expect depth value by decrementing it if
> > > what was previously 2 entry inside the stack are coalesced into
> > > only one entry.
> > 
> > Would it not make more sense to scan the entire hlock stack on
> > __lock_acquire() and avoid getting collapsible entries in the first
> > place?
> > 
> > Something like so...
> 
> It would work too, probably more compute intensive than my solution
> but this is lockdep code so i guess it is fine. Also dunno if we loose
> any valuable information by not keeping the stack ordered so one
> can check order in whick lock are taken.

Right; its a little bit more expensive, but only for acquires with
nest_lock set, which should be rare.

As to the order; since they're all of the same class, its fine to
collapse them.

However the proposed alternative avoids 'strange' boundary cases like:

	mutex_lock(&top_lock);

	for (...) {
		mutex_lock_nest_lock(&obj->lock1, &top_lock);
		mutex_lock_nest_lock(&obj->lock2, &top_lock);
	}

Which would currently result in running our of lock stack space real
quick since it would never be able to collapse.

In any case, can you 'test' the proposed alternative in any way?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/