Re: [PATCH v2 07/13] mm: update secretmem to use VMA flags on mmap_prepare

Lorenzo Stoakes <[email protected]> wrote:
> This patch updates secretmem to use the new vma_flags_t type which will
> soon supersede vm_flags_t altogether.
> 
> In order to make this change we also have to update mlock_future_ok(), we
> replace the vm_flags_t parameter with a simple boolean is_vma_locked one,
> which also simplifies the invocation here.
> 
> This is laying the groundwork for eliminating the vm_flags_t in
> vm_area_desc and more broadly throughout the kernel.
> 

Hi everyone,

Running mm-new through AI review flagged this one:

> diff --git a/mm/mmap.c b/mm/mmap.c
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -225,12 +225,12 @@ static inline unsigned long round_hint_to_min(unsigned long hint)
>  	return hint;
>  }
>
> -bool mlock_future_ok(const struct mm_struct *mm, vm_flags_t vm_flags,
> -			unsigned long bytes)
> +bool mlock_future_ok(const struct mm_struct *mm, bool is_vma_locked,
> +		     unsigned long bytes)
>  {
>  	unsigned long locked_pages, limit_pages;
>
> -	if (!(vm_flags & VM_LOCKED) || capable(CAP_IPC_LOCK))
> +	if (!is_vma_locked || capable(CAP_IPC_LOCK))
>  		return true;

[ ... ]

> @@ -416,7 +416,7 @@
>  		if (!can_do_mlock())
>  			return -EPERM;
>
> -	if (!mlock_future_ok(mm, vm_flags, len))
> +	if (!mlock_future_ok(mm, vm_flags & VM_LOCKED, len))
>  		return -EAGAIN;

This looks correct - do_mmap() now extracts the VM_LOCKED bit from
vm_flags and passes that to mlock_future_ok().

However, it appears check_brk_limits() was not updated when
mlock_future_ok() changed its signature. The function at mm/mmap.c:111
still passes the raw def_flags value:

    return mlock_future_ok(current->mm, current->mm->def_flags, len)
        ? 0 : -EAGAIN;

When def_flags has any bit set, this converts to true, incorrectly
triggering mlock limit checks even when VM_LOCKED is not set. Should
this be passing (current->mm->def_flags & VM_LOCKED) instead?

[ ... ]

Additionally, the test stub at tools/testing/vma/vma_internal.h:1627
still has the old signature (mm, vm_flags_t vm_flags, bytes) while the
production code now uses (mm, bool is_vma_locked, bytes). This could
cause compilation issues or mask bugs in the test suite.