Re: [PATCH 02/13] mm/filemap: convert fault and readahead logic to use vma_flags_t

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Nov 05, 2026 at 09:41:34PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] mm/filemap: use-after-free on vma in do_sync_mmap_readahead()
> - [Low] mm/filemap: inner vma declaration shadows outer vma in do_sync_mmap_readahead()
> --
>
> commit f5f0f8e634a6a2e2b76ad818de67e83f30cb7dea
> Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> mm/filemap: convert fault and readahead logic to use vma_flags_t
>
> This commit replaces direct checks of vm_flags_t values with vma_flags_t
> helpers in the filemap fault and mmap readahead paths.
>
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index b74bc1e5015c6..d8b538bd14043 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -3351,13 +3351,14 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> >  	struct address_space *mapping = file->f_mapping;
> >  	DEFINE_READAHEAD(ractl, file, ra, mapping, vmf->pgoff);
> >  	struct file *fpin = NULL;
> > -	vm_flags_t vm_flags = vmf->vma->vm_flags;
> > +	struct vm_area_struct *vma = vmf->vma;
> >  	bool force_thp_readahead = false;
> >  	unsigned int thp_order = 0;
> >  	unsigned short mmap_miss;
>
> [ ... ]
>
> > @@ -3414,7 +3416,7 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> >  		 * Fetch two folios so we get the chance to actually
> >  		 * readahead, unless we've been told not to.
> >  		 */
> > -		if (!(vm_flags & VM_RAND_READ))
> > +		if (!vma_test(vma, VMA_RAND_READ_BIT))
> >  			ra->size *= 2;
> >  		ra->async_size = folio_nr_pages;
> >  		ra->order = thp_order;
>
> [Severity: High]
> Can this trigger a use-after-free on vma?
>
> Previously, do_sync_mmap_readahead() cached vmf->vma->vm_flags by value in a
> local variable at entry while the fault lock was held, as introduced by
> commit dcfa24ba6899 ("filemap: Cache the value of vm_flags").
>
> In the force_thp_readahead block, maybe_unlock_mmap_for_io() is called before
> this check:
>
> 	if (force_thp_readahead) {
> 		unsigned long folio_nr_pages = 1UL << thp_order;
>
> 		fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> 		ractl._index &= ~(folio_nr_pages - 1);
> 		ra->size = folio_nr_pages;
> 		/*
> 		 * Fetch two folios so we get the chance to actually
> 		 * readahead, unless we've been told not to.
> 		 */
> 		if (!vma_test(vma, VMA_RAND_READ_BIT))
> 			ra->size *= 2;
>
> Since maybe_unlock_mmap_for_io() only pins vmf->vma->vm_file and calls
> release_fault_lock(vmf) to drop mmap_lock or the per-VMA lock, isn't vma
> unprotected against concurrent modification or freeing when vma_test(vma,
> VMA_RAND_READ_BIT) dereferences vma->flags?
>
> > @@ -3422,7 +3424,7 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> >  		return fpin;
> >  	}
> >
> > -	if (vm_flags & VM_EXEC) {
> > +	if (vma_test(vma, VMA_EXEC_BIT)) {
> >  		/*
> >  		 * Allow arch to request a preferred minimum folio order for
> >  		 * executable memory. This can often be beneficial to
>
> [Severity: Low]
> Does the newly added outer vma declaration in do_sync_mmap_readahead() cause
> the existing inner declaration in this VMA_EXEC_BIT block to shadow it?
>
> 	if (vma_test(vma, VMA_EXEC_BIT)) {
> 		...
> 		struct vm_area_struct *vma = vmf->vma;
> 		const unsigned long start = vma_start_pgoff(vma);
> 		const unsigned long end = vma_end_pgoff(vma);
>
> Should the inner vma variable be removed now that vma is declared at function
> scope?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261105-late-2026-vma-flags-v1-0-0d3cd15ab75f@kernel.org?part=2

It seems these are valid, oops!

Andrew suggested a fix off-list, and I agreed to this as an in-place fixup to
avoid respin.

I have also updated the series locally to reflect this change:

----8<----
 mm/filemap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/mm/filemap.c~mm-filemap-convert-fault-and-readahead-logic-to-use-vma_flags_t-fix
+++ a/mm/filemap.c
@@ -3410,6 +3410,7 @@ static struct file *do_sync_mmap_readahe

	if (force_thp_readahead) {
		unsigned long folio_nr_pages = 1UL << thp_order;
+		const bool random_read = vma_test(vma, VMA_RAND_READ_BIT);

		fpin = maybe_unlock_mmap_for_io(vmf, fpin);
		ractl._index &= ~(folio_nr_pages - 1);
@@ -3418,7 +3419,7 @@ static struct file *do_sync_mmap_readahe
		 * Fetch two folios so we get the chance to actually
		 * readahead, unless we've been told not to.
		 */
-		if (!vma_test(vma, VMA_RAND_READ_BIT))
+		if (!random_read)
			ra->size *= 2;
		ra->async_size = folio_nr_pages;
		ra->order = thp_order;
@@ -3438,7 +3439,6 @@ static struct file *do_sync_mmap_readahe
		 * pad that might exist between sections, which would be a waste
		 * of memory.
		 */
-		struct vm_area_struct *vma = vmf->vma;
		const unsigned long start = vma_start_pgoff(vma);
		const unsigned long end = vma_end_pgoff(vma);
		unsigned long ra_end;
_

--
Cheers, Lorenzo