Re: [RESEND RFC PATCH v2 5/5] mm/damon/vaddr: implement DAMOS_SPLIT handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect virtual address advancement skips valid memory regions, overshoots unaligned folios, and corrupts DAMOS statistics.
- [High] Missing validation for driver-allocated compound pages before calling split_folio_to_order().
--

commit 8064bd2b9a44cd475dbdec8fba8f8de5e1726797
Author: Lian Wang <lianux.mm@gmail.com>

mm/damon/vaddr: implement DAMOS_SPLIT handler
    
Implement the vaddr operations layer handler for DAMOS_SPLIT.
For each folio in the target region that is larger than the
scheme's target_order, split it via split_folio_to_order().
    
This supports both anonymous and file-backed (e.g. tmpfs/shmem)
folios, covering KVM guest memory backed by THP tmpfs.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 8bfc6dcc635d5..73cb61c655fc7 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c

[ ... ]

> @@ -940,6 +940,83 @@ static unsigned long damos_va_collapse(struct damon_target *target,
>  	return applied;
>  }
>  
> +static unsigned long damos_va_split(struct damon_target *target,

[ ... ]

> +		folio_sz = PAGE_SIZE << folio_order(folio);
> +
> +		if (folio_order(folio) > target_order) {
> +			if (!folio_trylock(folio)) {
> +				folio_walk_end(&fw, vma);
> +				goto unlock;
> +			}
> +			folio_get(folio);
> +			folio_walk_end(&fw, vma);
> +
> +			if (!split_folio_to_order(folio, target_order))

[Severity: High]
Can this call to split_folio_to_order() trigger a kernel panic if the folio
is a driver-allocated compound page?

folio_walk_start() can return driver-allocated compound pages mapped into
normal VMAs. These pages will have a folio_order() greater than zero, but
they lack the expected LRU, anonymous, or pagecache metadata. 

Should there be a check here to ensure the folio is splittable (for example,
verifying it is an LRU page) before attempting to split it?

> +				applied += folio_sz;
> +
> +			folio_unlock(folio);
> +			folio_put(folio);
> +			*sz_filter_passed += folio_sz;
> +			addr += folio_sz;

[Severity: High]
Could this advancement logic overshoot the folio boundary?

If the iterator starts in the middle of a large folio, advancing addr by the
unadjusted folio_sz might push the address past the actual end of the folio.

> +		} else {
> +			folio_walk_end(&fw, vma);
> +			*sz_filter_passed += chunk_sz;
> +			addr += chunk_sz;

[Severity: High]
Does advancing by chunk_sz here accidentally skip all subsequent memory
within the 2MB chunk?

If folio_walk_start() returns an order-0 folio or any folio smaller than
target_order, the else branch advances addr by chunk_sz (2MB). This appears
to skip over any remaining folios that might exist in the rest of the chunk.

> +		}
> +		mmap_read_unlock(mm);
> +		cond_resched();
> +		continue;
> +
> +unlock:
> +		*sz_filter_passed += chunk_sz;
> +		addr += chunk_sz;

[Severity: High]
Does advancing by chunk_sz in this error path inflate the DAMOS statistics
and skip over remaining memory?

If a VMA check fails, or if folio_trylock() fails above, the loop jumps
to unlock and adds chunk_sz (2MB) to both sz_filter_passed and addr. This
seems like it would bypass the rest of the memory in the chunk and report
incorrect statistics for the filtered size.

> +		mmap_read_unlock(mm);
> +		cond_resched();
> +	}
> +
> +out_mmput:
> +	mmput(mm);
> +	return applied;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702094633.75658-1-lianux.mm@gmail.com?part=5