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.
