linux-kernel - Re: [PATCH 00/16 v3] tracing: Add new file system tracefs

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: <20150127043421.GV29656@ZenIV.linux.org.uk>
Date:	Tue, 27 Jan 2015 04:34:21 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 00/16 v3] tracing: Add new file system tracefs

On Mon, Jan 26, 2015 at 07:39:09PM -0500, Steven Rostedt wrote:
> On Mon, 26 Jan 2015 18:49:16 -0500
> Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> > On Mon, 26 Jan 2015 18:43:14 -0500
> > Steven Rostedt <rostedt@...dmis.org> wrote:
> >  
> > > That is, we can not hold i_mutex and take trace_types_lock.
> > > 
> > > trace_types_lock needs to be held with the creation or destruction
> > > of events, which is what mkdir an rmdir do.
> > 
> > Although, I can not remember how this happened, but lockdep blew up on
> > me with this. I'll look again.
> 
> OK, found it. When events are created (module is loaded), the
> trace_types_lock is taken, and the event directories are created (these
> are in /sys/kernel/debug/tracing/events). We need to grab the i_mutex
> in order to create these files.
> 
> This means that we can not take the trace_types_lock within the mkdir
> or rmdir calls.
> 
> The directories are not static even outside the mkdir and rmdir calls.
> As events can be created from several sources, which will create new
> files and directories.

Frankly, the first impression is that you have trace_types_lock way too
high in the hierarchy - you are taking it well outside of the level where
you start walking through ftrace_trace_arrays...

What else is it used for?  You seem to use it protect trace_array refcounts,
but you use them in a very odd way...  What happens if lookup for a file
in that tree loses CPU just as it enters trace_array_get() (e.g. on the
same trace_types_lock), at which point rm .../instances/<whatever> comes,
sees that tr->ref is still zero, kicks the sucker out of the list and
frees it, immediately followed by mkdir creating a new instace?  If the
allocation of the new trace_array happens to reuse the just-freed one,
your trace_array_get() will actually succeed, won't it?

It's not fatal (after all, everything will work as if you opened the similar
file in new instance in the first place), but it looks rather bogus...
--
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/