Re: [RFC PATCH] mm/damon/sysfs-schemes: delete tried region in regions_rmdirs()

[SeongJae Park <sj@kernel.org>]

On Sat, 16 May 2026 21:39:58 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Moving `list_del()` out of the kobject release callback causes a Use-After-Free when `kobject_init_and_add()` fails in `damos_sysfs_populate_region_dir()`.
> --
> 
> commit 97136b36d31f71cb6972114db84fe86abd95246f
> Author: SeongJae Park <sj@kernel.org>
> Subject: mm/damon/sysfs-schemes: delete tried region in regions_rmdirs()
> 
> DAMON sysfs maintains the DAMOS tried region directory objects via a
> linked list. When the user requests refresh of the directories, DAMON
> sysfs removes all the region directories first, and then generate
> updated regions directory on the empty space. Deletion of the container
> region object from the linked list is now done inside the removal function
> instead of the kobj release callback.
> 
> [1] https://lore.kernel.org/20260513011920.119183-1-sj@kernel.org
> 
> > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > index 622c3799db87b..aefeb5d22fb7d 100644
> > --- a/mm/damon/sysfs-schemes.c
> > +++ b/mm/damon/sysfs-schemes.c
> > @@ -88,7 +88,6 @@ static void damon_sysfs_scheme_region_release(struct kobject *kobj)
> >  	struct damon_sysfs_scheme_region *region = container_of(kobj,
> >  			struct damon_sysfs_scheme_region, kobj);
> >  
> > -	list_del(&region->list);
> >  	kfree(region);
> >  }
> 
> Does removing list_del() here cause a use-after-free if kobject_init_and_add()
> fails in damos_sysfs_populate_region_dir()?
> 
> If kobject_init_and_add() fails, the error path drops the reference:
> 
> mm/damon/sysfs-schemes.c:damos_sysfs_populate_region_dir() {
>     ...
> 	list_add_tail(&region->list, &sysfs_regions->regions_list);
> 	sysfs_regions->nr_regions++;
> 	if (kobject_init_and_add(&region->kobj,
> 				&damon_sysfs_scheme_region_ktype,
> 				&sysfs_regions->kobj, "%d",
> 				sysfs_regions->nr_regions++)) {
> 		kobject_put(&region->kobj);
> 	}
>     ...
> }
> 
> Because region is added to the list before kobject_init_and_add(), dropping
> the reference will free the region, but it remains in regions_list since
> the release callback no longer unlinks it.

Good finding.  I wrote this patch on a tree having the data attributes
monitoring, which moved the list_add_tail() call to be done after
kobject_init_and_add() success, so missed this.

> 
> Should we add an explicit list_del(&region->list) in the error path of
> damos_sysfs_populate_region_dir() to prevent list corruption?

I will move list_add_tail() call to be done after kobject_init_and_add()
success.

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


Thanks,
SJ