| Message ID | 20190118145344.11532-4-christian@brauner.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | binderfs: debug galore | expand |
On Fri, Jan 18, 2019 at 03:53:42PM +0100, Christian Brauner wrote: > static int binderfs_fill_super(struct super_block *sb, void *data, int silent) > { > + int ret; > struct binderfs_info *info; > - int ret = -ENOMEM; > struct inode *inode = NULL; > struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns; > > @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent) > sb->s_op = &binderfs_super_ops; > sb->s_time_gran = 1; > > - info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL); > - if (!info) > - goto err_without_dentry; > + sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL); > + if (!sb->s_fs_info) > + return -ENOMEM; > + info = sb->s_fs_info; ... and that's when you should grab ipcns reference and stick it into info->ipc_ns, to match the logics in binderfs_kill_super(). Otherwise the failure above > ret = binderfs_parse_mount_opts(data, &info->mount_opts); > if (ret) > - goto err_without_dentry; > + return ret; ... or here leaves you with an ipcns leak. Destructor does if ->s_fs_info is non-NULL release ->s_fs_info->ipc_ns free ->s_fs_info so constructor should not leave object in a state when ipcns is already grabbed, but not stored in ->s_fs_info->ipc_ns (including the case of allocation failure leaving it with NULL ->s_fs_info).
On Fri, Jan 18, 2019 at 11:03:54PM +0000, Al Viro wrote: > On Fri, Jan 18, 2019 at 03:53:42PM +0100, Christian Brauner wrote: > > static int binderfs_fill_super(struct super_block *sb, void *data, int silent) > > { > > + int ret; > > struct binderfs_info *info; > > - int ret = -ENOMEM; > > struct inode *inode = NULL; > > struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns; > > > > @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent) > > sb->s_op = &binderfs_super_ops; > > sb->s_time_gran = 1; > > > > - info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL); > > - if (!info) > > - goto err_without_dentry; > > + sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL); > > + if (!sb->s_fs_info) > > + return -ENOMEM; > > + info = sb->s_fs_info; > > ... and that's when you should grab ipcns reference and stick it into > info->ipc_ns, to match the logics in binderfs_kill_super(). > > Otherwise the failure above > > > ret = binderfs_parse_mount_opts(data, &info->mount_opts); > > if (ret) > > - goto err_without_dentry; > > + return ret; > > ... or here leaves you with an ipcns leak. > > Destructor does > if ->s_fs_info is non-NULL > release ->s_fs_info->ipc_ns > free ->s_fs_info > so constructor should not leave object in a state when ipcns is already > grabbed, but not stored in ->s_fs_info->ipc_ns (including the case of > allocation failure leaving it with NULL ->s_fs_info). Yeah, total brainfart on my side. I shouldn't code in airports apparently... Fixed.
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index 02c96b5edfa9..c0fa495ee994 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -468,8 +468,8 @@ static const struct inode_operations binderfs_dir_inode_operations = { static int binderfs_fill_super(struct super_block *sb, void *data, int silent) { + int ret; struct binderfs_info *info; - int ret = -ENOMEM; struct inode *inode = NULL; struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns; @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_op = &binderfs_super_ops; sb->s_time_gran = 1; - info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL); - if (!info) - goto err_without_dentry; + sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL); + if (!sb->s_fs_info) + return -ENOMEM; + info = sb->s_fs_info; ret = binderfs_parse_mount_opts(data, &info->mount_opts); if (ret) - goto err_without_dentry; + return ret; info->ipc_ns = ipc_ns; info->root_gid = make_kgid(sb->s_user_ns, 0); @@ -511,12 +512,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent) if (!uid_valid(info->root_uid)) info->root_uid = GLOBAL_ROOT_UID; - sb->s_fs_info = info; - - ret = -ENOMEM; inode = new_inode(sb); if (!inode) - goto err_without_dentry; + return -ENOMEM; inode->i_ino = FIRST_INODE; inode->i_fop = &simple_dir_operations; @@ -527,24 +525,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_root = d_make_root(inode); if (!sb->s_root) - goto err_without_dentry; - - ret = binderfs_binder_ctl_create(sb); - if (ret) - goto err_with_dentry; - - return 0; - -err_with_dentry: - dput(sb->s_root); - sb->s_root = NULL; - -err_without_dentry: - put_ipc_ns(ipc_ns); - iput(inode); - kfree(info); + return -ENOMEM; - return ret; + return binderfs_binder_ctl_create(sb); } static struct dentry *binderfs_mount(struct file_system_type *fs_type,
Al pointed out that on binderfs_fill_super() error deactivate_locked_super() will call binderfs_kill_super() so all of the freeing and putting we currently do in binderfs_fill_super() is unnecessary and buggy. Let's simply return errors and let binderfs_fill_super() take care of cleaning up on error. Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Christian Brauner <christian@brauner.io> --- drivers/android/binderfs.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-)