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: <20170512161430.4a4fa278@gandalf.local.home>
Date:   Fri, 12 May 2017 16:14:30 -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:

> On Fri, May 12, 2017 at 01:15:44PM -0400, Steven Rostedt wrote:
> >  2) Allow for get_online_cpus() to nest  
> 
> So Thomas and me have been avoiding doing this.
> 
> In general we avoid nested locking in the kernel. Nested locking makes
> an absolute mockery of locking rules and what all gets protected.
> 
> Yes, its much easier.. But we managed to kill the BKL, so surely we can
> fix the hotplug lock too, right ;-)

Well, is it really a lock in that sense? Or more like a
preempt_disable()? Which, one can argue is a BKL in its own right.

get_online_cpus() is more like a preempt_disable() than a lock, as it
is preventing something from happening and not really protecting data.
preempt_disable() prevents a schedule from happening. get_online_cpus()
prevents CPUs from going offline or coming online.

Can you image the mess it would be if we prevent preempt_disable() from
nesting? get_online_cpus() is similar, but maybe not so horrific.

The problem I see with going the route of not letting get_online_cpus()
from nesting, is that we are going to have to start encapsulating large
areas where get_online_cpus() must be taken. Any time a low level
function needs to take get_online_cpus() and there happens to be a
higher level function that has a lock that also must have
get_online_cpus() held, that calls that lower level function (take
tracepoints_mutex for example), that means we need to remove the
get_online_cpus() from the lower level function, and make it a
requirement to be taken before calling that lower level function
everywhere. It moves the get_online_cpus() away from what really needs
to have protection, and makes it more into a global lock like the BKL.

Look at all the places that needed get_online_cpus() in your patches
where the function itself really didn't care about cpus going on or off
line.

-- Steve