Re: [RFC PATCH 3/9] mm/damon/sysfs-schemes: kobject_del() scheme dirs

[SeongJae Park <sj@kernel.org>]

On Tue, 16 Jun 2026 15:27:02 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incorrect `goto` ladder in `damon_sysfs_scheme_add_dirs` causes an uninitialized pointer dereference and a memory leak of the `stats` kobject.
> - [Medium] Missing `kobject_del()` in the `out:` error paths of `*_add_dirs()` loops leads to delayed sysfs directory removal and name collisions.
> --
> 
> commit a283ef4fe4b682b7b66cf9b57221cb6884b386cc
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/sysfs-schemes: kobject_del() scheme dirs
> 
> On CONFIG_DEBUG_KOBJECT_RELEASE enabled kernel, lack of kobject_del()
> could cause directories creation failures due to the name conflicts. Fix
> those issues for scheme directories by adding kobject_del() calls.
> 
> > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > index 329cfd0bbe9f3..37cf6edb54f17 100644
> > --- a/mm/damon/sysfs-schemes.c
> > +++ b/mm/damon/sysfs-schemes.c
> > @@ -2686,6 +2686,7 @@ void damon_sysfs_schemes_rm_dirs(struct damon_sysfs_schemes *schemes)
> >  
> >  	for (i = 0; i < schemes->nr; i++) {
> >  		damon_sysfs_scheme_rm_dirs(schemes_arr[i]);
> > +		kobject_del(&schemes_arr[i]->kobj);
> >  		kobject_put(&schemes_arr[i]->kobj);
> >  	}
> >  	schemes->nr = 0;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but since we are fixing delayed
> sysfs directory removal here, do we have the same name collision vulnerability
> in the error paths of our *_add_dirs() loops?
> 
> If kobject_init_and_add() succeeds for a scheme, but damon_sysfs_scheme_add_dirs()
> fails, the error path drops the reference without calling kobject_del():
> 
> mm/damon/sysfs-schemes.c:damon_sysfs_schemes_add_dirs() {
>     ...
> 		err = kobject_init_and_add(&scheme->kobj,
> 				&damon_sysfs_scheme_ktype, &schemes->kobj,
> 				"%d", i);
> 		if (err)
> 			goto out;
> 		err = damon_sysfs_scheme_add_dirs(scheme);
> 		if (err)
> 			goto out;
>     ...
> out:
> 	damon_sysfs_schemes_rm_dirs(schemes);
> 	kobject_put(&scheme->kobj);
> 	return err;
> }
> 
> Under CONFIG_DEBUG_KOBJECT_RELEASE, would the delayed kobject_release
> callback leave the "%d" directory around temporarily, causing -EEXIST
> if the user immediately retries?
> 
> The identical pattern appears to exist in damon_sysfs_targets_add_dirs(),
> damon_sysfs_contexts_add_dirs(), and damon_sysfs_kdamonds_add_dirs().

Good catch, I will fix thse in the next revision.

> 
> [Severity: High]
> This is a pre-existing issue, but there appears to be an uninitialized pointer
> dereference and a memory leak in the goto ladder of
> damon_sysfs_scheme_add_dirs().
> 
> If damon_sysfs_scheme_set_tried_regions() fails, we jump to
> put_tried_regions_out:
> 
> mm/damon/sysfs-schemes.c:damon_sysfs_scheme_add_dirs() {
>     ...
> 	err = damon_sysfs_scheme_set_stats(scheme);
> 	if (err)
> 		goto put_filters_watermarks_quotas_access_pattern_out;
> 	err = damon_sysfs_scheme_set_tried_regions(scheme);
> 	if (err)
> 		goto put_tried_regions_out;
> 	return 0;
> 
> put_tried_regions_out:
> 	kobject_put(&scheme->tried_regions->kobj);
> 	scheme->tried_regions = NULL;
> put_filters_watermarks_quotas_access_pattern_out:
>     ...
> }
> 
> Because scheme is allocated via kmalloc_obj() (which wraps kmalloc and doesn't
> zero memory), and the failing set_tried_regions() doesn't initialize
> scheme->tried_regions on error, doesn't scheme->tried_regions contain an
> uninitialized value here? Dereferencing it would cause a panic.

Good finding, but this deserves another patch.  I will separately work on this.

> 
> Additionally, this jump target falls through to
> put_filters_watermarks_quotas_access_pattern_out without ever calling
> kobject_put(&scheme->stats->kobj), so wouldn't the successfully allocated
> stats kobject be permanently leaked?

Good finding, but you reported this in another reply to this thread and I
promised to separately work on this.


Thanks,
SJ

[...]