Re: [PATCH 14/18] 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 2 potential issue(s) to consider:

New issues:
- [Low] Typo in the commit message referencing the wrong function name.

Pre-existing issues:
- [High] Static local variables used for caching access checks cause data races and cross-context contamination between concurrent DAMON threads.
--

commit 75de4af226707e222be94ee075dde34562b3817d
Author: SJ Park <sj@kernel.org>

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

The damon_update_region_access_rate() function does not use the attrs
parameter. This commit removes the unused parameter to clean up the code.

> damon_region_access_rate() is not using attrs parameter.  Remove it.

[Severity: Low]
This isn't a bug, but there's a typo in the commit message referencing
damon_region_access_rate() instead of damon_update_region_access_rate().

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 5c2da45f988cf..8530213086691 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -91,12 +91,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 using function-scoped static variables
for last_addr, last_folio_sz, and last_accessed here in 
__damon_pa_check_access() cause data races when multiple DAMON threads run 
concurrently?

Since these variables are shared globally across all DAMON physical address 
monitoring threads without synchronization, it seems an interleaved execution
could overwrite the cached access state. Could this result in cross-context
contamination where one thread erroneously reuses last_accessed for a 
completely different memory region?

>  		return;
>  	}
>  
>  	last_accessed = damon_pa_young(sampling_addr, &last_folio_sz);
> -	damon_update_region_access_rate(r, last_accessed, attrs);
> +	damon_update_region_access_rate(r, last_accessed);
>  
>  	last_addr = sampling_addr;
>  }
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index e73ec1ce016e5..2eaced0765e26 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
[ ... ]
> @@ -501,19 +501,19 @@ static void __damon_va_check_access(struct mm_struct *mm,
>  	static bool last_accessed;
>  
>  	if (!mm) {
> -		damon_update_region_access_rate(r, false, attrs);
> +		damon_update_region_access_rate(r, false);
>  		return;
>  	}
>  
>  	/* If the region is in the last checked page, reuse the result */
>  	if (same_target && (ALIGN_DOWN(last_addr, last_folio_sz) ==
>  				ALIGN_DOWN(r->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 also a pre-existing issue, but do the static local variables in
__damon_va_check_access() present the same data race risk?

When multiple concurrent kdamond threads check virtual memory accesses, they
share these unprotected variables. If identical virtual addresses exist in
different monitored processes, could one context reuse the last_accessed result
from another process due to interleaved thread execution?

>  		return;
>  	}
>  
>  	last_accessed = damon_va_young(mm, r->sampling_addr, &last_folio_sz);
> -	damon_update_region_access_rate(r, last_accessed, attrs);
> +	damon_update_region_access_rate(r, last_accessed);
>  
>  	last_addr = r->sampling_addr;
>  }

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