Re: [PATCH 16/18] mm/damon/vaddr: remove attrs param from __damon_va_check_access()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Thread-unsafe static state in access check functions leads to memory access rate corruption across processes.
- [High] Incorrect PTE advancement for large folios in DAMON operations causes skipped memory.
--

commit 126a430836c56a402d2fefdf82a9e58aed70a6d7
Author: SJ Park <sj@kernel.org>

mm/damon/vaddr: remove attrs param from __damon_va_check_access()

The function is not using attrs parameter. Remove it.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 2eaced0765e26..2058db9c01d52 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -493,8 +493,7 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
>   * r	the region to be checked
>   */
>  static void __damon_va_check_access(struct mm_struct *mm,
> -				struct damon_region *r, bool same_target,
> -				struct damon_attrs *attrs)
> +				struct damon_region *r, bool same_target)
>  {
>  	static unsigned long last_addr;
>  	static unsigned long last_folio_sz = PAGE_SIZE;
>  	static bool last_accessed;

[Severity: High]
This isn't a bug introduced by this patch, but does using static variables
here cause a cross-thread data race?

If multiple DAMON contexts (running in separate kdamond threads) monitor
virtual addresses simultaneously, a thread processing one process's virtual
address might read values just written by another thread.

Could this lead to incorrect reuse of the last_accessed result and corrupt
access rate statistics across different processes? A similar pattern appears
to exist in mm/damon/paddr.c:__damon_pa_check_access() as well.

[ ... ]

Additionally, while reviewing this file, I noticed another pre-existing
issue in damos_va_stat_pmd_entry() and damos_va_migrate_pmd_entry():

mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
        ...
        nr = folio_nr_pages(folio);
        s->last_applied = folio;
        ...
}

[Severity: High]
This is a pre-existing issue, but does advancing the PTE by
folio_nr_pages() overshoot the loop bounds when the start address falls
in the middle of a large folio?

Because the address is not necessarily aligned to the start of the large
folio, advancing by the entire folio size might overshoot the end of the
folio and inadvertently skip subsequent PTEs that belong to unrelated
memory. Should the advancement step account for the subpage offset?

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