When opening a ubifs tmpfile on an encrypted directory, function
fscrypt_setup_filename allocates memory for the name that is to be
stored in the directory entry, but after the name has been copied to the
directory entry inode, the memory 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 'tmpfile' just opening a
tmpfile:
unreferenced object 0xffff88810178f380 (size 32):
comm "tmpfile", pid 509, jiffies 4294934744 (age 1524.742s)
backtrace:
__kmem_cache_alloc_node
__kmalloc
fscrypt_setup_filename
ubifs_tmpfile
vfs_tmpfile
path_openat
Free this memory after it has been copied to the inode.
Signed-off-by: Mårten Lindahl <[email protected]>
---
fs/ubifs/dir.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 0f29cf201136..089ca6910124 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -491,6 +491,7 @@ static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
goto out_cancel;
unlock_2_inodes(dir, inode);
+ fscrypt_free_filename(&nm);
ubifs_release_budget(c, &req);
return finish_open_simple(file, 0);
---
base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
change-id: 20230329-memleak-fix-87a01daf469e
Best regards,
--
Mårten Lindahl <[email protected]>
Hi Mårten,
> When opening a ubifs tmpfile on an encrypted directory, function
> fscrypt_setup_filename allocates memory for the name that is to be
> stored in the directory entry, but after the name has been copied to the
> directory entry inode, the memory 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 'tmpfile' just opening a
> tmpfile:
>
> unreferenced object 0xffff88810178f380 (size 32):
> comm "tmpfile", pid 509, jiffies 4294934744 (age 1524.742s)
> backtrace:
> __kmem_cache_alloc_node
> __kmalloc
> fscrypt_setup_filename
> ubifs_tmpfile
> vfs_tmpfile
> path_openat
>
> Free this memory after it has been copied to the inode.
>
> Signed-off-by: Mårten Lindahl <[email protected]>
> ---
> fs/ubifs/dir.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 0f29cf201136..089ca6910124 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -491,6 +491,7 @@ static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> goto out_cancel;
> unlock_2_inodes(dir, inode);
>
> + fscrypt_free_filename(&nm);
> ubifs_release_budget(c, &req);
>
> return finish_open_simple(file, 0);
Looks good, just one small nit. I'd prefer to add
fscrypt_free_filename() after ubifs_release_budget() just like
ubifs_create/link does, so that ubifs can get unused budget earlier.
After looking through the code, I found another place create_whiteout
has the same problem(Imported in 278d9a243635f26c05("ubifs: Rename
whiteout atomically") by me). Would you mind fixing this point just by
removing unused 'nm' in create_whiteout()?
>
> ---
> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
> change-id: 20230329-memleak-fix-87a01daf469e
>
> Best regards,
>
Hi Zhihao!
On 3/30/23 04:25, Zhihao Cheng wrote:
> Hi Mårten,
>> When opening a ubifs tmpfile on an encrypted directory, function
>> fscrypt_setup_filename allocates memory for the name that is to be
>> stored in the directory entry, but after the name has been copied to the
>> directory entry inode, the memory 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 'tmpfile' just opening a
>> tmpfile:
>>
>> unreferenced object 0xffff88810178f380 (size 32):
>> comm "tmpfile", pid 509, jiffies 4294934744 (age 1524.742s)
>> backtrace:
>> __kmem_cache_alloc_node
>> __kmalloc
>> fscrypt_setup_filename
>> ubifs_tmpfile
>> vfs_tmpfile
>> path_openat
>>
>> Free this memory after it has been copied to the inode.
>>
>> Signed-off-by: Mårten Lindahl <[email protected]>
>> ---
>> fs/ubifs/dir.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index 0f29cf201136..089ca6910124 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -491,6 +491,7 @@ static int ubifs_tmpfile(struct user_namespace
>> *mnt_userns, struct inode *dir,
>> goto out_cancel;
>> unlock_2_inodes(dir, inode);
>> + fscrypt_free_filename(&nm);
>> ubifs_release_budget(c, &req);
>> return finish_open_simple(file, 0);
>
> Looks good, just one small nit. I'd prefer to add
> fscrypt_free_filename() after ubifs_release_budget() just like
> ubifs_create/link does, so that ubifs can get unused budget earlier.
OK, I will move it after ubifs_release_budget.
>
> After looking through the code, I found another place create_whiteout
> has the same problem(Imported in 278d9a243635f26c05("ubifs: Rename
> whiteout atomically") by me). Would you mind fixing this point just by
> removing unused 'nm' in create_whiteout()?
I see what you mean. As I understand it calling fscrypt_setup_filename
is not needed in create_whiteout. I would prefer removing those lines in
a separate patch since that leak is related to do_rename(). If it's OK
with you I can make a patch for that. Would that be OK?
Kind regards
Mårten
>>
>> ---
>> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
>> change-id: 20230329-memleak-fix-87a01daf469e
>>
>> Best regards,
>>
>
Hi Mårten,
> Hi Zhihao!
>
> On 3/30/23 04:25, Zhihao Cheng wrote:
>> Hi Mårten,
>>> When opening a ubifs tmpfile on an encrypted directory, function
>>> fscrypt_setup_filename allocates memory for the name that is to be
>>> stored in the directory entry, but after the name has been copied to the
>>> directory entry inode, the memory 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 'tmpfile' just opening a
>>> tmpfile:
>>>
>>> unreferenced object 0xffff88810178f380 (size 32):
>>> comm "tmpfile", pid 509, jiffies 4294934744 (age 1524.742s)
>>> backtrace:
>>> __kmem_cache_alloc_node
>>> __kmalloc
>>> fscrypt_setup_filename
>>> ubifs_tmpfile
>>> vfs_tmpfile
>>> path_openat
>>>
>>> Free this memory after it has been copied to the inode.
>>>
>>> Signed-off-by: Mårten Lindahl <[email protected]>
>>> ---
>>> fs/ubifs/dir.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>> index 0f29cf201136..089ca6910124 100644
>>> --- a/fs/ubifs/dir.c
>>> +++ b/fs/ubifs/dir.c
>>> @@ -491,6 +491,7 @@ static int ubifs_tmpfile(struct user_namespace
>>> *mnt_userns, struct inode *dir,
>>> goto out_cancel;
>>> unlock_2_inodes(dir, inode);
>>> + fscrypt_free_filename(&nm);
>>> ubifs_release_budget(c, &req);
>>> return finish_open_simple(file, 0);
>>
>> Looks good, just one small nit. I'd prefer to add
>> fscrypt_free_filename() after ubifs_release_budget() just like
>> ubifs_create/link does, so that ubifs can get unused budget earlier.
> OK, I will move it after ubifs_release_budget.
>>
>> After looking through the code, I found another place create_whiteout
>> has the same problem(Imported in 278d9a243635f26c05("ubifs: Rename
>> whiteout atomically") by me). Would you mind fixing this point just by
>> removing unused 'nm' in create_whiteout()?
>
> I see what you mean. As I understand it calling fscrypt_setup_filename
> is not needed in create_whiteout. I would prefer removing those lines in
> a separate patch since that leak is related to do_rename(). If it's OK
> with you I can make a patch for that. Would that be OK?
>
Yes. You may send another patch to fix it. Thanks.