Re: [PATCH] mm/damon/sysfs-schemes: fix double increment of nr_regions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incomplete fix leaves nr_regions counter permanently leaking on the error path.
- [High] List corruption and Double Free due to lockless list manipulation in kobject release.
- [Medium] Region directory naming preserves an unintended 1-based indexing regression.
--

commit 8f7ffd21263a0409418cf4d88e9b7e2d7a90117e
Author: Vineet Agarwal <agarwal.vineet2006@gmail.com>

mm/damon/sysfs-schemes: fix double increment of nr_regions

This commit attempts to fix a double increment of nr_regions when populating
region directories. It removes the post-increment from the
kobject_init_and_add call, relying instead on the earlier increment of
sysfs_regions->nr_regions.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 622c3799db87b..5d966ac864193 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -2998,7 +2998,7 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
>  	if (kobject_init_and_add(&region->kobj,
>  				&damon_sysfs_scheme_region_ktype,
>  				&sysfs_regions->kobj, "%d",
> -				sysfs_regions->nr_regions++)) {
> +				sysfs_regions->nr_regions)) {
>  		kobject_put(&region->kobj);
>  	}
>  }

Does this error path leak the counter?

If kobject_init_and_add() fails, kobject_put() triggers
damon_sysfs_scheme_region_release() to delete the region from the list and
free it. However, sysfs_regions->nr_regions has already been incremented
prior to this call and is never decremented on this error path. Because the
cleanup function damon_sysfs_scheme_regions_rm_dirs() only decrements the
counter for items remaining in the list, the counter permanently leaks. After
an error, subsequent directory names will skip numbers and drift indefinitely.

Also, does this preserve an unintended 1-based indexing regression?

Prior to commit 66178e4ec30a, region directories were 0-indexed. By passing
the already-incremented sysfs_regions->nr_regions to kobject_init_and_add(),
the first region directory is named 1. This violates standard DAMON sysfs
array conventions where targets, schemes, and filters all start at 0,
potentially breaking user space tools.

Should the code pass the unincremented sysfs_regions->nr_regions to
kobject_init_and_add() and only increment it on the success path?

Finally, this isn't a bug in this patch, but does the kobject release path
have a lockless list manipulation issue that can cause a double-free?

The function damon_sysfs_scheme_region_release() calls list_del(&region->list)
without holding damon_sysfs_lock. If kobject_release() is delayed (for
example, when CONFIG_DEBUG_KOBJECT_RELEASE is enabled), the region remains
linked in sysfs_regions->regions_list after
damon_sysfs_scheme_regions_rm_dirs() has called kobject_put() on it.

If a subsequent command like update_schemes_tried_regions executes while the
region is awaiting delayed release, damon_sysfs_scheme_regions_rm_dirs() will
traverse the list and call kobject_put() a second time on the same
zero-refcount object. Furthermore, when the delayed release finally executes,
list_del() will run concurrently with other list operations without locks,
resulting in list corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512041157.109845-1-agarwal.vineet2006@gmail.com?part=1