Re: [RFC PATCH v3 25/28] mm/damon/sysfs-schemes: move memcg_path_to_id() to sysfs-common

[SeongJae Park <sj@kernel.org>]

On Sat, 16 May 2026 19:16:05 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit silently fixes a memcg reference leak by adding a call to `mem_cgroup_iter_break()` without documenting it in the commit message.
> --
> 
> commit 1ef74f20c7bcc208023c4d45c1ced02bf234a75c
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/sysfs-schemes: move memcg_path_to_id() to sysfs-common
> 
> This commit moves the static function damon_sysfs_memcg_path_to_id()
> from sysfs-schemes.c to sysfs-common.c and makes it non-static so it can
> be reused in subsequent commits.
> 
> > The next commit will need to find the memcg id from the user-passed path
> > to the memory cgroup, from sysfs.c.  memcg_path_to_id() is doing that,
> > but defined in sysfs-schemes.c as a static function.  Move the function
> > to sysfs-common.c and mark it as non-static, so that the next commit can
> > reuse the function.
> 
> The commit message states that the patch only moves the function and changes
> its linkage. However, a functional change was included in the moved code.
> 
> [ ... ]
> > diff --git a/mm/damon/sysfs-common.c b/mm/damon/sysfs-common.c
> > index 83e24a9b5a0db..bdc6ae2639e4f 100644
> > --- a/mm/damon/sysfs-common.c
> > +++ b/mm/damon/sysfs-common.c
> [ ... ]
> > @@ -104,3 +104,44 @@ const struct kobj_type damon_sysfs_ul_range_ktype = {
> >  	.default_groups = damon_sysfs_ul_range_groups,
> >  };
> >  
> > +
> > +static bool damon_sysfs_memcg_path_eq(struct mem_cgroup *memcg,
> > +		char *memcg_path_buf, char *path)
> > +{
> > +#ifdef CONFIG_MEMCG
> > +	cgroup_path(memcg->css.cgroup, memcg_path_buf, PATH_MAX);
> > +	if (sysfs_streq(memcg_path_buf, path))
> > +		return true;
> > +#endif /* CONFIG_MEMCG */
> > +	return false;
> > +}
> > +
> > +int damon_sysfs_memcg_path_to_id(char *memcg_path, u64 *id)
> > +{
> > +	struct mem_cgroup *memcg;
> > +	char *path;
> > +	bool found = false;
> > +
> > +	if (!memcg_path)
> > +		return -EINVAL;
> > +
> > +	path = kmalloc_array(PATH_MAX, sizeof(*path), GFP_KERNEL);
> > +	if (!path)
> > +		return -ENOMEM;
> > +
> > +	for (memcg = mem_cgroup_iter(NULL, NULL, NULL); memcg;
> > +			memcg = mem_cgroup_iter(NULL, memcg, NULL)) {
> > +		/* skip offlined memcg */
> > +		if (!mem_cgroup_online(memcg))
> > +			continue;
> > +		if (damon_sysfs_memcg_path_eq(memcg, path, memcg_path)) {
> > +			*id = mem_cgroup_id(memcg);
> > +			found = true;
> > +			mem_cgroup_iter_break(NULL, memcg);
> 
> Is it intentional to include this memcg reference leak fix here?
> 
> It looks like the original code was missing this call to
> mem_cgroup_iter_break() before breaking out of the loop.
> 
> Should this bug fix be split into a separate patch, or documented in the
> commit message so that stable maintainers can backport it if necessary?

Intentional thing, that made to ensure Sashiko can [1] review this.  The final
version of this patch will not have that.

> 
> > +			break;
> > +		}
> > +	}
> > +
> > +	kfree(path);
> > +	return found ? 0 : -EINVAL;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260516183712.81393-1-sj@kernel.org?part=25

[1] https://lore.kernel.org/20260516015001.147547-1-sj@kernel.org


Thanks,
SJ