Re: [RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release

[Greg Kroah-Hartman]

On Thu, Dec 10, 2015 at 01:47:14PM +0100, Roman Pen wrote:
> __debugfs_remove does not wait for dentry release, thus dentry can still be
> alive and file operations can still be invoked after the function returns.
> 
> >From debugfs point of view this behaviour is definitely ok, but that can be
> critical for users of debugfs and lead to usage-after-free: file operations
> can be called after dentry is considered as removed.
> 
> Simple grep over the sources shows that dynamic debugfs file creation and
> removal is exactly the case, and common usage is the following:
> 
>   create_dev():
>       dev = kmalloc();
>       dev->debugfs_dentry = debugfs_create_file("my_dev", , dev, dev_fops);
>                                                             ^^^
>                                             !! pointer is passed to file
>                                             !! operations as private data
> 
>   remove_dev(dev):
>       debugfs_remove(dev->debugfs_dentry);
>       kfree(dev);
>             ^^^
>       !! memory is freed, but fops->open/read/write
>       !! can still be called and lead to usage-after-free
> 
> Here is quick grep output of the case described above:
> 
>    *** drivers/block/pktcdvd.c:
>        pkt_debugfs_dev_remove[489]    debugfs_remove(pd->dfs_f_info);
>        pkt_debugfs_dev_remove[490]    debugfs_remove(pd->dfs_d_root);
> 
>    *** drivers/char/virtio_console.c:
>        unplug_port[1595]              debugfs_remove(port->debugfs_file);
> 
>    *** drivers/crypto/qat/qat_common/adf_cfg.c:
>        adf_cfg_dev_remove[187]        debugfs_remove(dev_cfg_data->debug);
> 
>    *** drivers/gpu/drm/drm_debugfs.c:
>        drm_debugfs_remove_files[203]  debugfs_remove(tmp->dent);
> 
>   .... and more and more and more ...
> 
> In my grep output each third line is exactly this case: people expect that
> debugfs_remove() is a barrier and file operations won't be invoked after it
> (same behaviour as kobject_del(),kobject_put() tuple).
> 
> So in this patch debugfs_remove() waits for completion of final dentry release
> callback.
> 
> BUT! I am not sure that nobody tries to remove the dentry from it's own file
> operation (dentry suicide).  And if so - deadlock will happen.
> 
> Probably, dentry_remove_self() should be implemented for such cases, which is
> similar to sysfs_remove_file_self().  But for now I do not want to add new
> function which can be useless in the nearest future.
> 
> Signed-off-by: Roman Pen <r.peniaev@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  fs/debugfs/inode.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Without a real user, I don't want to take this, as it's "odd".  Also
this is debugfs, you shouldn't be using this for any "real" code, it's
only for debugging.  If you want this for a real api, use configfs.

thanks,

greg k-h