Re: [PATCH v5 22/38] mm/mlock: clear VMA_LOCKED_MASK over mmap callback

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

On Mon, Oct 12, 2026 at 08:19:03PM +0200, David Hildenbrand (Arm) wrote:
> On 10/9/26 20:04, Lorenzo Stoakes (ARM) wrote:
> > Currently there's a confusing mess around VMA_LOCKED_BIT and
> > VMA_LOCKONFAULT_BIT.
> >
> > It is permitted for drivers to set any flags they like, with the VMA
> > already possessing lock flags.
> >
> > This results in the absurd situation of a VMA possessing both
> > VMA_SPECIAL_FLAGS and VMA_LOCKED_MASK flags, which is not permitted.
> >
> > This has resulted in mlock_vma_folio() having a very silly check for this
> > scenario to work around it.
> >
> > There is no need for this - just clear the flags before invoking the hook
> > and reinstate them afterwards if they are required.
> >
> > Nothing relies upon this being set during the mmap operation.
> >
> > mmap_prepare is unaffected by this so requires no fix.
> >
> > Reviewed-by: Zi Yan <ziy@nvidia.com>
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> > ---
> >  mm/internal.h |  9 +--------
> >  mm/vma.c      | 17 +++++++++++++++++
> >  2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 557e629db8c5..4ae08fedffff 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -970,14 +970,7 @@ void mlock_folio(struct folio *folio);
> >  static inline void mlock_vma_folio(struct folio *folio,
> >  				struct vm_area_struct *vma)
> >  {
> > -	/*
> > -	 * The VM_SPECIAL check here serves two purposes.
> > -	 * 1) VM_IO check prevents migration from double-counting during mlock.
> > -	 * 2) Although mmap_region() and mlock_fixup() take care that VM_LOCKED
> > -	 *    is never left set on a VM_SPECIAL vma, there is an interval while
> > -	 *    file->f_op->mmap() is using vm_insert_page(s), when VM_LOCKED may
> > -	 *    still be set while VM_SPECIAL bits are added: so ignore it then.
> > -	 */
> > +	/* The VM_IO check prevents migration from double-counting during mlock. */
> >  	if (unlikely((vma->vm_flags & (VM_LOCKED|VM_SPECIAL)) == VM_LOCKED))
> >  		mlock_folio(folio);
> >  }
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 39ceb03023e2..4f09fb98a2fd 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -2607,6 +2607,11 @@ static int __mmap_new_file_vma(struct mmap_state *map,
> >  	if (!map->vm_file->f_op->mmap)
> >  		return 0;
> >
> > +	/*
> > +	 * Driver-specified flags may make the lock flags invalid, so clear
> > +	 * VMA_LOCKED_MASK and reinstate it afterwards if appropriate.
> > +	 */
> > +	vma_clear_flags_mask(vma, VMA_LOCKED_MASK);
> >  	error = mmap_file(vma->vm_file, vma);
> >  	map->vm_file = vma->vm_file;
> >
> > @@ -2629,6 +2634,18 @@ static int __mmap_new_file_vma(struct mmap_state *map,
> >  		return error;
> >  	}
> >
> > +	/*
> > +	 * If the VMA is still eligible for mlock(), reinstate any original
> > +	 * VMA_LOCKED_BIT and/or VMA_LOCKONFAULT_BIT flags.
> > +	 */
> > +	if (vma_supports_mlock(vma)) {
> > +		const vma_flags_t mask =
> > +			vma_flags_and_mask(&map->vma_flags,
> > +					   VMA_LOCKED_MASK);
>
> I'd just have at least the = vma_flags_and_mask on the same line. It's ok to
> exceed 80 chars.

I've had people yell at me about this before on review :) you can't really win.

Since I'm doing a respin anyway I can put it on one line.

>
>
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>

Thanks!

--
Cheers, Lorenzo