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

[Mårten Lindahl]

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@xxxxxxxxxx>
> > Signed-off-by: Mårten Lindahl <marten.lindahl@xxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>

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,