From 5455f7b4255d5c050d63017ada46553aebbecde2 Mon Sep 17 00:00:00 2001 From: Nicolai Stange Date: Tue, 31 Oct 2017 00:15:54 +0100 Subject: [PATCH] debugfs: defer debugfs_fsdata allocation to first usage Currently, __debugfs_create_file allocates one struct debugfs_fsdata instance for every file created. However, there are potentially many debugfs file around, most of which are never touched by userspace. Thus, defer the allocations to the first usage, i.e. to the first debugfs_file_get(). A dentry's ->d_fsdata starts out to point to the "real", user provided fops. After a debugfs_fsdata instance has been allocated (and the real fops pointer has been moved over into its ->real_fops member), ->d_fsdata is changed to point to it from then on. The two cases are distinguished by setting BIT(0) for the real fops case. struct debugfs_fsdata's foremost purpose is to track active users and to make debugfs_remove() block until they are done. Since no debugfs_fsdata instance means no active users, make debugfs_remove() return immediately in this case. Take care of possible races between debugfs_file_get() and debugfs_remove(): either debugfs_remove() must see a debugfs_fsdata instance and thus wait for possible active users or debugfs_file_get() must see a dead dentry and return immediately. Make a dentry's ->d_release(), i.e. debugfs_release_dentry(), check whether ->d_fsdata is actually a debugfs_fsdata instance before kfree()ing it. Similarly, make debugfs_real_fops() check whether ->d_fsdata is actually a debugfs_fsdata instance before returning it, otherwise emit a warning. The set of possible error codes returned from debugfs_file_get() has grown from -EIO to -EIO and -ENOMEM. Make open_proxy_open() and full_proxy_open() pass the -ENOMEM onwards to their callers. Change-Id: I978318e95910338b8a6d4da66c273f72bde864cc Signed-off-by: Nicolai Stange Signed-off-by: Greg Kroah-Hartman Git-commit: 7d39bc50c47b3f8ed0e1a9d671ecb9ec02f10a2d Git-repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git Signed-off-by: Tingwei Zhang --- fs/debugfs/file.c | 55 ++++++++++++++++++++++++++++++++++++------- fs/debugfs/inode.c | 36 +++++++++++++++------------- fs/debugfs/internal.h | 8 +++++++ 3 files changed, 73 insertions(+), 26 deletions(-) diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index d92038c5f131..0536072b60d7 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -53,6 +53,15 @@ const struct file_operations *debugfs_real_fops(const struct file *filp) { struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata; + if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) { + /* + * Urgh, we've been called w/o a protecting + * debugfs_file_get(). + */ + WARN_ON(1); + return NULL; + } + return fsd->real_fops; } EXPORT_SYMBOL_GPL(debugfs_real_fops); @@ -74,9 +83,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops); */ int debugfs_file_get(struct dentry *dentry) { - struct debugfs_fsdata *fsd = dentry->d_fsdata; + struct debugfs_fsdata *fsd; + void *d_fsd; - /* Avoid starvation of removers. */ + d_fsd = READ_ONCE(dentry->d_fsdata); + if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) { + fsd = d_fsd; + } else { + fsd = kmalloc(sizeof(*fsd), GFP_KERNEL); + if (!fsd) + return -ENOMEM; + + fsd->real_fops = (void *)((unsigned long)d_fsd & + ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); + refcount_set(&fsd->active_users, 1); + init_completion(&fsd->active_users_drained); + if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) { + kfree(fsd); + fsd = READ_ONCE(dentry->d_fsdata); + } + } + + /* + * In case of a successful cmpxchg() above, this check is + * strictly necessary and must follow it, see the comment in + * __debugfs_remove_file(). + * OTOH, if the cmpxchg() hasn't been executed or wasn't + * successful, this serves the purpose of not starving + * removers. + */ if (d_unlinked(dentry)) return -EIO; @@ -98,7 +133,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get); */ void debugfs_file_put(struct dentry *dentry) { - struct debugfs_fsdata *fsd = dentry->d_fsdata; + struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata); if (refcount_dec_and_test(&fsd->active_users)) complete(&fsd->active_users_drained); @@ -109,10 +144,11 @@ static int open_proxy_open(struct inode *inode, struct file *filp) { struct dentry *dentry = F_DENTRY(filp); const struct file_operations *real_fops = NULL; - int r = 0; + int r; - if (debugfs_file_get(dentry)) - return -ENOENT; + r = debugfs_file_get(dentry); + if (r) + return r == -EIO ? -ENOENT : r; real_fops = debugfs_real_fops(filp); real_fops = fops_get(real_fops); @@ -233,10 +269,11 @@ static int full_proxy_open(struct inode *inode, struct file *filp) struct dentry *dentry = F_DENTRY(filp); const struct file_operations *real_fops = NULL; struct file_operations *proxy_fops = NULL; - int r = 0; + int r; - if (debugfs_file_get(dentry)) - return -ENOENT; + r = debugfs_file_get(dentry); + if (r) + return r == -EIO ? -ENOENT : r; real_fops = debugfs_real_fops(filp); real_fops = fops_get(real_fops); diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 9c4699038964..f7f672db1cdb 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -184,7 +184,10 @@ static const struct super_operations debugfs_super_operations = { static void debugfs_release_dentry(struct dentry *dentry) { - kfree(dentry->d_fsdata); + void *fsd = dentry->d_fsdata; + + if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) + kfree(dentry->d_fsdata); } static struct vfsmount *debugfs_automount(struct path *path) @@ -310,35 +313,25 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode, { struct dentry *dentry; struct inode *inode; - struct debugfs_fsdata *fsd; - - fsd = kmalloc(sizeof(*fsd), GFP_KERNEL); - if (!fsd) - return NULL; if (!(mode & S_IFMT)) mode |= S_IFREG; BUG_ON(!S_ISREG(mode)); dentry = start_creating(name, parent); - if (IS_ERR(dentry)) { - kfree(fsd); + if (IS_ERR(dentry)) return NULL; - } inode = debugfs_get_inode(dentry->d_sb); - if (unlikely(!inode)) { - kfree(fsd); + if (unlikely(!inode)) return failed_creating(dentry); - } inode->i_mode = mode; inode->i_private = data; inode->i_fop = proxy_fops; - fsd->real_fops = real_fops; - refcount_set(&fsd->active_users, 1); - dentry->d_fsdata = fsd; + dentry->d_fsdata = (void *)((unsigned long)real_fops | + DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); d_instantiate(dentry, inode); fsnotify_create(d_inode(dentry->d_parent), dentry); @@ -601,8 +594,17 @@ static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) simple_unlink(d_inode(parent), dentry); d_delete(dentry); - fsd = dentry->d_fsdata; - init_completion(&fsd->active_users_drained); + + /* + * Paired with the closing smp_mb() implied by a successful + * cmpxchg() in debugfs_file_get(): either + * debugfs_file_get() must see a dead dentry or we must see a + * debugfs_fsdata instance at ->d_fsdata here (or both). + */ + smp_mb(); + fsd = READ_ONCE(dentry->d_fsdata); + if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) + return; if (!refcount_dec_and_test(&fsd->active_users)) wait_for_completion(&fsd->active_users_drained); } diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h index 40669884d68f..49ba8dde85ef 100644 --- a/fs/debugfs/internal.h +++ b/fs/debugfs/internal.h @@ -27,4 +27,12 @@ struct debugfs_fsdata { struct completion active_users_drained; }; +/* + * A dentry's ->d_fsdata either points to the real fops or to a + * dynamically allocated debugfs_fsdata instance. + * In order to distinguish between these two cases, a real fops + * pointer gets its lowest bit set. + */ +#define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0) + #endif /* _DEBUGFS_INTERNAL_H_ */