Re: [PATCH] ubifs: Fix memory leak in do_rename

["Mårten Lindahl" <martenli@axis.com>]

Hi Zhihao!

On 3/30/23 16:17, Zhihao Cheng wrote:
>> If renaming a file in an encrypted directory, function
>> fscrypt_setup_filename allocates memory for a file name. This name is
>> never used, and before returning to the caller the memory for it is not
>> freed.
>>
>> When running kmemleak on it we see that it is registered as a leak. The
>> report below is triggered by a simple program 'rename' that renames a
>> file in an encrypted directory:
>>
>>    unreferenced object 0xffff888101502840 (size 32):
>>      comm "rename", pid 9404, jiffies 4302582475 (age 435.735s)
>>      backtrace:
>>        __kmem_cache_alloc_node
>>        __kmalloc
>>        fscrypt_setup_filename
>>        do_rename
>>        ubifs_rename
>>        vfs_rename
>>        do_renameat2
>>
>> To fix this we can remove the call to fscrypt_setup_filename as it's not
>> needed.
>>
>> Reported-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>> ---
>>   fs/ubifs/dir.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>
> I'm okay with this modification. It would be better to add a fix tag. 
> (Fixes: 278d9a243635f26 ("ubifs: Rename whiteout atomically"))
>
>
> Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>

Thanks! I'll send a v2 with the Fixes tag and your Reviewed-by tag.

Kind regards

Mårten

>
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index 0f29cf201136..4c2dda1346e7 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -358,7 +358,6 @@ static struct inode *create_whiteout(struct inode 
>> *dir, struct dentry *dentry)
>>       umode_t mode = S_IFCHR | WHITEOUT_MODE;
>>       struct inode *inode;
>>       struct ubifs_info *c = dir->i_sb->s_fs_info;
>> -    struct fscrypt_name nm;
>>         /*
>>        * Create an inode('nlink = 1') for whiteout without updating 
>> journal,
>> @@ -369,10 +368,6 @@ static struct inode *create_whiteout(struct 
>> inode *dir, struct dentry *dentry)
>>       dbg_gen("dent '%pd', mode %#hx in dir ino %lu",
>>           dentry, mode, dir->i_ino);
>>   -    err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
>> -    if (err)
>> -        return ERR_PTR(err);
>> -
>>       inode = ubifs_new_inode(c, dir, mode, false);
>>       if (IS_ERR(inode)) {
>>           err = PTR_ERR(inode);
>> @@ -395,7 +390,6 @@ static struct inode *create_whiteout(struct inode 
>> *dir, struct dentry *dentry)
>>       make_bad_inode(inode);
>>       iput(inode);
>>   out_free:
>> -    fscrypt_free_filename(&nm);
>>       ubifs_err(c, "cannot create whiteout file, error %d", err);
>>       return ERR_PTR(err);
>>   }
>>
>> ---
>> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
>> change-id: 20230330-memleak_rename-fix-6f94a6f99350
>>
>> Best regards,
>>
>