Messages in this thread | | | Date | Sun, 7 Feb 2016 22:39:45 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: [RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release |
| |
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
| |