Re: Fw:Re: [PATCH] fs: nilfs2: fix memory leak in nilfs sysfs create device group

[Ryusuke Konishi]

Hi Dongliang,

On Sat, Jan 22, 2022 at 9:31 AM Dongliang Mu <mudongliangabcd@xxxxxxxxx> wrote:
> > (added Nanyong Sun to CC)
> > Hi Dongliang,
> >
> > On Thu, Jan 20, 2022 at 11:07 PM Pavel Skripkin <paskripkin@xxxxxxxxx> wrote:
> >
> >
> > Hi Dongliang,
> >
> > On 1/20/22 16:44, Dongliang Mu wrote:
> >
> > The preivous commit 8fd0c1b0647a ("nilfs2: fix memory leak in
> > nilfs_sysfs_delete_device_group") only handles the memory leak in the
> > nilfs_sysfs_delete_device_group. However, the similar memory leak still
> > occurs in the nilfs_sysfs_create_device_group.
> >
> > Fix it by adding kobject_del when
> > kobject_init_and_add succeeds, but one of the following calls fails.
> >
> > Fixes: 8fd0c1b0647a ("nilfs2: fix memory leak in nilfs_sysfs_delete_device_group")
> >
> >
> > Why Fixes tag points to my commit? This issue was introduced before my patch
> >
> >
> > As Pavel pointed out, this patch is independent of his patch.
> > The following one ?
>
> Hi Pavel,
>
> This is an incorrect fixes tag. I need to dig more about `git log -p
> fs/nilfs2/sysfs.c`.
>
> I wonder if there are any automatic or semi-automatic ways to capture
> this fixes tag. Or how do you guys identify the fixes tag?

I guess `git blame fs/nilfs2/sysfs.c` may help you to confirm where the change
came from.   It shows information of commits for every line of the input file.
If you are using github, 'blame button' is available.

If an issue is reproducible, we use `git bisect` to identify the patch
that caused the
issue, however, even then, try to understand why and how it affected
by looking at
source code and the commit.

>
> >
> > 5f5dec07aca7 ("nilfs2: fix memory leak in nilfs_sysfs_create_device_group")
> >
> > Signed-off-by: Dongliang Mu <dzm91@xxxxxxxxxxx>
> > ---
> > fs/nilfs2/sysfs.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> >
> > Can you describe what memory leak issue does this patch actually fix ?
> >
> > It looks like kobject_put() can call __kobject_del() unless circular
> > references exist.
> >
> > kobject_put() -> kref_put() -> kobject_release() ->
> > kobject_cleanup() -> __kobject_del()
> >
> > As explained in Documentation/core-api/kobject.rst,
> >
> > kobject_del() can be used to drop the reference to the parent object, if
> > circular references are constructed.
> >
> > But, at least, the parent object is NULL in this case.
> > I really want to understand what the real problem is.
> >
> > Thanks,
> > Ryusuke Konishi
>
> I know where my problem is. From the disconnect function, I think the
> kobject_del and kobject_put are both necessary without checking the
> documentation of kobjects.
>
> Then I think the current error handling may miss kobject_del, and this
> patch is generated.
>
> As a result, I think we can ignore this patch. Sorry for my false alarm.

Okay, thank you for your reply.
If you notice anything we missed on this difference, please let us know.

Regards,
Ryusuke Konishi