Re: [PATCH 6.1.y] mm/damon/sysfs-schemes: kobject_del() scheme dirs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: Missing `kobject_del()` for dynamically sized sysfs array directories (kdamonds, contexts, targets, regions) causes name conflicts on recreation.
--

commit 313d3e99fc72ef8df36e0924dc61e045a278961d
Author: SJ 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.

Link: https://lore.kernel.org/20260628220121.97360-4-sj@kernel.org

> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index bb2f4650741fa..7e4fb112d1154 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -954,6 +954,7 @@ static 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);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this fix for
the schemes directories, it appears the exact same omission of kobject_del()
is still present for other dynamically sized sysfs arrays (kdamonds,
contexts, targets, and regions).

For instance, in damon_sysfs_kdamonds_rm_dirs():

static void damon_sysfs_kdamonds_rm_dirs(struct damon_sysfs_kdamonds *kdamonds)
{
	...
	for (i = 0; i < kdamonds->nr; i++) {
		damon_sysfs_kdamond_rm_dirs(kdamonds_arr[i]);
		kobject_put(&kdamonds_arr[i]->kobj);
	}
	...
}

When updating these directories, the old directories are dropped using only
kobject_put() and new ones are immediately added with kobject_init_and_add()
using the same names. If CONFIG_DEBUG_KOBJECT_RELEASE is enabled or a
reference is held, won't this deferred removal leave the old directory
active and cause the recreation to fail with -EEXIST?

Should kobject_del() also be added to damon_sysfs_kdamonds_rm_dirs(),
damon_sysfs_contexts_rm_dirs(), damon_sysfs_targets_rm_dirs(), and
damon_sysfs_regions_rm_dirs() to prevent these same name conflicts?

> @@ -991,13 +992,15 @@ static int damon_sysfs_schemes_add_dirs(struct damon_sysfs_schemes *schemes,
>  			goto out;
>  		err = damon_sysfs_scheme_add_dirs(scheme);
>  		if (err)
> -			goto out;
> +			goto del_out;
>  
>  		schemes_arr[i] = scheme;
>  		schemes->nr++;
>  	}
>  	return 0;
>  
> +del_out:
> +	kobject_del(&scheme->kobj);
>  out:
>  	damon_sysfs_schemes_rm_dirs(schemes);
>  	kobject_put(&scheme->kobj);

[Severity: High]
This is also a pre-existing issue, but similar to the rm_dirs() paths, the
error paths for adding other array elements appear to be missing
kobject_del().

For example, in damon_sysfs_kdamonds_add_dirs():

	for (i = 0; i < nr_kdamonds; i++) {
		...
		err = kobject_init_and_add(&kdamond->kobj,
				&damon_sysfs_kdamond_ktype, &kdamonds->kobj,
				"%d", i);
		if (err)
			goto out;

		err = damon_sysfs_kdamond_add_dirs(kdamond);
		if (err)
			goto out;
		...
out:
	damon_sysfs_kdamonds_rm_dirs(kdamonds);
	kobject_put(&kdamond->kobj);
	return err;

If adding the child directories via damon_sysfs_kdamond_add_dirs() fails,
the parent kdamond->kobj has already been added to sysfs. Doesn't branching
directly to 'out' leave a lingering sysfs node without calling kobject_del()?

Should a similar 'del_out' error path be added to
damon_sysfs_kdamonds_add_dirs(), damon_sysfs_contexts_add_dirs(), and
damon_sysfs_targets_add_dirs()?

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