Re: [PATCH v3 05/40] mm/vma: ensure mmap_prepare doesn't set actions on a mergeable vma

[David Hildenbrand (Arm)]

On 9/25/26 11:53, Lorenzo Stoakes (ARM) wrote:
> On Fri, Sep 25, 2026 at 12:28:49AM -0700, Suren Baghdasaryan wrote:
>> On Thu, Sep 17, 2026 at 9:25 AM Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
>>>
>>> When a user requests an mmap_action be performed in mmap_prepare, this
>>> involves populating the VMA range with data.
>>>
>>> However, if the VMA is mergeable, it might then mistakenly be merged with
>>> another VMA without having populated the range.
>>>
>>> Every mmap action currently available sets VMA flags such that the VMA
>>> cannot be merged.
>>>
>>> However, to ensure that no future mmap action falls foul of this, assert
>>> that this is the case upon mmap_prepare validation.
>>>
>>> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
>>
>> Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> 
> Thanks!
> 
>>
>>> ---
>>>  mm/vma.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/mm/vma.c b/mm/vma.c
>>> index d6ed10cefc8f..62f2ce1ad5a1 100644
>>> --- a/mm/vma.c
>>> +++ b/mm/vma.c
>>> @@ -2809,6 +2809,15 @@ static int mmap_validate(unsigned long prev_start, unsigned long prev_end,
>>>  int mmap_prepare_validate(const struct vm_area_desc *prev_desc,
>>>                           const struct vm_area_desc *desc)
>>>  {
>>> +       /*
>>> +        * It is not valid to execute mmap actions for VMAs which can be merged,
>>> +        * as any such merge would leave portions of the mapping incorrectly
>>> +        * unmapped.
>>> +        */
>>> +       if (vma_flags_can_merge(&desc->vma_flags) &&
>>> +           WARN_ON_ONCE(desc->action.type != MMAP_NOTHING))
>>
>> Any reason you chose this "if (A && WARN_ON_ONCE(B))" pattern instead
>> of a simpler "if (WARN_ON_ONCE(A && B))"? Unless there are races
>> between A and B updates, I think these would be equivalent, right?
> 
> They are equivalent yes!
> 
> So it's more of a style thing, as that reads less clearly to me:
> 
> 	if (WARN_ON_ONCE(vma_flags_can_merge(&desc->vma_flags) &&
> 			 desc->action.type != MMAP_NOTHING))
> 
> And this version highlights what is the broken thing here (action type set) a
> little more clearly though obviously that's only if mergeable so arguable.
> 
> I don't have a really strong opinion though I can change it if you prefer?

Looks clearer to me like that

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David