Re: [PATCH 1/2] debugfs: prevent access to possibly dead file_operations at file open

[Greg Kroah-Hartman]

On Tue, Dec 01, 2015 at 12:21:31AM +0100, Nicolai Stange wrote:
> Nothing prevents a dentry found by path lookup before a return of
> __debugfs_remove() to actually get opened after that return. Now, after
> the return of __debugfs_remove(), there are no guarantees whatsoever
> regarding the memory the corresponding inode's file_operations object
> had been kept in.
> 
> Since __debugfs_remove() is seldomly invoked, usually from module exit
> handlers only, the race is hard to trigger and the impact is very low.
> 
> A discussion of the problem outlined above as well as a suggested
> solution can be found in the (sub-)thread rooted at
> 
>   http://lkml.kernel.org/g/20130401203445.GA20862@xxxxxxxxxxxxxxxxxx
>   ("Yet another pipe related oops.")
> 
> Basically, Greg KH suggests to introduce an intermediate fops and
> Al Viro points out that a pointer to the original ones may be stored in
> ->d_fsdata.
> 
> Follow this line of reasoning:
> - Add SRCU as a reverse dependency of DEBUG_FS.
> - Introduce a srcu_struct object for the debugfs subsystem.
> - In debugfs_create_file(), store a pointer to the original
>   file_operations object in ->d_fsdata.
> - In __debugfs_remove(), clear out that dentry->d_fsdata member again.
>   debugfs_remove() and debugfs_remove_recursive() wait for a SRCU grace
>   period before returning to their caller.
> - Introduce an intermediate file_operations object named
>   "debugfs_proxy_file_operations". It's ->open() functions checks,
>   under the protection of a SRCU read lock, whether the "original"
>   file_operations pointed to by ->d_fsdata is still valid and if so,
>   tries to acquire a reference on the owning module. On success, it sets
>   the file object's ->f_op to the original file_operations and forwards
>   the ongoing open() call to the original ->open().
> - For clarity, rename the former debugfs_file_operations to
>   debugfs_noop_file_operations -- they are in no way canonical.
> 
> The choice of SRCU over "normal" RCU is justified by the fact, that the
> former may also be used to protect ->i_private data from going away
> during the execution of a file's readers and writers which may (and do)
> sleep.
> 
> Signed-off-by: Nicolai Stange <nicstange@xxxxxxxxx>
> ---
>  Applicable to the Linus tree.
>  The second of the two patches depends on the first one

This doesn't apply to Linus's tree anymore, due to vfs changes, can you
refresh it?

Also, one other request below:

> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -19,6 +19,7 @@
>  #include <linux/seq_file.h>
>  
>  #include <linux/types.h>
> +#include <linux/srcu.h>
>  
>  struct device;
>  struct file_operations;
> @@ -44,7 +45,10 @@ extern struct dentry *arch_debugfs_dir;
>  #if defined(CONFIG_DEBUG_FS)
>  
>  /* declared over in file.c */
> -extern const struct file_operations debugfs_file_operations;
> +extern const struct file_operations debugfs_noop_file_operations;
> +extern const struct file_operations debugfs_proxy_file_operations;
> +
> +extern struct srcu_struct debugfs_srcu;
>  
>  struct dentry *debugfs_create_file(const char *name, umode_t mode,
>  				   struct dentry *parent, void *data,

We really need an "internal" .h file for debugfs for these things you
are adding, no need to add them so that the whole kernel can see them.

Can you create a fs/debugfs/internal.h file for these structures and
then you don't need the addition of the linux/srcu.h in this file
either.

thanks,

greg k-h