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

Greg Kroah-Hartman
2016-02-08T06:39:47Z

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@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org
> ---
> 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