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

[SeongJae Park <sj@kernel.org>]

On Sun, 17 May 2026 18:27:49 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Moving the `sz_filter_passed` assignment to after `kobject_init_and_add()` exposes uninitialized kernel heap memory to user space.
> - [High] Relying solely on `kobject_put()` to remove directories from sysfs leaves directories visible if references are held.
> --
> 
> commit eda0f7269f695739057cee7bd02d81742d566163
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/sysfs-schemes: delete tried region in regions_rmdirs()
> 
> This commit updates DAMON sysfs to remove region objects from the linked list
> immediately before decrementing the kobject reference count to prevent
> use-after-free races. It also modifies region population to only add the
> region to the list after a successful kobject initialization.
> 
> Link: https://lore.kernel.org/20260513011920.119183-1-sj@kernel.org
> 
> > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > --- a/mm/damon/sysfs-schemes.c
> > +++ b/mm/damon/sysfs-schemes.c
> > @@ -163,7 +163,7 @@ static void damon_sysfs_scheme_regions_rm_dirs(
> >  	struct damon_sysfs_scheme_region *r, *next;
> >  
> >  	list_for_each_entry_safe(r, next, &regions->regions_list, list) {
> > -		/* release function deletes it from the list */
> > +		list_del(&r->list);
> >  		kobject_put(&r->kobj);
> 
> This is not a new regression, but does relying solely on kobject_put() to
> remove directories from sysfs leave them visible if references are still
> held?
> 
> If an elevated reference count exists, such as a process having an open file
> descriptor to a sysfs attribute, the kobject is not immediately released and
> kobject_del() is delayed.
> 
> Could this leave the old region directory accessible in sysfs indefinitely,
> causing stale directories to accumulate alongside new ones? Should
> kobject_del(&r->kobj) be called explicitly before kobject_put() to cleanly
> and synchronously remove the directory?

Makes sense, I will work on this.  Not a blocker of this patch, as Sashiko also
mentioned, though.

> 
> >  		regions->nr_regions--;
> >  	}
> 
> [ ... ]
> 
> > @@ -2925,15 +2925,16 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
> >  	region = damon_sysfs_scheme_region_alloc(r);
> >  	if (!region)
> >  		return;
> > -	region->sz_filter_passed = sz_filter_passed;
> > -	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);
> > +		return;
> >  	}
> > +	region->sz_filter_passed = sz_filter_passed;
> 
> Could moving the sz_filter_passed assignment after kobject_init_and_add()
> expose uninitialized memory to user space?
> 
> If the region object is allocated without zeroing the memory, calling
> kobject_init_and_add() makes the region's kobject immediately visible in
> sysfs.
> 
> If a concurrent read to the sz_filter_passed sysfs attribute occurs during
> this window, could it read the field without locking and leak uninitialized
> kernel heap memory to user space? Should the assignment occur before the
> kobject is registered?

Good finding.  I will set sz_filter_passed before kobject_init_and_add() in the
next version.

> 
> > +	list_add_tail(&region->list, &sysfs_regions->regions_list);
> > +	sysfs_regions->nr_regions++;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260517175915.3352-1-sj@kernel.org?part=1


Thanks,
SJ