linux-kernel - Re: [RFC][PATCH 0/5] perf/tracing/cpuhotplug: Fix locking order

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: <20170515150643.72eafc46@gandalf.local.home>
Date:   Mon, 15 May 2017 15:06:43 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [RFC][PATCH 0/5] perf/tracing/cpuhotplug: Fix locking order

On Fri, 12 May 2017 21:49:56 +0200
Peter Zijlstra <peterz@...radead.org> wrote:

> In general we avoid nested locking in the kernel. Nested locking makes
> an absolute mockery of locking rules and what all gets protected.

I'm not against the goal of having get_online_cpus() not be nested,
but I don't agree with the above comment.

If we look at locking in a more abstract view, lock(A) is just
something to protect critical section A. Lock(B) is just something to
protect critical section B. To prevent deadlocks, if one enters
critical section A and then enters critical section B before leaving
critical section A, then the system must never enter critical section B
and then enter critical section A.

But for nested locks, the nested lock is not a new critical section. If
we have lock(A); ...; lock(A); ...; unlock(A); ...; unlock(A); that
just shows a single entry into critical section A. Even if we have:

lock(A);...; lock(B); ...; lock(A); ...; unlock(A); ...;
unlock(B); ...; unlock(A);

As long as there never exits a lock(B); ...; lock(A); without first
taking lock A before taking lock(B).

Because the rules only matter when entering a critical section, and the
taking of the second lock(A) is basically just a nop.

We do this all the time in the kernel, but we just don't do it with
nesting locks. We usually do it with __func()s.

void func(void) {
	lock(A);
	__func();
	unlock(A);
}

and later we could even have:

void foo(void) {
	lock(A);
	...;
	lock(B);
	...;
	__func();
	...;
	unlock(B);
	...;
	unlock(A);
}

If lock(A) was able to nest, then __func() wouldn't be needed. We could
always just call func() which would take lock(A); lockdep will still
work just fine, as it would only care when the lock is first taken, not
its nesting.

One thing we need to do with the current approach is

void __func(void) {

	lockdep_assert_held(A);

	...;
}

to make sure that A is held when calling __func().

Again, I'm not against the lofty goal of having no nesting of
get_online_cpus(), but I just don't agree that nesting locks make a
mockery out of the locking rules.

-- Steve