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

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

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

Miquel Sabaté Solà @ 2025-09-25 20:26 +02:

> 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.

I clearly read too fast here. I applied your suggestion for v2. Sorry
for the noise!

>
> 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 --]