Messages in this thread | | | Date | Thu, 21 Apr 2022 19:41:15 +0800 | Subject | Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() | From | Qu Wenruo <> |
| |
On 2022/4/21 19:25, Nikolay Borisov wrote: > > > On 21.04.22 г. 14:21 ч., Nikolay Borisov wrote: >> >> >> On 21.04.22 г. 12:51 ч., Haowen Bai wrote: >>> Free "bargs" before return. >>> >>> Signed-off-by: Haowen Bai <baihaowen@meizu.com> >>> --- >>> fs/btrfs/ioctl.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >>> index f08233c2b0b2..d4c8bea914b7 100644 >>> --- a/fs/btrfs/ioctl.c >>> +++ b/fs/btrfs/ioctl.c >>> @@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file >>> *file, void __user *arg) >>> /* this is (2) */ >>> mutex_unlock(&fs_info->balance_mutex); >>> ret = -EINPROGRESS; >>> - goto out; >>> + goto out_bargs; >>> } >>> } else { >>> /* this is (1) */ >>> mutex_unlock(&fs_info->balance_mutex); >>> ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS; >>> - goto out; >>> + goto out_bargs; >>> } >>> locked: >> >> I think this is a better fix: >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 7a6974e877f4..906ed1b93654 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -4391,15 +4391,13 @@ static long btrfs_ioctl_balance(struct file >> *file, void __user *arg) >> goto again; >> } else { >> /* this is (2) */ >> - mutex_unlock(&fs_info->balance_mutex); >> ret = -EINPROGRESS; >> - goto out; >> + goto out_bargs; >> } >> } else { >> /* this is (1) */ >> - mutex_unlock(&fs_info->balance_mutex); >> ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS; >> - goto out; >> + goto out_bargs; >> } >> >> locked: >> > > > Actually to simplify further: > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 7a6974e877f4..bbda55d41a06 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4353,6 +4353,7 @@ static long btrfs_ioctl_balance(struct file *file, > void __user *arg) > bargs = memdup_user(arg, sizeof(*bargs)); > if (IS_ERR(bargs)) { > ret = PTR_ERR(bargs); > + bargs = NULL; > goto out; > } > > @@ -4391,13 +4392,11 @@ static long btrfs_ioctl_balance(struct file > *file, void __user *arg) > goto again; > } else { > /* this is (2) */ > - mutex_unlock(&fs_info->balance_mutex); > ret = -EINPROGRESS; > goto out; > } > } else { > /* this is (1) */ > - mutex_unlock(&fs_info->balance_mutex); > ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS; > goto out; > } > @@ -4406,7 +4405,7 @@ static long btrfs_ioctl_balance(struct file *file, > void __user *arg) > if (bargs->flags & BTRFS_BALANCE_RESUME) { > if (!fs_info->balance_ctl) { > ret = -ENOTCONN; > - goto out_bargs; > + goto out; > } > > bctl = fs_info->balance_ctl; > @@ -4420,18 +4419,18 @@ static long btrfs_ioctl_balance(struct file > *file, void __user *arg) > > if (bargs->flags & ~(BTRFS_BALANCE_ARGS_MASK | > BTRFS_BALANCE_TYPE_MASK)) { > ret = -EINVAL; > - goto out_bargs; > + goto out; > } > > if (fs_info->balance_ctl) { > ret = -EINPROGRESS; > - goto out_bargs; > + goto out; > } > > bctl = kzalloc(sizeof(*bctl), GFP_KERNEL); > if (!bctl) { > ret = -ENOMEM; > - goto out_bargs; > + goto out; > } > > memcpy(&bctl->data, &bargs->data, sizeof(bctl->data)); > @@ -4457,12 +4456,11 @@ static long btrfs_ioctl_balance(struct file > *file, void __user *arg) > } > > kfree(bctl); > -out_bargs:
This looks much better, without the extra label it's easier to read.
But, the @need_unlock variable seems to be uninitialized now, as it's only set in case (3), but case (2) and (1) now goes through out which needs to check @need_unlock.
Thanks, Qu
> +out: > kfree(bargs); > mutex_unlock(&fs_info->balance_mutex); > if (need_unlock) > btrfs_exclop_finish(fs_info); > -out: > mnt_drop_write_file(file); > return ret; > } > > > >
| |