[PATCH 0/5 v4] tracing: Add new file system tracefs

* [PATCH 0/5 v4] tracing: Add new file system tracefs
@ 2015-02-02 15:47 Steven Rostedt
  2015-02-02 15:47 ` [PATCH 1/5 v4] tracefs: Add new tracefs file system Steven Rostedt
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Steven Rostedt @ 2015-02-02 15:47 UTC (permalink / raw)
  To: linux-kernel; +Cc: Al Viro, Greg Kroah-Hartman, Ingo Molnar, Andrew Morton

Al,

Seems we never came up with a solution to how to have mkdir create
new files without locking issues. I would like to keep the file creation
interface separate from the rest of the tracing code and not have the
two coupled tightly. That is, I do not plan on replacing locks like
trace_type_lock with dentry->d_inode->i_lock, as I find that is not very
robust.

Also, you never explained to me why releasing the inode mutex in the
tracefs mkdir (and rmdir) functions would brake anything. Yes, there's
a possibility that you could have two users, where one user is opening
a file, and another user deletes the instance and creates a new instance
and the user ends up opening the new instance instead. But as instances
and even accessing them should be done by privilege users only, this
would be a strange case, especially if one privilege user is deleting
an instance that another privilege user is accessing.

I'll explain here so that others understand what locking issue we have.
When mkdir (and rmdir) handlers are called, the parent dentry inode mutex
is held. The mkdir in the instances directory will create more files and
directories that have a copy of the top level tracing directory (options,
trace files, events, etc). To create an instance, we also need to hold
the trace_types_lock, as that is held to iterate all instances to make
sure one isn't deleted while being accessed. Thus, for mkdir in an instance
director, the lock order will be i_mutex followed by trace_types_lock.
But then later, if a module is loaded and we need to create directories
for the events that are in the new module, we need to add these directories
to each trace array, which means we need to take the trace_arrays_lock
to prevent any from being removed, and then create the files. Making the
locking order trace_types_lock followed by i_mutex.

We have the same problem with event_mutex, which is held to protect the
creation of events.

In reality, there's no deadlock in mkdir because the only lock that is held
when we need to take the trace_arrays_lock or event_mutex is the instance dir
lock. And that lock is never taken while holding the trace_arrays_lock or
event_mutex. Although, in rmdir, the instance directory lock is held and
that is taken when we create a directory, so we need to find a way not to
need to hold the trace_arrays_lock when creating the instance instance.
directory, which could be done. The trick is, we need to find a way to
teach lockdep that there's no deadlock (and make sure there really isn't one).

Honestly, unless I know of a real problem with releasing the parent lock
(and the instance lock on rmdir), I don't want to go this route. Because
all access to files within an instance must first grab the trace array
within that instance, which checks that it still exists (under the
trace_arrays_lock), and ups its ref count which prevents it from being
deleted.

Note, the current implementation also releases the lock, and so far there
has not been any issues with this. I have two tests that stress the
creation, deletion and accessing of the intances. Several threads are
created, where some delete instances, other add them, and others read
from them. I've had these tests since I created instances and they have
found bugs, but nothing yet to do with the releasing of the parent lock.

I would like to get these patches into 3.20. As they still do the locking
like it has been, but at least now it's not to generic debugfs and is
contained in the tracefs inode.c file, I still think this is a bit better.
It also solves the mounting of tracing without debugfs.

Anyway, back to the description of this patch series:

=======================

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.

Changes from v3:

 o Instead of pulling in Al's patches, I just pulled in his tree and
   placed thes patches on top of it (thus, Al's patches are not in this
   series).

 o Change tracefs to reflect the same changes that Al did to debugfs.
   That is, I based tracefs on top of debugfs, these patches show that
   I did it on top of debugfs + Al's changes to them.

 o Added a new method called tracefs_create_instance_dir(). This removes
   needing to use the dentry->d_inode->i_private, and only allows for
   one mkdir/rmdir ops to be set. This is then saved by the tracefs
   system and the mkdir/rmdir can only be used by the instance dir that
   is returned.

Changes from v2:

 o Al Viro wrote a series of patches to create "debugfs_create_automount()"
   that gives us a safe way to modify the debugfs dentry to add
   automounting. tracefs now uses this function to automount itself
   on the debugfs/tracing directory.

Changes from v1:

 o Fixed all the posting problems (included files that were missing
   and removed changes that were not suppose to be there).

 o Changed the mkdir/rmdir logic. Instead of trying to keep the inode
   mutexes locked, which caused locking issues with other locks that
   were taken, the locks are still released. But this time, they are
   released by the tracefs system which has a bit more control.
   As the mkdir/rmdir methods for the tracing facility only need the
   new name of the instance, the tracefs mkdir/rmdir copies the name
   from the dentry, releases the locks, and passes in the copy to
   the tracing methods.

Internal SHA1: fbd7add127fedbb79cccc2890241bb6c1b195f69

Steven Rostedt (Red Hat) (5):
      tracefs: Add new tracefs file system
      tracing: Convert the tracing facility over to use tracefs
      tracing: Automatically mount tracefs on debugfs/tracing
      tracefs: Add directory /sys/kernel/tracing
      tracing: Have mkdir and rmdir be part of tracefs

----
 fs/Makefile                          |   1 +
 fs/tracefs/Makefile                  |   4 +
 fs/tracefs/inode.c                   | 650 +++++++++++++++++++++++++++++++++++
 include/linux/tracefs.h              |  45 +++
 include/uapi/linux/magic.h           |   2 +
 kernel/trace/ftrace.c                |  22 +-
 kernel/trace/trace.c                 | 160 ++++-----
 kernel/trace/trace.h                 |   2 +-
 kernel/trace/trace_events.c          |  32 +-
 kernel/trace/trace_functions_graph.c |   7 +-
 kernel/trace/trace_kprobe.c          |  10 +-
 kernel/trace/trace_probe.h           |   2 +-
 kernel/trace/trace_stat.c            |  10 +-
 13 files changed, 811 insertions(+), 136 deletions(-)

^ permalink raw reply	[flat|nested] 6+ messages in thread