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

[David Hildenbrand (Arm)]

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 at nvidia.com>
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs at 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.


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

-- 
Cheers,

David