Re: [PATCH] btrfs: ioctl: Fix memory leak on duplicated memory

["Miquel Sabaté Solà" <mssola@mssola.com>]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

Filipe Manana @ 2025-09-25 18:48 +01:

> On Thu, Sep 25, 2025 at 6:25 PM Boris Burkov <boris@bur.io> wrote:
>>
>> On Thu, Sep 25, 2025 at 04:53:31PM +0200, Miquel Sabaté Solà wrote:
>> > On 'btrfs_ioctl_qgroup_assign' we first duplicate the argument as
>> > provided by the user, which is kfree'd in the end. But this was not the
>> > case when allocating memory for 'prealloc'. In this case, if it somehow
>> > failed, then the previous code would go directly into calling
>> > 'mnt_drop_write_file', without freeing the string duplicated from the
>> > user space.
>> >
>> > Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
>>
>> LGTM, thanks for the fix!
>>
>> One thing though: I don't like the label names. I think with multiple
>> cleanups the best way is to name each label with the cleanup it is for.
>> Once you have some named ones, "out" feels unspecific, and encoding
>> every single action like "out_sa_drop_write" doesn't scale as you add
>> more cleanups, so it's just not a useful pattern. It's already quite
>> clunky with just two.
>>
>> If you fixup the names, you can add:
>>
>> Reviewed-by: Boris Burkov <boris@bur.io>
>>
>> > ---
>> >  fs/btrfs/ioctl.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> > index 185bef0df1c2..00381fdbff9d 100644
>> > --- a/fs/btrfs/ioctl.c
>> > +++ b/fs/btrfs/ioctl.c
>> > @@ -3740,7 +3740,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>> >               prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
>> >               if (!prealloc) {
>> >                       ret = -ENOMEM;
>> > -                     goto drop_write;
>> > +                     goto out_sa_drop_write;
>> >               }
>> >       }
>> >
>> > @@ -3775,6 +3775,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>> >
>> >  out:
>>
>> call this free_prealloc
>>
>> >       kfree(prealloc);
>> > +out_sa_drop_write:
>>
>> and this one free_args
>
>
> Rather than adding yet one more label, which over time has proven
> error prone, I'd rather have a single label.
> Just the existing 'out' label and then the fix would be to replace the
>
> goto drop_write;
>
> with
>
> goto out;
>
> kfree() against a NULL pointer is safe.

I wanted to keep it simple and just fix the issue at hand. Actually I
found out about this as part of a larger refactoring involving cleanup
functions [1], which would fix the amount of labels as well.

Hence, as David mentions on another email, I would handle cleaning up
the amount of labels as part of another series.

>
> Also, missing a Fixes tag which should be:
>
> Fixes: 4addc1ffd67a ("btrfs: qgroup: preallocate memory before adding
> a relation")

I will add it as part of v2, thanks!

>
> Thanks.
>
>>
>> >       kfree(sa);
>> >  drop_write:
>> >       mnt_drop_write_file(file);
>> > --
>> > 2.51.0
>> >
>>

Thanks for the review,
Miquel

[1] https://lore.kernel.org/all/87plbh4qe9.fsf@/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]