Re: [RFC PATCH v1.2 13/17] mm/damon/core: remove attrs param from damon_update_region_access_rate()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected static local variables in access check functions cause cross-thread data races and cross-context access measurement pollution.
--

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

mm/damon/core: remove attrs param from damon_update_region_access_rate()

This commit removes the unused attrs parameter from
damon_update_region_access_rate() and updates callers in the core,
physical address, and virtual address monitoring code to match the new
signature.

[ ... ]
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index d0598f5f26882..db48ad274b078 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -93,12 +93,12 @@ static void __damon_pa_check_access(struct damon_region *r,
>  	/* If the region is in the last checked page, reuse the result */
>  	if (ALIGN_DOWN(last_addr, last_folio_sz) ==
>  				ALIGN_DOWN(sampling_addr, last_folio_sz)) {
> -		damon_update_region_access_rate(r, last_accessed, attrs);
> +		damon_update_region_access_rate(r, last_accessed);

[Severity: High]
This is a pre-existing issue, but does relying on last_accessed here cause
data races when multiple kdamond threads run concurrently?

In __damon_pa_check_access(), last_addr, last_folio_sz, and last_accessed
are defined as static variables. Because they are shared globally across all
concurrent damon contexts without synchronization, multiple kdamond threads
will read and write them simultaneously.

Could this allow one thread to incorrectly reuse a last_accessed result
generated by a completely different thread monitoring a different physical
region, corrupting the measurement accuracy?

>  		return;
>  	}

[ ... ]
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index d271476035641..2e12cc628fea0 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -503,19 +503,19 @@ static void __damon_va_check_access(struct mm_struct *mm,
>  	static bool last_accessed;

[Severity: High]
This isn't a bug introduced by this patch, but this static variable shares the
same cross-thread data race issue as the physical address check.

If multiple contexts are monitoring different processes, they will write to
these variables simultaneously.

Does this data race allow one kdamond thread to accidentally reuse the
last_accessed result from another thread, breaking context isolation?

>  
>  	if (!mm) {
> -		damon_update_region_access_rate(r, false, attrs);
> +		damon_update_region_access_rate(r, false);
>  		return;
>  	}

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