Re: [PATCH v2 1/2] mm: prevent droppable mappings from being locked

[anthony.yznaga@oracle.com]

On 3/11/26 2:27 AM, David Hildenbrand (Arm) wrote:
> On 3/10/26 16:58, Anthony Yznaga wrote:
>> Droppable mappings must not be lockable. There is a check for VMAs with
>> VM_DROPPABLE set in mlock_fixup() along with checks for other types of
>> unlockable VMAs which ensures this when calling mlock()/mlock2().
>>
>> For mlockall(MCL_FUTURE), the check for unlockable VMAs is different.
>> In apply_mlockall_flags(), if the flags parameter has MCL_FUTURE set, the
>> current task's mm's default VMA flag field mm->def_flags has VM_LOCKED
>> applied to it. VM_LOCKONFAULT is also applied if MCL_ONFAULT is also set.
>> When these flags are set as default in this manner they are cleared in
>> __mmap_complete() for new mappings that do not support mlock. A check for
>> VM_DROPPABLE in __mmap_complete() is missing resulting in droppable
>> mappings created with VM_LOCKED set. To fix this and reduce that chance of
>> similar bugs in the future, introduce and use vma_supports_mlock().
>>
>> Fixes: 9651fcedf7b9 ("mm: add MAP_DROPPABLE for designating always lazily freeable mappings")
> Should we Cc: stable? I think we should, to fix mlockall(MCL_FUTURE)
> behavior.

I found this issue through code inspection while doing mshare dev work. 
I don't have a strong idea how likely it is to happen in practice. If it 
did it might not be easily diagnosed so I'm fine adding the tag.


>
>> Suggested-by: David Hildenbrand <david@kernel.org>
>> Signed-off-by: Anthony Yznaga <anthony.yznaga@oracle.com>
>> ---
>> v2:
>>   - Implement vma_supports_mlock() instead of vma flags mask (DavidH)
>>   - Add selftests (Lorenzo)
>>
>>   include/linux/hugetlb_inline.h    |  2 +-
>>   mm/internal.h                     | 10 ++++++++++
>>   mm/mlock.c                        | 10 ++++++----
>>   mm/vma.c                          |  4 +---
>>   tools/testing/vma/include/stubs.h |  5 +++++
>>   5 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/hugetlb_inline.h b/include/linux/hugetlb_inline.h
>> index 593f5d4e108b..755281fab23d 100644
>> --- a/include/linux/hugetlb_inline.h
>> +++ b/include/linux/hugetlb_inline.h
>> @@ -30,7 +30,7 @@ static inline bool is_vma_hugetlb_flags(const vma_flags_t *flags)
>>   
>>   #endif
>>   
>> -static inline bool is_vm_hugetlb_page(struct vm_area_struct *vma)
>> +static inline bool is_vm_hugetlb_page(const struct vm_area_struct *vma)
>>   {
>>   	return is_vm_hugetlb_flags(vma->vm_flags);
>>   }
>> diff --git a/mm/internal.h b/mm/internal.h
>> index cb0af847d7d9..8c67637abcdd 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1218,6 +1218,16 @@ static inline struct file *maybe_unlock_mmap_for_io(struct vm_fault *vmf,
>>   	}
>>   	return fpin;
>>   }
>> +
>> +static inline bool vma_supports_mlock(const struct vm_area_struct *vma)
>> +{
>> +	if (vma->vm_flags & (VM_SPECIAL | VM_DROPPABLE))
>> +		return false;
>> +	if (vma_is_dax(vma) || is_vm_hugetlb_page(vma))
>> +		return false;
>> +	return vma != get_gate_vma(current->mm);
> As discussed, it would be great to find out whether checking
> get_gate_vma() makes any sense here. Likely not. :)

I do think the get_gate_vma() check can go away, but I figured that was 
better done in a separate patch. Is that better done as a separate 
submission or should I tack on patches to this series?


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