Re: [RFC PATCH v1 3/4] mm/damon: introduce DAMON_HUGEPAGE for hot region hugepage collapsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Silent failure and invalid state when target PID is not found.
- [High] TOCTOU race condition on `aggr_interval` leading to divide-by-zero kernel panic.
- [Medium] Memory leak of DAMON context upon module initialization failure.
--

commit 982630eef231c8c59229da1208ed3240bf24f081
Author: Asier Gutierrez <gutierrez.asier@huawei-partners.com>

mm/damon: introduce DAMON_HUGEPAGE for hot region hugepage collapsing

This patch introduces a new DAMON module (DAMON_HUGEPAGE)
which collapses hot regions into huge pages.

> diff --git a/mm/damon/hugepage.c b/mm/damon/hugepage.c
> new file mode 100644
> index 0000000000000..02729736dc560
> --- /dev/null
> +++ b/mm/damon/hugepage.c

[ ... ]

> +static struct damos *damon_hugepage_new_scheme(void)
> +{
> +	struct damos_access_pattern pattern = {
> +		/* Find regions having PMD_SIZE or larger size */
> +		.min_sz_region = PMD_SIZE,
> +		.max_sz_region = ULONG_MAX,
> +		.min_nr_accesses = 0,
> +		.max_nr_accesses = UINT_MAX,
> +		.min_age_region = min_age /
> +			damon_hugepage_mon_attrs.aggr_interval,

Is there a race condition here? Since damon_hugepage_mon_attrs is exposed
as a module parameter, aggr_interval could be modified locklessly. If it is
set to 0 concurrently between the check in damon_hugepage_apply_parameters()
and this division, could it trigger a divide-by-zero kernel panic?

> +		.max_age_region = UINT_MAX,
> +	};
> +
> +	return damon_new_scheme(
> +		&pattern,
> +		/* synchrounous partial collapse as soon as found */
> +		DAMOS_HUGEPAGE, 0,
> +		/* under the quota. */
> +		&damon_hugepage_quota,
> +		&(struct damos_watermarks){}, NUMA_NO_NODE);
> +}
> +
> +static int damon_hugepage_apply_parameters(void)
> +{
> +	struct damon_ctx *param_ctx;
> +	struct damon_target *param_target;
> +	struct damos *scheme;
> +	struct damos_quota_goal *goal;
> +	struct pid *spid;
> +	int err;
> +
> +	err = damon_modules_new_vaddr_ctx_target(&param_ctx, &param_target);
> +	if (err)
> +		return err;
> +
> +	param_ctx->addr_unit = addr_unit;
> +	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
> +
> +	spid = find_get_pid(monitored_pid);
> +	if (!spid)
> +		goto out;

Does this silently return success when the monitored PID is not found?

Because err is 0 from the successful context creation above, jumping
to the out label returns 0.

This would cause DAMON to start with an empty context without informing
the user that their configuration failed.

> +
> +	param_target->pid = spid;
> +
> +	if (!damon_hugepage_mon_attrs.aggr_interval) {
> +		err = -EINVAL;
> +		goto out;
> +	}

[ ... ]

> +static int __init damon_hugepage_init(void)
> +{
> +	int err;
> +
> +	if (!damon_initialized()) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	err = damon_modules_new_vaddr_ctx_target(&ctx, &target);
> +	if (err)
> +		goto out;
> +
> +	call_control.data = ctx;
> +
> +	/* 'enabled' has set before this function, probably via command line */
> +	if (enabled)
> +		err = damon_hugepage_turn(true);
> +
> +out:
> +	if (err && enabled)
> +		enabled = false;
> +	return err;
> +}

Does this code leak the allocated ctx and target if damon_hugepage_turn()
fails?

Because module initialization failure prevents the module exit routine
from running, it seems damon_destroy_ctx() should be called in the error
path to avoid a memory leak.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430134139.2446417-1-gutierrez.asier@huawei-partners.com?part=3