Re: [PATCH 14/18] mm/damon/core: remove attrs param from damon_update_region_access_rate()

[SJ Park <sj@kernel.org>]

On Tue, 30 Jun 2026 04:19:23 +0000 sashiko-bot@kernel.org wrote:

> 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().

Good finding.  I will ask Andrew to fix up when he picks this, unless I end up
posting a new revision.

> 
> > 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?

The same issue was found by Sashiko before, and I'm separately working on it.
Not a blocker of this issue, anyway.

> 
> >  		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?

Ditto.


Thanks,
SJ

[...]