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

[Al Viro]

On Mon, Jan 26, 2015 at 07:30:49PM +0000, Al Viro wrote:
> On Mon, Jan 26, 2015 at 10:09:13AM -0500, Steven Rostedt wrote:
> > There has been complaints that tracing is tied too much to debugfs,
> > as there are systems that would like to perform tracing, but do
> > not mount debugfs for security reasons. That is because any subsystem
> > may use debugfs for debugging, and these interfaces are not always
> > tested for security.
> > 
> > Creating a new tracefs that the tracing directory will now be attached
> > to allows system admins the ability to access the tracing directory
> > without the need to mount debugfs.
> > 
> > Another advantage is that debugfs does not support the system calls
> > for mkdir and rmdir. Tracing uses these system calls to create new
> > instances for sub buffers. This was done by a hack that hijacked the
> > dentry ops from the "instances" debugfs dentry, and replacing it with
> > one that could work.
> > 
> > Instead of using this hack, tracefs can provide a proper interface to
> > allow the tracing system to have a mkdir and rmdir feature.
> > 
> > To maintain backward compatibility with older tools that expect that
> > the tracing directory is mounted with debugfs, the tracing directory
> > is still created under debugfs and tracefs is automatically mounted
> > there.
> > 
> > Finally, a new directory is created when tracefs is enabled called
> > /sys/kernel/tracing. This will be the new location that system admins
> > may mount tracefs if they are not using debugfs.
> 
> You are still fighting an inconvenient API, but now it's not debugfs one -
> it's your copy thereof.  Why not give your instances/ an inode_operations
> of its own?  One with ->mkdir() and ->rmdir(), leaving all other directories
> as-is.  That way you don't need the secondary methods at all.  And sure,
> debugfs_create_dir() grabs ->i_mutex on parent, making you drop that in
> your ->mkdir() if you want to call it.  But now you are not talking to it -
> just to your own code, where you are free to change the calling conventions,
> making it caller's responsibility to get that ->i_mutex.  The same goes for
> the rmdir side...

Is the following correct?
	* only one directory in the entire tree (/instances) allows mkdir/rmdir.
	* ftrace_trace_arrays always starts with global_trace and the rest
is in 1-to-1 correspondence with subdirectories of /instances.
	* tr->name is NULL for global_trace and non-NULL for everything
else.
	* all modifications of that list are happening from mkdir/rmdir
	* in the end of ->mkdir tr->dir set to the dentry of our subdirectory,
and it's non-NULL (or trace_array creation simply fails)
	* global_array.dir is set to magical value ((struct dentry *)1) upon
the first call of tracing_init_dentry().  Prior to that it's NULL.  BTW, may
I politely inquire what the fuck are those contortions in
tracing_init_dentry_tr() about?  Looks like a stunningly convoluted way
to trigger that automount point creation early in tracer_init_tracefs().
Why not do that right there explicitly?

What are mount options doing?  I mean, sure, you set the mode/uid/gid
of root.  Which is not modifiable anyway...  And AFAICS that's all those
options are affecting.

AFAICS, nothing ever removes files in debugfs root.  Right?  If so, you don't
need the games with simple_pin_fs() - they are pointless in such situation
anyway.  Just do tracefs_mount = kern_mount(&trace_fs_type); in tracefs_init()
and be done with that; lose the tracefs_mount_count and all calls of
simple_{pin,release}_fs().

While we are at it, what the hell is tracefs_file_operations about?  Looks
like some bastard offspring of /dev/null, but I don't see anything that would
use it...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/