[PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
@ 2021-09-06  4:13 Dongliang Mu
  2021-09-06  5:43 ` Dongliang Mu
  0 siblings, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-09-06  4:13 UTC (permalink / raw)
  To: Ryusuke Konishi; +Cc: paskripkin, Dongliang Mu, linux-nilfs, linux-kernel

The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
object name. However, it is incomplete to only add kobject_put in the
nilfs_sysfs_delete_device_group. The function
nilfs_sysfs_create_device_group also needs the kobject_put to
free the object name in the error handling part.

Fix this by adding kobject_put in the error handling code of
nilfs_sysfs_create_device_group.

Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 fs/nilfs2/sysfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
index 68e8d61e28dd..7ab60711ca76 100644
--- a/fs/nilfs2/sysfs.c
+++ b/fs/nilfs2/sysfs.c
@@ -1024,6 +1024,7 @@ int nilfs_sysfs_create_device_group(struct super_block *sb)

 cleanup_dev_kobject:
 	kobject_del(&nilfs->ns_dev_kobj);
+	kobject_put(&nilfs->ns_dev_kobj);

 free_dev_subgroups:
 	kfree(nilfs->ns_dev_subgroups);
-- 
2.25.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-06  4:13 [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group Dongliang Mu
@ 2021-09-06  5:43 ` Dongliang Mu
  2021-09-06 15:56   ` Pavel Skripkin
  0 siblings, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-09-06  5:43 UTC (permalink / raw)
  To: Ryusuke Konishi; +Cc: Pavel Skripkin, linux-nilfs, linux-kernel

On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
> object name. However, it is incomplete to only add kobject_put in the
> nilfs_sysfs_delete_device_group. The function
> nilfs_sysfs_create_device_group also needs the kobject_put to
> free the object name in the error handling part.
>
> Fix this by adding kobject_put in the error handling code of
> nilfs_sysfs_create_device_group.

Even after I add this patch, my local syzkaller still reports this
memory leak one hour later. Therefore, there are some other paths or
magics which can trigger the memory leak. I need to dig deeper.

Pavel, do you have any idea about this crash report?

BUG: memory leak
unreferenced object 0xffff88804a1a8a60 (size 32):
  comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
  hex dump (first 32 bytes):
    6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff814750c6>] kstrdup+0x36/0x70
    [<ffffffff81475153>] kstrdup_const+0x53/0x80
    [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
    [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
    [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
    [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
    [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
    [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
    [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
    [<ffffffff81565158>] vfs_get_tree+0x28/0x100
    [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
    [<ffffffff815a3f71>] do_mount+0xa1/0xc0
    [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
    [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
    [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae


>
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  fs/nilfs2/sysfs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
> index 68e8d61e28dd..7ab60711ca76 100644
> --- a/fs/nilfs2/sysfs.c
> +++ b/fs/nilfs2/sysfs.c
> @@ -1024,6 +1024,7 @@ int nilfs_sysfs_create_device_group(struct super_block *sb)
>
>  cleanup_dev_kobject:
>         kobject_del(&nilfs->ns_dev_kobj);
> +       kobject_put(&nilfs->ns_dev_kobj);
>
>  free_dev_subgroups:
>         kfree(nilfs->ns_dev_subgroups);
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-06  5:43 ` Dongliang Mu
@ 2021-09-06 15:56   ` Pavel Skripkin
  2021-09-06 16:47     ` Pavel Skripkin
  2021-09-07  3:37     ` Dongliang Mu
  0 siblings, 2 replies; 8+ messages in thread
From: Pavel Skripkin @ 2021-09-06 15:56 UTC (permalink / raw)
  To: Dongliang Mu, Ryusuke Konishi; +Cc: linux-nilfs, linux-kernel

On 9/6/21 08:43, Dongliang Mu wrote:
> On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>
>> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
>> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
>> object name. However, it is incomplete to only add kobject_put in the
>> nilfs_sysfs_delete_device_group. The function
>> nilfs_sysfs_create_device_group also needs the kobject_put to
>> free the object name in the error handling part.
>>
>> Fix this by adding kobject_put in the error handling code of
>> nilfs_sysfs_create_device_group.
> 
> Even after I add this patch, my local syzkaller still reports this
> memory leak one hour later. Therefore, there are some other paths or
> magics which can trigger the memory leak. I need to dig deeper.
> 
> Pavel, do you have any idea about this crash report?
> 
> BUG: memory leak
> unreferenced object 0xffff88804a1a8a60 (size 32):
>    comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
>    hex dump (first 32 bytes):
>      6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>    backtrace:
>      [<ffffffff814750c6>] kstrdup+0x36/0x70
>      [<ffffffff81475153>] kstrdup_const+0x53/0x80
>      [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
>      [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
>      [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
>      [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
>      [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
>      [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
>      [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
>      [<ffffffff81565158>] vfs_get_tree+0x28/0x100
>      [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
>      [<ffffffff815a3f71>] do_mount+0xa1/0xc0
>      [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
>      [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
>      [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> 

Hi, Dongliang!


This report says nothing to me... It shows, that there is missing 
kobject_put() somewhere. I think, we need a reproducer for this leak, 
otherwise only code review can help :(



With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-06 15:56   ` Pavel Skripkin
@ 2021-09-06 16:47     ` Pavel Skripkin
  2021-09-06 16:51       ` Pavel Skripkin
  2021-09-07  4:00       ` Dongliang Mu
  2021-09-07  3:37     ` Dongliang Mu
  1 sibling, 2 replies; 8+ messages in thread
From: Pavel Skripkin @ 2021-09-06 16:47 UTC (permalink / raw)
  To: Dongliang Mu, Ryusuke Konishi; +Cc: linux-nilfs, linux-kernel

On 9/6/21 18:56, Pavel Skripkin wrote:
> On 9/6/21 08:43, Dongliang Mu wrote:
>> On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>>
>>> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
>>> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
>>> object name. However, it is incomplete to only add kobject_put in the
>>> nilfs_sysfs_delete_device_group. The function
>>> nilfs_sysfs_create_device_group also needs the kobject_put to
>>> free the object name in the error handling part.
>>>
>>> Fix this by adding kobject_put in the error handling code of
>>> nilfs_sysfs_create_device_group.
>> 
>> Even after I add this patch, my local syzkaller still reports this
>> memory leak one hour later. Therefore, there are some other paths or
>> magics which can trigger the memory leak. I need to dig deeper.
>> 
>> Pavel, do you have any idea about this crash report?
>> 
>> BUG: memory leak
>> unreferenced object 0xffff88804a1a8a60 (size 32):
>>    comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
>>    hex dump (first 32 bytes):
>>      6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
>>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>    backtrace:
>>      [<ffffffff814750c6>] kstrdup+0x36/0x70
>>      [<ffffffff81475153>] kstrdup_const+0x53/0x80
>>      [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
>>      [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
>>      [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
>>      [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
>>      [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
>>      [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
>>      [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
>>      [<ffffffff81565158>] vfs_get_tree+0x28/0x100
>>      [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
>>      [<ffffffff815a3f71>] do_mount+0xa1/0xc0
>>      [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
>>      [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
>>      [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
>> 
>> 
> 
> Hi, Dongliang!
> 
> 
> This report says nothing to me... It shows, that there is missing
> kobject_put() somewhere. I think, we need a reproducer for this leak,
> otherwise only code review can help :(
> 
> 
> 

Hm, I guess, I see... We should call kobject_put() in case of 
kobject_init_and_add() failure:

lib/kobject.c:459

  * If this function returns an error, kobject_put() must be called to
  * properly clean up the memory associated with the object.  This is the


so I suggest:

diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
index 68e8d61e28dd..e8717f4ba2a1 100644
--- a/fs/nilfs2/sysfs.c
+++ b/fs/nilfs2/sysfs.c
@@ -1026,6 +1026,7 @@ int nilfs_sysfs_create_device_group(struct 
super_block *sb) 	kobject_del(&nilfs->ns_dev_kobj);

  free_dev_subgroups:
+	kobject_put(&nilfs->ns_dev_kobj);
  	kfree(nilfs->ns_dev_subgroups);

  failed_create_device_group:


This patch should fix 2 memory leaks :)




With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-06 16:47     ` Pavel Skripkin
@ 2021-09-06 16:51       ` Pavel Skripkin
  2021-09-07  4:00       ` Dongliang Mu
  1 sibling, 0 replies; 8+ messages in thread
From: Pavel Skripkin @ 2021-09-06 16:51 UTC (permalink / raw)
  To: Dongliang Mu, Ryusuke Konishi; +Cc: linux-nilfs, linux-kernel

On 9/6/21 19:47, Pavel Skripkin wrote:
> On 9/6/21 18:56, Pavel Skripkin wrote:
>> On 9/6/21 08:43, Dongliang Mu wrote:
>>> On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>>>
>>>> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
>>>> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
>>>> object name. However, it is incomplete to only add kobject_put in the
>>>> nilfs_sysfs_delete_device_group. The function
>>>> nilfs_sysfs_create_device_group also needs the kobject_put to
>>>> free the object name in the error handling part.
>>>>
>>>> Fix this by adding kobject_put in the error handling code of
>>>> nilfs_sysfs_create_device_group.
>>> 
>>> Even after I add this patch, my local syzkaller still reports this
>>> memory leak one hour later. Therefore, there are some other paths or
>>> magics which can trigger the memory leak. I need to dig deeper.
>>> 
>>> Pavel, do you have any idea about this crash report?
>>> 
>>> BUG: memory leak
>>> unreferenced object 0xffff88804a1a8a60 (size 32):
>>>    comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
>>>    hex dump (first 32 bytes):
>>>      6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
>>>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>>    backtrace:
>>>      [<ffffffff814750c6>] kstrdup+0x36/0x70
>>>      [<ffffffff81475153>] kstrdup_const+0x53/0x80
>>>      [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
>>>      [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
>>>      [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
>>>      [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
>>>      [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
>>>      [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
>>>      [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
>>>      [<ffffffff81565158>] vfs_get_tree+0x28/0x100
>>>      [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
>>>      [<ffffffff815a3f71>] do_mount+0xa1/0xc0
>>>      [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
>>>      [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
>>>      [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
>>> 
>>> 
>> 
>> Hi, Dongliang!
>> 
>> 
>> This report says nothing to me... It shows, that there is missing
>> kobject_put() somewhere. I think, we need a reproducer for this leak,
>> otherwise only code review can help :(
>> 
>> 
>> 
> 
> Hm, I guess, I see... We should call kobject_put() in case of
> kobject_init_and_add() failure:
> 
> lib/kobject.c:459
> 
>    * If this function returns an error, kobject_put() must be called to
>    * properly clean up the memory associated with the object.  This is the
> 
> 
> so I suggest:
> 
> diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
> index 68e8d61e28dd..e8717f4ba2a1 100644
> --- a/fs/nilfs2/sysfs.c
> +++ b/fs/nilfs2/sysfs.c
> @@ -1026,6 +1026,7 @@ int nilfs_sysfs_create_device_group(struct
> super_block *sb) 	kobject_del(&nilfs->ns_dev_kobj);
> 
>    free_dev_subgroups:
> +	kobject_put(&nilfs->ns_dev_kobj);
>    	kfree(nilfs->ns_dev_subgroups);
> 
>    failed_create_device_group:
> 
> 
> This patch should fix 2 memory leaks :)
> 
> 

Also, I think, we should add kobject_put() call to

1. nilfs_sysfs_create_##name##_group
2. nilfs_sysfs_create_snapshot_group


To prevent other random kobject memory leaks in nilfs...


With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-06 16:47     ` Pavel Skripkin
  2021-09-06 16:51       ` Pavel Skripkin
@ 2021-09-07  4:00       ` Dongliang Mu
  2021-09-13  5:23         ` Dongliang Mu
  1 sibling, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-09-07  4:00 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: Ryusuke Konishi, linux-nilfs, linux-kernel

On Tue, Sep 7, 2021 at 12:47 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 9/6/21 18:56, Pavel Skripkin wrote:
> > On 9/6/21 08:43, Dongliang Mu wrote:
> >> On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>>
> >>> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
> >>> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
> >>> object name. However, it is incomplete to only add kobject_put in the
> >>> nilfs_sysfs_delete_device_group. The function
> >>> nilfs_sysfs_create_device_group also needs the kobject_put to
> >>> free the object name in the error handling part.
> >>>
> >>> Fix this by adding kobject_put in the error handling code of
> >>> nilfs_sysfs_create_device_group.
> >>
> >> Even after I add this patch, my local syzkaller still reports this
> >> memory leak one hour later. Therefore, there are some other paths or
> >> magics which can trigger the memory leak. I need to dig deeper.
> >>
> >> Pavel, do you have any idea about this crash report?
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff88804a1a8a60 (size 32):
> >>    comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
> >>    hex dump (first 32 bytes):
> >>      6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
> >>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >>    backtrace:
> >>      [<ffffffff814750c6>] kstrdup+0x36/0x70
> >>      [<ffffffff81475153>] kstrdup_const+0x53/0x80
> >>      [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
> >>      [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
> >>      [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
> >>      [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
> >>      [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
> >>      [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
> >>      [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
> >>      [<ffffffff81565158>] vfs_get_tree+0x28/0x100
> >>      [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
> >>      [<ffffffff815a3f71>] do_mount+0xa1/0xc0
> >>      [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
> >>      [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
> >>      [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
> >>
> >>
> >
> > Hi, Dongliang!
> >
> >
> > This report says nothing to me... It shows, that there is missing
> > kobject_put() somewhere. I think, we need a reproducer for this leak,
> > otherwise only code review can help :(
> >
> >
> >
>
> Hm, I guess, I see... We should call kobject_put() in case of
> kobject_init_and_add() failure:
>
> lib/kobject.c:459
>
>   * If this function returns an error, kobject_put() must be called to
>   * properly clean up the memory associated with the object.  This is the
>
>
> so I suggest:
>
> diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
> index 68e8d61e28dd..e8717f4ba2a1 100644
> --- a/fs/nilfs2/sysfs.c
> +++ b/fs/nilfs2/sysfs.c
> @@ -1026,6 +1026,7 @@ int nilfs_sysfs_create_device_group(struct
> super_block *sb)        kobject_del(&nilfs->ns_dev_kobj);
>
>   free_dev_subgroups:
> +       kobject_put(&nilfs->ns_dev_kobj);
>         kfree(nilfs->ns_dev_subgroups);
>
>   failed_create_device_group:
>
>
> This patch should fix 2 memory leaks :)

Yes. We should move kobject_put from the label cleanup_dev_kobject to
free_dev_subgroups.

However, I tried the syz reproducer just sent to you. The memory leak
is still there.

>
>
>
>
> With regards,
> Pavel Skripkin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-07  4:00       ` Dongliang Mu
@ 2021-09-13  5:23         ` Dongliang Mu
  0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2021-09-13  5:23 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: Ryusuke Konishi, linux-nilfs, linux-kernel

There are many commits related to memory leak fix in nilfs_sysfs.

4 days nilfs2: fix memory leak in nilfs_sysfs_delete_snapshot_group
4 days nilfs2: fix memory leak in nilfs_sysfs_create_snapshot_group
4 days nilfs2: fix memory leak in nilfs_sysfs_delete_##name##_group
4 days nilfs2: fix memory leak in nilfs_sysfs_create_##name##_group
4 days nilfs2: fix NULL pointer in nilfs_##name##_attr_release
4 days nilfs2: fix memory leak in nilfs_sysfs_create_device_group

This thread can be closed.

Best regards,
Dongliang Mu

On Tue, Sep 7, 2021 at 12:00 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Tue, Sep 7, 2021 at 12:47 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
> >
> > On 9/6/21 18:56, Pavel Skripkin wrote:
> > > On 9/6/21 08:43, Dongliang Mu wrote:
> > >> On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >>>
> > >>> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
> > >>> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
> > >>> object name. However, it is incomplete to only add kobject_put in the
> > >>> nilfs_sysfs_delete_device_group. The function
> > >>> nilfs_sysfs_create_device_group also needs the kobject_put to
> > >>> free the object name in the error handling part.
> > >>>
> > >>> Fix this by adding kobject_put in the error handling code of
> > >>> nilfs_sysfs_create_device_group.
> > >>
> > >> Even after I add this patch, my local syzkaller still reports this
> > >> memory leak one hour later. Therefore, there are some other paths or
> > >> magics which can trigger the memory leak. I need to dig deeper.
> > >>
> > >> Pavel, do you have any idea about this crash report?
> > >>
> > >> BUG: memory leak
> > >> unreferenced object 0xffff88804a1a8a60 (size 32):
> > >>    comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
> > >>    hex dump (first 32 bytes):
> > >>      6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
> > >>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > >>    backtrace:
> > >>      [<ffffffff814750c6>] kstrdup+0x36/0x70
> > >>      [<ffffffff81475153>] kstrdup_const+0x53/0x80
> > >>      [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
> > >>      [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
> > >>      [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
> > >>      [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
> > >>      [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
> > >>      [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
> > >>      [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
> > >>      [<ffffffff81565158>] vfs_get_tree+0x28/0x100
> > >>      [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
> > >>      [<ffffffff815a3f71>] do_mount+0xa1/0xc0
> > >>      [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
> > >>      [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
> > >>      [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >>
> > >>
> > >
> > > Hi, Dongliang!
> > >
> > >
> > > This report says nothing to me... It shows, that there is missing
> > > kobject_put() somewhere. I think, we need a reproducer for this leak,
> > > otherwise only code review can help :(
> > >
> > >
> > >
> >
> > Hm, I guess, I see... We should call kobject_put() in case of
> > kobject_init_and_add() failure:
> >
> > lib/kobject.c:459
> >
> >   * If this function returns an error, kobject_put() must be called to
> >   * properly clean up the memory associated with the object.  This is the
> >
> >
> > so I suggest:
> >
> > diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
> > index 68e8d61e28dd..e8717f4ba2a1 100644
> > --- a/fs/nilfs2/sysfs.c
> > +++ b/fs/nilfs2/sysfs.c
> > @@ -1026,6 +1026,7 @@ int nilfs_sysfs_create_device_group(struct
> > super_block *sb)        kobject_del(&nilfs->ns_dev_kobj);
> >
> >   free_dev_subgroups:
> > +       kobject_put(&nilfs->ns_dev_kobj);
> >         kfree(nilfs->ns_dev_subgroups);
> >
> >   failed_create_device_group:
> >
> >
> > This patch should fix 2 memory leaks :)
>
> Yes. We should move kobject_put from the label cleanup_dev_kobject to
> free_dev_subgroups.
>
> However, I tried the syz reproducer just sent to you. The memory leak
> is still there.
>
> >
> >
> >
> >
> > With regards,
> > Pavel Skripkin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group
  2021-09-06 15:56   ` Pavel Skripkin
  2021-09-06 16:47     ` Pavel Skripkin
@ 2021-09-07  3:37     ` Dongliang Mu
  1 sibling, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2021-09-07  3:37 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: Ryusuke Konishi, linux-nilfs, linux-kernel

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

On Mon, Sep 6, 2021 at 11:56 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 9/6/21 08:43, Dongliang Mu wrote:
> > On Mon, Sep 6, 2021 at 12:13 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>
> >> The commit 8fd0c1b0647a ("nilfs2: fix memory leak in
> >> nilfs_sysfs_delete_device_group") adds a kobject_put to free the leaking
> >> object name. However, it is incomplete to only add kobject_put in the
> >> nilfs_sysfs_delete_device_group. The function
> >> nilfs_sysfs_create_device_group also needs the kobject_put to
> >> free the object name in the error handling part.
> >>
> >> Fix this by adding kobject_put in the error handling code of
> >> nilfs_sysfs_create_device_group.
> >
> > Even after I add this patch, my local syzkaller still reports this
> > memory leak one hour later. Therefore, there are some other paths or
> > magics which can trigger the memory leak. I need to dig deeper.
> >
> > Pavel, do you have any idea about this crash report?
> >
> > BUG: memory leak
> > unreferenced object 0xffff88804a1a8a60 (size 32):
> >    comm "syz-executor", pid 14551, jiffies 4294960586 (age 14.780s)
> >    hex dump (first 32 bytes):
> >      6c 6f 6f 70 35 00 00 00 00 00 00 00 00 00 00 00  loop5...........
> >      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >    backtrace:
> >      [<ffffffff814750c6>] kstrdup+0x36/0x70
> >      [<ffffffff81475153>] kstrdup_const+0x53/0x80
> >      [<ffffffff822773a2>] kvasprintf_const+0xc2/0x110
> >      [<ffffffff82337c5b>] kobject_set_name_vargs+0x3b/0xe0
> >      [<ffffffff823385ed>] kobject_init_and_add+0x6d/0xc0
> >      [<ffffffff81d2bd08>] nilfs_sysfs_create_device_group+0x98/0x3a0
> >      [<ffffffff81d14fc4>] init_nilfs+0x424/0x580
> >      [<ffffffff81d02962>] nilfs_mount+0x532/0x8c0
> >      [<ffffffff815c754b>] legacy_get_tree+0x2b/0x90
> >      [<ffffffff81565158>] vfs_get_tree+0x28/0x100
> >      [<ffffffff815a3a82>] path_mount+0xb92/0xfe0
> >      [<ffffffff815a3f71>] do_mount+0xa1/0xc0
> >      [<ffffffff815a4584>] __x64_sys_mount+0xf4/0x160
> >      [<ffffffff8433fd35>] do_syscall_64+0x35/0xb0
> >      [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> >
>
> Hi, Dongliang!
>
>
> This report says nothing to me... It shows, that there is missing
> kobject_put() somewhere. I think, we need a reproducer for this leak,
> otherwise only code review can help :(

I have attached a syz reproducer to this email. Even if the kernel is
patched with your patch and my patch, it still triggers a memory leak.

>
>
>
> With regards,
> Pavel Skripkin

[-- Attachment #2: syz --]
[-- Type: application/octet-stream, Size: 13763 bytes --]

# {Threaded:true Collide:true Repeat:true RepeatTimes:0 Procs:8 Slowdown:1 Sandbox:none Fault:false FaultCall:-1 FaultNth:0 Leak:true NetInjection:true NetDevices:true NetReset:true Cgroups:true BinfmtMisc:true CloseFDs:true KCSAN:false DevlinkPCI:false USB:true VhciInjection:true Wifi:true IEEE802154:true Sysctl:true UseTmpDir:true HandleSegv:true Repro:false Trace:false}
r0 = openat2(0xffffffffffffff9c, 0x0, 0x0, 0x0)
syz_mount_image$nilfs2(&(0x7f0000000000), &(0x7f0000000100)='./file0\x00', 0x0, 0x19, &(0x7f0000000200)=[{&(0x7f0000010000)="0200000000003434180100007a4a793440d197c3010000001f00000000000000000010000000000002000000000000001000000005000000010000000000000002000000000000000000000000000000e001000000000000561b675f00000000571b675f00000000571b675f000000000100320000000100561b675f00000000004eed0000000000000000000b00000080002000c0001000e28253618b3b42849b9543a54024813a00"/192, 0xc0, 0x400}, {&(0x7f0000010100)="00000000000000000100"/32, 0x20, 0x500}, {&(0x7f0000010200)="1740cc4333e8f05b11faaf1e400007000000000000000000561b675f0000000010000000000000000b0000000500000030010000000000000100000000000000020000000000000001000000000000000100000001000000010000000000000000000000000000000600000000000000010000000000000003000000030000000200000000000000000000000000000003000000000000000100000000000000040000000000000002000000000000000400000000000000010000000000000001000000010000000500000000000000000000000000000005000000000000000100000000000000010000000100000006000000000000000000000000000000030000000000000001000000000000000300000003000000000000000000000001000000000000000200"/320, 0x140, 0x1000}, {&(0x7f0000010400)="0200000000000000100001022e0000000200000000000000100002022e2e00000b00000000000000e00706012e6e696c667300"/64, 0x40, 0x1800}, {&(0x7f0000010500)="f43f000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000ff0f00"/2080, 0x820, 0x2000}, {&(0x7f0000010e00)="00000000000000000000000000000000561b675f00000000561b675f000000000000000000000000000000000000000000800100"/64, 0x40, 0x3080}, {&(0x7f0000010f00)="01000000000000000008000000000000561b675f00000000561b675f0000000000000000000000000000000000000000ed4102000000000000000000000000000100"/96, 0x60, 0x3100}, {&(0x7f0000011000)="00000000000000000000000000000000561b675f00000000561b675f000000000000000000000000000000000000000000800100"/64, 0x40, 0x3380}, {&(0x7f0000011100)="00000000000000000000000000000000561b675f00000000561b675f000000000000000000000000000000000000000000800100"/64, 0x40, 0x3400}, {&(0x7f0000011200)="00000000000000000000000000000000561b675f00000000561b675f000000000000000000000000000000000000000000800100"/64, 0x40, 0x3480}, {&(0x7f0000011300)="00000000000000000000000000000000561b675f00000000561b675f000000000000000000000000000000000000000000800100"/64, 0x40, 0x3500}, {&(0x7f0000011400)="00000000000000000000000000000000561b675f00000000561b675f0000000000000000000000000000000000000000a4810100"/64, 0x40, 0x3580}, {&(0x7f0000011500)="0100"/32, 0x20, 0x3800}, {&(0x7f0000011600)="0000000000000000000000000000000000000000000000000100000000000000561b675f000000000b000000000000000200000000000000040000000000000003000000000000000000000000000000561b675f00000000561b675f000000000000000000000000000000000000000000800100000000000000000000000000020000000000000003000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000200000000000000", 0xe0, 0x38c0}, {&(0x7f0000011700)="0200000000000000000000000000000000000000000000000300000000000000", 0x20, 0x3a40}, {&(0x7f0000011800)="0200000000000000000000000000000000000000000000000400000000000000", 0x20, 0x3b00}, {&(0x7f0000011900)="0200000000000000000000000000000000000000000000000500000000000000", 0x20, 0x3bc0}, {&(0x7f0000011a00)="0200000000000000000000000000000000000000000000000600000000000000", 0x20, 0x3c80}, {&(0x7f0000011b00)="0200000000000000000000000000000000000000000000000700000000000000", 0x20, 0x3d40}, {&(0x7f0000011c00)="0200000000000000000000000000000000000000000000000800000000000000", 0x20, 0x3e00}, {&(0x7f0000011d00)="0200000000000000000000000000000000000000000000000900000000000000", 0x20, 0x3ec0}, {&(0x7f0000011e00)="1d0000000000000002000000000000001e000000000000000000000000000000561b675f000000000b0000000300000000000000000000000000000003000000", 0x40, 0x4000}, {&(0x7f0000011f00)="f93f0000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000000040000000400000004000007f00"/2080, 0x820, 0x4800}, {&(0x7f0000012800)="03000000000000000100000000000000ffffffffffffffff000000000000000004000000000000000100000000000000ffffffffffffffff000000000000000005000000000000000100000000000000ffffffffffffffff000000000000000006000000000000000100000000000000ffffffffffffffff000000000000000007000000000000000100000000000000ffffffffffffffff000000000000000008000000000000000100000000000000ffffffffffffffff0000", 0xba, 0x5820}, {&(0x7f0000012900)="d348c23990010000561b675f0000000003000000000000000000000000000000561b675f00000000561b675f00000000000000000000000000000000000000000080010000000000000000000000000009000000000000000a000000000000000b000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000561b675f00000000561b675f0000000000000000000000000000000000000000008001000000000000000000000000000500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000561b675f00000000561b675f00000000000000000000000000000000000000000080010000000000000000000000000006", 0x151, 0x6000}], 0x0, &(0x7f0000001580)={[{@order_relaxed}, {@nodiscard}, {@order_relaxed}, {@order_strict}]})
openat$cgroup_netprio_ifpriomap(r0, 0x0, 0x2, 0x0)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-13  5:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-06  4:13 [PATCH] nilfs2: fix memory leak in nilfs_sysfs_create_device_group Dongliang Mu
2021-09-06  5:43 ` Dongliang Mu
2021-09-06 15:56   ` Pavel Skripkin
2021-09-06 16:47     ` Pavel Skripkin
2021-09-06 16:51       ` Pavel Skripkin
2021-09-07  4:00       ` Dongliang Mu
2021-09-13  5:23         ` Dongliang Mu
2021-09-07  3:37     ` Dongliang Mu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®