Re: [PATCH 1/7] mm/damon/core: initialize damos->last_applied

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

Pre-existing issues:
- [High] Incorrect PTE advancement for large folios in vaddr stat scanning and migration.
- [High] Commit of DAMON parameters corrupts the active scheme's quota state by zeroing dynamic fields in-place.
- [High] Failure in `damos_commit_dests` leaves the active scheme in an inconsistent state, leading to a NULL pointer dereference.
--

--- Patch [1]: [PATCH 1/7] mm/damon/core: initialize damos->last_applied ---
commit 90344a381581ba5ad7a7c87826b18bf948659516
Author: SJ Park <[email protected]>

mm/damon/core: initialize damos->last_applied

This commit fixes an issue where the damos->last_applied field was being used
without initialization, leading to DAMON unexpectedly skipping the application
of DAMOS actions for a folio the first time per scheme. It addresses this by
initializing the field in damon_new_scheme().

Link: https://lore.kernel.org/[email protected]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 603b102ff80f9..20d267c615faf 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -705,6 +705,7 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
>  	INIT_LIST_HEAD(&scheme->ops_filters);
>  	scheme->stat = (struct damos_stat){};
>  	scheme->max_nr_snapshots = 0;
> +	scheme->last_applied = NULL;
>  	INIT_LIST_HEAD(&scheme->list);
>  
>  	scheme->quota = *(damos_quota_init(quota));
                        ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this in-place zeroing
corrupt the active scheme's quota state?

Looking at damon_new_scheme(), it passes the provided quota pointer to
damos_quota_init(), which zeros out dynamic fields directly:

mm/damon/core.c:damos_quota_init() {
    ...
    quota->esz = 0;
    quota->total_charged_sz = 0;
    ...
}

When damon_commit_ctx() applies new parameters to an active context, it copies
schemes by calling damon_new_scheme() with a pointer to the running context's
scheme quota.

Could this wipe out the current quota charge window progress and reset the
goal tuner's feedback loop (esz_bp) on every parameter commit, causing
inconsistent tuning pressure?


[Severity: High]
This is a pre-existing issue, but can the failure path in damos_commit_dests()
lead to a NULL pointer dereference?

In mm/damon/core.c:damos_commit_dests(), if memory allocation fails for
dst->node_id_arr or dst->weight_arr, the function returns -ENOMEM:

mm/damon/core.c:damos_commit_dests() {
    ...
    dst->node_id_arr = kmalloc_array(src->nr_dests,
        sizeof(*dst->node_id_arr), GFP_KERNEL);
    if (!dst->node_id_arr) {
        dst->weight_arr = NULL;
        return -ENOMEM;
    }
    ...
}

However, dst->nr_dests is updated later. If the allocation fails while
updating an active context during __damon_commit_ctx(), could the active
scheme be left with weight_arr == NULL but a non-zero nr_dests?

If so, would this cause a kernel panic when kdamond resumes and calls
damos_va_migrate_dests_add(), which iterates up to dests->nr_dests and
unconditionally dereferences dests->weight_arr[i]?


[Severity: High]
This is a pre-existing issue, but does the PTE iteration in
damos_va_stat_pmd_entry() and damos_va_migrate_pmd_entry() overshoot large
folios?

In both functions, when a valid folio is found, the loop increment is set to
the total number of pages in the folio:

mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
    ...
    if (!damos_va_filter_out(s, folio, vma, addr, pte, NULL))
        *sz_filter_passed += folio_size(folio);
    nr = folio_nr_pages(folio);
    s->last_applied = folio;
    ...
}

If the current PTE maps a tail page of a large folio (for example, if the VMA
starts in the middle of a large folio), wouldn't advancing pte += nr and
addr += nr * PAGE_SIZE overshoot the end of the large folio?

Could this cause DAMON to skip subsequent PTEs in the mapped page table that
might belong to entirely different folios, missing stats and migration actions
for them?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1