linux-kernel - Re: [PATCH] cgroup/tracing: Move taking of spin lock out of trace event handlers

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]

Message-ID: <20180711174940.GH72677@devbig577.frc2.facebook.com>
Date:   Wed, 11 Jul 2018 10:49:40 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, cgroups@...r.kernel.org,
        Johannes Weiner <hannes@...xchg.org>,
        Li Zefan <lizefan@...wei.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [PATCH] cgroup/tracing: Move taking of spin lock out of trace
 event handlers

On Mon, Jul 09, 2018 at 05:48:54PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rostedt@...dmis.org>
> 
> It is unwise to take spin locks from the handlers of trace events.
> Mainly, because they can introduce lockups, because it introduces locks
> in places that are normally not tested. Worse yet, because trace events
> are tucked away in the include/trace/events/ directory, locks that are
> taken there are forgotten about.
> 
> As a general rule, I tell people never to take any locks in a trace
> event handler.
> 
> Several cgroup trace event handlers call cgroup_path() which eventually
> takes the kernfs_rename_lock spinlock. This injects the spinlock in the
> code without people realizing it. It also can cause issues for the
> PREEMPT_RT patch, as the spinlock becomes a mutex, and the trace event
> handlers are called with preemption disabled.
> 
> By moving the calculation of the cgroup_path() out of the trace event
> handlers and into a macro (surrounded by a
> trace_cgroup_##type##_enabled()), then we could place the cgroup_path
> into a string, and pass that to the trace event. Not only does this
> remove the taking of the spinlock out of the trace event handler, but
> it also means that the cgroup_path() only needs to be called once (it
> is currently called twice, once to get the length to reserver the
> buffer for, and once again to get the path itself. Now it only needs to
> be done once.
> 
> Reported-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>

Applied to cgroup/for-4.19.

Thanks.

-- 
tejun