Re: [PATCH v2 2/3] mm/damon/sysfs: support hugepage_mem_bp quota goal metric

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Kernel panic if user sets DAMOS quota goal target_value to 0 via sysfs.
- [High] Potential kernel panic in `damos_hugepage_mem_bp()` if system memory usage drops to 0.
- [High] Potential kernel panic in `damos_get_in_active_mem_bp()` if LRU lists are empty.
--

commit 2cad27ab47cb3fdb7470dc5639b19615c6ec257b
Author: Asier Gutierrez <gutierrez.asier@huawei-partners.com>
Subject: mm/damon/sysfs: support hugepage_mem_bp quota goal metric

This commit adds the DAMOS_QUOTA_HUGEPAGE_MEM_BP metric to the sysfs
interface, allowing users to configure the hugepage_mem_bp quota goal
metric for DAMOS autotune policies.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 329cfd0bbe9f3..4b5429a5b4158 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1267,6 +1267,10 @@ struct damos_sysfs_qgoal_metric_name damos_sysfs_qgoal_metric_names[] = {
>  		.metric = DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP,
>  		.name = "node_eligible_mem_bp",
>  	},
> +	{
> +		.metric = DAMOS_QUOTA_HUGEPAGE_MEM_BP,
> +		.name = "hugepage_mem_bp",
> +	},

[Severity: High]
This isn't a bug introduced by this patch, but exposing this metric makes a
pre-existing divide-by-zero pathway reachable. Can this cause a kernel panic
if system memory usage drops to 0?

Looking at damos_hugepage_mem_bp() in mm/damon/core.c, it calculates total
without verifying it is greater than zero:

mm/damon/core.c:damos_hugepage_mem_bp() {
    ...
    total = totalram_pages() - global_zone_page_state(NR_FREE_PAGES);
    return mult_frac(thp, 10000, total);
}

If the number of free pages equals totalram_pages() (which is possible on
mostly empty systems, in tightly constrained VMs, or due to vmstat per-CPU
drift), total becomes 0. Could this cause a divide-by-zero panic during
quota adjustment?

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this sysfs
exposure, I noticed another pre-existing divide-by-zero risk if LRU lists
are empty. Does this code safely handle zero total active/inactive memory?

mm/damon/core.c:damos_get_in_active_mem_bp() {
    ...
    total = active + inactive;
    if (active_ratio)
        return mult_frac(active, 10000, total);
    return mult_frac(inactive, 10000, total);
}

If there are no pages on the active and inactive LRU lists (e.g., in systems
relying solely on Hugetlbfs, unevictable memory, or during early
initialization), total evaluates to 0. Does this need a check to avoid a
divide-by-zero panic?

[Severity: High]
This isn't a bug introduced by this patch, but since we are looking at quota
goals, is there a risk of a kernel panic if a user sets target_value to 0
via sysfs?

Looking at target_value_store() in mm/damon/sysfs-schemes.c, user input is
directly accepted:

mm/damon/sysfs-schemes.c:target_value_store() {
    ...
    int err = kstrtoul(buf, 0, &goal->target_value);
    ...
}

Then in damos_quota_score() in mm/damon/core.c, this target_value is used as
a denominator:

mm/damon/core.c:damos_quota_score() {
    ...
    highest_score = max(highest_score,
                mult_frac(goal->current_value, 10000,
                    goal->target_value));
    ...
}

Since kdamond will trigger a divide-by-zero if target_value is 0, should
there be validation in target_value_store() to reject a value of 0?

>  };
>  
>  static ssize_t target_metric_show(struct kobject *kobj,

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