Re: [RFC PATCH 14/27] mm/damon/paddr: support REPORT_MADE type probe filter

[SJ Park <sj@kernel.org>]

On Sun, 15 Nov 2026 20:41:34 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] mm/damon: out-of-bounds and uninitialized read in
>   damon_report_exist()
> - [Medium] mm/damon: phys_addr_t truncation and unit mismatch in
>   damon_pa_filter_match()
> - [Medium] mm/damon: uninitialized filter->report_type in
>   damon_pa_filter_match()
> --
> 
> commit db7c0b8660ac4e99fdce33921b7535218eaa4dc6
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/paddr: support REPORT_MADE type probe filter
> 
> This commit adds support for the DAMON_FILTER_TYPE_REPORT_MADE probe
> filter in the physical address space operation set by querying
> damon_report_exist() over the last sampling interval.
> 
> > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> > index 5b773012a8764..04e95047f8cea 100644
> > --- a/mm/damon/paddr.c
> > +++ b/mm/damon/paddr.c
> > @@ -140,9 +140,10 @@ static void damon_pa_prep_probes(struct damon_ctx *ctx, bool set_samples)
> >  }
> >  
> >  static bool damon_pa_filter_match(struct damon_filter *filter,
> > -		struct folio *folio)
> > +		struct folio *folio, phys_addr_t pa, struct damon_ctx *ctx)
> >  {
> >  	bool matched = false;
> > +	struct damon_addr_range addr_range;
> >  
> >  	switch (filter->type) {
> >  	case DAMON_FILTER_TYPE_PGIDLE_UNSET:
> > @@ -157,19 +158,27 @@ static bool damon_pa_filter_match(struct damon_filter *filter,
> >  		else
> >  			matched = damon_folio_young(folio) == false;
> >  		break;
> > +	case DAMON_FILTER_TYPE_REPORT_MADE:
> > +		addr_range.start = pa;
> > +		addr_range.end = pa + 1;
> 
> [Severity: Medium]
> Can assigning pa and pa + 1 into addr_range truncate 64-bit physical
> addresses and miss reports when ctx->addr_unit > 1?
> 
> struct damon_addr_range defines start and end as unsigned long, so on
> 32-bit PAE/LPAE systems where phys_addr_t is 64-bit, physical addresses
> at or above 4 GiB lose their upper 32 bits (and pa + 1 wraps to 0 when
> the low 32 bits are 0xffffffff).

Missing a report for this edge case is fine.  It only degrades DAMON's
monitoring results, which is anyway best effort.


> In addition, when ctx->addr_unit > 1,
> pa is aligned to ctx->addr_unit, so setting addr_range.end = pa + 1 only
> checks the first byte of the sampled unit instead of [pa, pa +
> ctx->addr_unit).

That's fine.  Technically speaking, the sampling is only for the sampling byte.

> 
> > +		matched = damon_report_exist(filter->report_type, &addr_range,
> > +				jiffies -
> > +				usecs_to_jiffies(ctx->attrs.sample_interval));
> > +		break;
> 
> [Severity: High]
> Does calling damon_report_exist() read uninitialized or out-of-bounds
> elements from damon_report_queue.reports?
> 
> In damon_submit_report(), the report is written at queue->reports[tail]
> and queue->tail is then incremented, so queue->tail points to the next
> slot to be written and is never wrapped when it reaches queue->size:
> 
> mm/damon/core.c:damon_submit_report() {
>     ...
> 	tail = queue->tail;
> 	if (tail == queue->size)
> 		tail = 0;
> 	queue->reports[tail] = *report;
> 	queue->tail++;
>     ...
> }
> 
> mm/damon/core.c:damon_report_exist() {
>     ...
> 	idx = queue->tail;
> 	for (nr_read = 0; nr_read < queue->nr_reports; nr_read++) {
> 		report = &queue->reports[idx];
> 		if (idx)
> 			idx--;
> 		else
> 			idx = queue->size - 1;
>     ...
> }
> 
> Because damon_report_exist() indexes queue->reports[idx] at queue->tail
> before decrementing idx, it reads uninitialized heap memory on the first
> iteration while 1 <= nr_reports < queue->size (and never inspects index
> 0), and reads out of bounds at queue->reports[queue->size] and beyond once
> queue->size reports have been submitted.

Yes, I will fix it in the earlier patch.

> 
> [Severity: Medium]
> Can filter->report_type be uninitialized here when a new
> DAMON_FILTER_TYPE_REPORT_MADE filter is added via damon_commit_ctx()?
> 
> damon_new_filter() allocates struct damon_filter with kmalloc_obj()
> without zeroing the union, and when damon_commit_filters() creates a new
> filter it only copies union members for DAMON_FILTER_TYPE_MEMCG and
> DAMON_FILTER_TYPE_HUGEPAGE_SIZE:
> 
> mm/damon/core.c:damon_commit_filters() {
>     ...
> 		new_filter = damon_new_filter(src_filter->type,
> 				src_filter->matching, src_filter->allow);
> 		if (!new_filter)
> 			return -ENOMEM;
> 		switch (src_filter->type) {
> 		case DAMON_FILTER_TYPE_MEMCG:
> 			new_filter->memcg_id = src_filter->memcg_id;
> 			break;
> 		case DAMON_FILTER_TYPE_HUGEPAGE_SIZE:
> 			if (src_filter->range_max < src_filter->range_min) {
> 				damon_destroy_filter(new_filter);
> 				return -EINVAL;
> 			}
> 			new_filter->range_min = src_filter->range_min;
> 			new_filter->range_max = src_filter->range_max;
> 			break;
> 		default:
> 			break;
> 		}
>     ...
> }
> 
> As a result, new_filter->report_type retains uninitialized slab bytes
> when damon_pa_filter_match() passes it to damon_report_exist().

Yes, I will fix it in the earlier patch.

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


Thanks,
SJ