Re: [RFC PATCH v2.2 15/28] mm/damon/sysfs-schemes: implement tried_regions/<r>/probes/

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Use-After-Free and Double-Put of kobjects due to delayed release
- [High] Lockless list corruption in delayed kobject release
- [High] State variables (`nr_regions`, `total_bytes`) leak on error paths
- [Medium] Undefined Behavior from unvalidated pointer arithmetic on NULL
--

commit 2a0f4a006e3babc368de4c2d048d4ec6e1aec966
Author: SeongJae Park <sj@kernel.org>

mm/damon/sysfs-schemes: implement tried_regions/<r>/probes/

This commit adds a probes/ sysfs directory under DAMOS tried regions to
show per-region probe hit counts.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 245d63808411a..0bca89217b8fa 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
[ ... ]
> @@ -34,10 +61,36 @@ static struct damon_sysfs_scheme_region *damon_sysfs_scheme_region_alloc(
>  	sysfs_region->ar = region->ar;
>  	sysfs_region->nr_accesses = region->nr_accesses_bp / 10000;
>  	sysfs_region->age = region->age;
> +	sysfs_region->probes = NULL;
>  	INIT_LIST_HEAD(&sysfs_region->list);
>  	return sysfs_region;
>  }
>  
> +static int damos_sysfs_region_add_dirs(
> +		struct damon_sysfs_scheme_region *region)
> +{
> +	struct damos_sysfs_probes *probes = damos_sysfs_probes_alloc();
> +	int err;
> +
> +	if (!probes)
> +		return -ENOMEM;
> +	err = kobject_init_and_add(&probes->kobj, &damos_sysfs_probes_ktype,
> +			&region->kobj, "probes");
> +	if (err) {
> +		kobject_put(&probes->kobj);
> +		return err;
> +	}
> +
> +	region->probes = probes;
> +	return 0;
> +}
> +
> +static void damos_sysfs_region_rm_dirs(
> +		struct damon_sysfs_scheme_region *region)
> +{
> +	kobject_put(&region->probes->kobj);

If damos_sysfs_region_add_dirs() fails, region->probes remains NULL.
If damos_sysfs_region_rm_dirs() is subsequently called on this region,
will &region->probes->kobj evaluate the address of a member of a NULL pointer?

While this evaluates to a NULL pointer because kobj is at offset 0, is it
safe to rely on this undefined behavior, or should there be an
if (!region->probes) check?

> +}
> +
>  static ssize_t start_show(struct kobject *kobj, struct kobj_attribute *attr,
>  		char *buf)
>  {
> @@ -165,6 +218,7 @@ static void damon_sysfs_scheme_regions_rm_dirs(
>  
>  	list_for_each_entry_safe(r, next, &regions->regions_list, list) {
>  		/* release function deletes it from the list */
> +		damos_sysfs_region_rm_dirs(r);
>  		kobject_put(&r->kobj);

If the kobject release is delayed (e.g. because userspace holds a sysfs file
open in the region directory, or CONFIG_DEBUG_KOBJECT_RELEASE is enabled),
the region remains on the regions_list.

If a user consecutively writes to the sysfs state file to clear regions,
damon_sysfs_scheme_regions_rm_dirs() will iterate over the same region again.

Will this call kobject_put(&region->probes->kobj) a second time on the
already-freed probes kobject, triggering a use-after-free and refcount
underflow?

Should region->probes be set to NULL after the put to avoid this?

Also, regarding the list management, this isn't a regression introduced here,
but the comment mentions the release function deletes it from the list. If
kobject release runs asynchronously in a worker thread, can the release
function list_del() run concurrently while this loop iterates the list,
silently corrupting the doubly-linked list?

>  		regions->nr_regions--;
>  	}
> @@ -2932,9 +2986,14 @@ 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++)) {
> -		kobject_put(&region->kobj);
> -	}
> +				sysfs_regions->nr_regions++))
> +		goto out;
> +	if (damos_sysfs_region_add_dirs(region))
> +		goto out;
> +	return;
> +
> +out:
> +	kobject_put(&region->kobj);

Earlier in this function, sysfs_regions->total_bytes is increased, and
sysfs_regions->nr_regions is incremented.

If damos_sysfs_region_add_dirs() fails and the code jumps to the out label,
these state variables are not reverted. Since
damon_sysfs_scheme_regions_rm_dirs() only decrements nr_regions by 1 per
region in the list, do these counters permanently drift, causing accounting
issues?

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