| Message ID | 20191012005920.630331484@goodmis.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tracing: Fix tracefs lockdown and various clean ups | expand |
On Fri, Oct 11, 2019 at 5:59 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > I bisected this down to the addition of the proxy_ops into tracefs for > lockdown. It appears that the allocation of the proxy_ops and then freeing > it in the destroy_inode callback, is causing havoc with the memory system. > Reading the documentation about destroy_inode and talking with Linus about > this, this is buggy and wrong. Can you still add the explanation about the inode memory leak to this message? Right now it just says "it's buggy and wrong". True. But doesn't explain _why_ it is buggy and wrong. Linus
On Sat, 12 Oct 2019 15:56:15 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Oct 11, 2019 at 5:59 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > I bisected this down to the addition of the proxy_ops into tracefs for > > lockdown. It appears that the allocation of the proxy_ops and then freeing > > it in the destroy_inode callback, is causing havoc with the memory system. > > Reading the documentation about destroy_inode and talking with Linus about > > this, this is buggy and wrong. > > Can you still add the explanation about the inode memory leak to this message? > > Right now it just says "it's buggy and wrong". True. But doesn't > explain _why_ it is buggy and wrong. > Sure. The patches just finished my testing (along with other fixes that I need to send you). I have to make a few other updates in the change log though, so I'll be rebasing them (but not touching the code), to clean up the change logs. -- Steve
On Sat, 12 Oct 2019 20:35:02 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sat, 12 Oct 2019 15:56:15 -0700 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Fri, Oct 11, 2019 at 5:59 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > > > I bisected this down to the addition of the proxy_ops into tracefs for > > > lockdown. It appears that the allocation of the proxy_ops and then freeing > > > it in the destroy_inode callback, is causing havoc with the memory system. > > > Reading the documentation about destroy_inode and talking with Linus about > > > this, this is buggy and wrong. > > > > Can you still add the explanation about the inode memory leak to this message? > > > > Right now it just says "it's buggy and wrong". True. But doesn't > > explain _why_ it is buggy and wrong. > > > > Sure. The patches just finished my testing (along with other fixes that > I need to send you). I have to make a few other updates in the change > log though, so I'll be rebasing them (but not touching the code), to > clean up the change logs. > I updated this change log to state: "I bisected this down to the addition of the proxy_ops into tracefs for lockdown. It appears that the allocation of the proxy_ops and then freeing it in the destroy_inode callback, is causing havoc with the memory system. Reading the documentation about destroy_inode and talking with Linus about this, this is buggy and wrong. When defining the destroy_inode() method, it is expected that the destroy_inode() will also free the inode, and not just the extra allocations done in the creation of the inode. The faulty commit causes a memory leak of the inode data structure when they are deleted." -- Steve
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 9fc14e38927f..eeeae0475da9 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -20,7 +20,6 @@ #include <linux/parser.h> #include <linux/magic.h> #include <linux/slab.h> -#include <linux/security.h> #define TRACEFS_DEFAULT_MODE 0700 @@ -28,25 +27,6 @@ static struct vfsmount *tracefs_mount; static int tracefs_mount_count; static bool tracefs_registered; -static int default_open_file(struct inode *inode, struct file *filp) -{ - struct dentry *dentry = filp->f_path.dentry; - struct file_operations *real_fops; - int ret; - - if (!dentry) - return -EINVAL; - - ret = security_locked_down(LOCKDOWN_TRACEFS); - if (ret) - return ret; - - real_fops = dentry->d_fsdata; - if (!real_fops->open) - return 0; - return real_fops->open(inode, filp); -} - static ssize_t default_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) { @@ -241,12 +221,6 @@ static int tracefs_apply_options(struct super_block *sb) return 0; } -static void tracefs_destroy_inode(struct inode *inode) -{ - if (S_ISREG(inode->i_mode)) - kfree(inode->i_fop); -} - static int tracefs_remount(struct super_block *sb, int *flags, char *data) { int err; @@ -283,7 +257,6 @@ static int tracefs_show_options(struct seq_file *m, struct dentry *root) static const struct super_operations tracefs_super_operations = { .statfs = simple_statfs, .remount_fs = tracefs_remount, - .destroy_inode = tracefs_destroy_inode, .show_options = tracefs_show_options, }; @@ -414,7 +387,6 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *fops) { - struct file_operations *proxy_fops; struct dentry *dentry; struct inode *inode; @@ -430,20 +402,8 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, if (unlikely(!inode)) return failed_creating(dentry); - proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL); - if (unlikely(!proxy_fops)) { - iput(inode); - return failed_creating(dentry); - } - - if (!fops) - fops = &tracefs_file_operations; - - dentry->d_fsdata = (void *)fops; - memcpy(proxy_fops, fops, sizeof(*proxy_fops)); - proxy_fops->open = default_open_file; inode->i_mode = mode; - inode->i_fop = proxy_fops; + inode->i_fop = fops ? fops : &tracefs_file_operations; inode->i_private = data; d_instantiate(dentry, inode); fsnotify_create(dentry->d_parent->d_inode, dentry);