Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754245AbbLJMr4 (ORCPT ); Thu, 10 Dec 2015 07:47:56 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:36520 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753867AbbLJMr0 (ORCPT ); Thu, 10 Dec 2015 07:47:26 -0500 From: Roman Pen Cc: Roman Pen , Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: [RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release Date: Thu, 10 Dec 2015 13:47:14 +0100 Message-Id: <1449751634-7887-4-git-send-email-r.peniaev@gmail.com> X-Mailer: git-send-email 2.6.2 In-Reply-To: <1449751634-7887-1-git-send-email-r.peniaev@gmail.com> References: <1449751634-7887-1-git-send-email-r.peniaev@gmail.com> To: unlisted-recipients:; (no To-header on input) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4102 Lines: 124 __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 Cc: Greg Kroah-Hartman Cc: linux-kernel@vger.kernel.org --- fs/debugfs/inode.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index a1d077a..2525158 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -193,9 +193,23 @@ static struct vfsmount *debugfs_automount(struct path *path) return p->func(p->data); } +static void debugfs_release(struct dentry *dentry) +{ + struct completion *comp; + + /* Paired with __debugfs_remove */ + smp_rmb(); + comp = dentry->d_fsdata; + if (likely(comp)) { + dentry->d_fsdata = NULL; + complete(comp); + } +} + static const struct dentry_operations debugfs_dops = { .d_delete = always_delete_dentry, .d_automount = debugfs_automount, + .d_release = debugfs_release, }; static int debug_fill_super(struct super_block *sb, void *data, int silent) @@ -542,14 +556,24 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) int ret = 0; if (simple_positive(dentry)) { + struct completion comp; + + init_completion(&comp); dget(dentry); if (d_is_dir(dentry)) ret = simple_rmdir(d_inode(parent), dentry); else simple_unlink(d_inode(parent), dentry); - if (!ret) + if (likely(!ret)) { d_delete(dentry); + dentry->d_fsdata = ∁ + /* Paired with debugfs_release callback */ + smp_wmb(); + } dput(dentry); + + if (likely(!ret)) + wait_for_completion(&comp); } return ret; } -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/