Re: nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group()

[Markus Elfring]

> I think there is only one object that can be modified in this function,

Such a view can be reasonable.


> so I didn't mention it.

I suggest to reconsider the conclusion.


>> I guess that an imperative wording is preferred also for this change 
>> description.
>
> This sentence is referenced from the code comment, so I haven't change it.
> https://elixir.bootlin.com/linux/v5.7-rc7/source/lib/kobject.c#L459

I find that that there are further possibilities to consider for improvements
around the presented commit message (even after the mentioned copy
from the function description of this programming interface).


>> How do you think about to combine this update step together with
>> “nilfs2: Fix reference count leak in nilfs_sysfs_create_device_group”
>> into a small patch series?
>
> I'd like to improve the similar issues after I reporting this bunch of bugs.

Did you find questionable implementation details with the help of an evolving
source code analysis tool?

Regards,
Markus

Re: [PATCH] nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group()

[Markus Elfring]

> kobject_init_and_add() takes reference even when it fails.

It will be helpful to mention which object is referenced here, won't it?


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

I guess that an imperative wording is preferred also for this change 
description.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=b0c3ba31be3e45a130e13b278cf3b90f69bda6f6#n151

How do you think about to combine this update step together with
“nilfs2: Fix reference count leak in nilfs_sysfs_create_device_group”
into a small patch series?
https://lore.kernel.org/patchwork/patch/1248696/
https://lore.kernel.org/lkml/20200527200933.31135-1-wu000...@umn.edu/

Regards,
Markus

Re: [PATCH] nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group.

[Ryusuke Konishi]

Andrew,

Apply this, please.

Acked-by: Ryusuke Konishi 

On Thu, May 28, 2020 at 4:55 AM  wrote:
>
> From: Qiushi Wu 
>
> kobject_init_and_add() takes reference even when it fails.
> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object. Previous
> commit "b8eb718348b8" fixed a similar problem.
>
> Fixes: a5a7332a291b ("nilfs2: add 
> /sys/fs/nilfs2//mounted_snapshots/group")
> Signed-off-by: Qiushi Wu 
> ---
>  fs/nilfs2/sysfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
> index e60be7bb55b0..b2517c5e773a 100644
> --- a/fs/nilfs2/sysfs.c
> +++ b/fs/nilfs2/sysfs.c
> @@ -209,8 +209,10 @@ int nilfs_sysfs_create_snapshot_group(struct nilfs_root 
> *root)
> "%llu", root->cno);
> }
>
> -   if (err)
> +   if (err) {
> +   kobject_put(>snapshot_kobj);
> return err;
> +   }
>
> return 0;
>  }
> --
> 2.17.1
>

[PATCH] nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group.

[wu000273]

From: Qiushi Wu 

kobject_init_and_add() takes reference even when it fails.
If this function returns an error, kobject_put() must be called to
properly clean up the memory associated with the object. Previous
commit "b8eb718348b8" fixed a similar problem.

Fixes: a5a7332a291b ("nilfs2: add 
/sys/fs/nilfs2//mounted_snapshots/group")
Signed-off-by: Qiushi Wu 
---
 fs/nilfs2/sysfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
index e60be7bb55b0..b2517c5e773a 100644
--- a/fs/nilfs2/sysfs.c
+++ b/fs/nilfs2/sysfs.c
@@ -209,8 +209,10 @@ int nilfs_sysfs_create_snapshot_group(struct nilfs_root 
*root)
"%llu", root->cno);
}
 
-   if (err)
+   if (err) {
+   kobject_put(>snapshot_kobj);
return err;
+   }
 
return 0;
 }
-- 
2.17.1