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

[SeongJae Park <sj@kernel.org>]

On Sun, 21 Jun 2026 16:14:56 +0000 sashiko-bot@kernel.org wrote:

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

Nice finding.  Not a blocker of this patch, though.  I will separatelly work on
this.
> 
> >  		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?

Ditto.


Thanks,
SJ

[...]