Re: [RFC PATCH v1.1 01/11] mm/damon/sysfs: kobject_del() target (normal), context and kdamond dirs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak and uninitialized pointer dereference in `damon_sysfs_scheme_add_dirs()` error path due to shifted labels.
- [Medium] Missing `kobject_del()` in `damos_sysfs_probes_rm_dirs()` causes sysfs name conflicts under `CONFIG_DEBUG_KOBJECT_RELEASE`.
- [Medium] Missing `kobject_del()` in `damon_sysfs_schemes_add_dirs()` error path causes sysfs name conflicts.
--

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

mm/damon/sysfs: kobject_del() target (normal), context and kdamond 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 normal creation paths of target, context and kdamond
directories, and error paths of context and kdamond directories by
adding kobject_del() calls.

Cc: <stable@vger.kernel.org> # 5.18.x

> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 2e95e3bac774d..dba1c67fc188f 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c

[ ... ]

> @@ -1642,6 +1643,7 @@ static void damon_sysfs_contexts_rm_dirs(struct damon_sysfs_contexts *contexts)
>  
>  	for (i = 0; i < contexts->nr; i++) {
>  		damon_sysfs_context_rm_dirs(contexts_arr[i]);
> +		kobject_del(&contexts_arr[i]->kobj);
>  		kobject_put(&contexts_arr[i]->kobj);
>  	}
>  	contexts->nr = 0;

[Severity: Medium]
This isn't a bug introduced by this patch, but does damos_sysfs_probes_rm_dirs()
need a similar kobject_del() fix to avoid sysfs name conflicts?

mm/damon/sysfs-schemes.c:damos_sysfs_probes_rm_dirs() {
...
	for (i = 0; i < probes->nr; i++)
		kobject_put(&probes_arr[i]->kobj);
	probes->nr = 0;
...
}

[ ... ]

> @@ -1680,13 +1682,15 @@ static int damon_sysfs_contexts_add_dirs(struct damon_sysfs_contexts *contexts,
>  
>  		err = damon_sysfs_context_add_dirs(context);
>  		if (err)
> -			goto out;
> +			goto del_out;
>  
>  		contexts_arr[i] = context;
>  		contexts->nr++;
>  	}
>  	return 0;
>  
> +del_out:
> +	kobject_del(&context->kobj);
>  out:
>  	damon_sysfs_contexts_rm_dirs(contexts);
>  	kobject_put(&context->kobj);

[Severity: Medium]
This is a pre-existing issue, but does damon_sysfs_schemes_add_dirs() need a
similar kobject_del() fix in its error path?

mm/damon/sysfs-schemes.c:damon_sysfs_schemes_add_dirs() {
...
		err = kobject_init_and_add(&scheme->kobj, ...);
		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;
}

If damon_sysfs_scheme_add_dirs() fails after kobject_init_and_add() succeeds,
the out label puts the reference without unlinking it.

Could this cause a directory creation failure on retry under delayed release?

[Severity: High]
This isn't a bug introduced by this patch, but while looking at error paths,
does damon_sysfs_scheme_add_dirs() have an uninitialized pointer dereference
and a resource leak?

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;
...
}

If damon_sysfs_scheme_set_tried_regions() fails, the code jumps to
put_tried_regions_out and dereferences scheme->tried_regions. 

Since damon_sysfs_scheme_alloc() allocates memory without zeroing it, won't
this dereference an uninitialized pointer?

Additionally, this jump entirely skips cleaning up the successfully
initialized stats object.

Should there be a put_stats_out cleanup label to avoid leaking it?

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