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

[sashiko-bot@kernel.org]

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?

> +			break;
> +		}
> +	}
> +
> +	kfree(path);
> +	return found ? 0 : -EINVAL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516183712.81393-1-sj@kernel.org?part=25