Re: [PATCH 2/4] mm/vma: write lock source VMA before duplicating it in __split_vma()

[David Hildenbrand (Arm)]

On 10/23/26 21:26, Davidlohr Bueso wrote:
> On Fri, 23 Oct 2026, Lorenzo Stoakes (ARM) wrote:
> 
>> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
>>
>> __split_vma() duplicates the source VMA before taking its VMA write lock,
>> and until the VMA is write-locked, a concurrent thread might set
>> VMA_MAYBE_GUARD_BIT, resulting in the duplication missing the flag.
>>
>> If this happens, then fork may fail to duplicate guard markers and, if a
>> file-backed mapping, THP collapse may also discard the markers.
>>
>> Resolve this by taking the VMA write lock before vm_area_dup() and, since
>> all callers of vm_area_dup() now hold the VMA write lock, assert that this
>> is the case there, and add a comment to explain why it's needed.
>>
>> Fixes: 2ab7f1bbafc9 ("mm/madvise: allow guard page install/remove under VMA
>> lock")
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
>> [ ljs - add VMA write assert in vm_area_dup(), commit message touchup ]
>> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> 
> Reviewed-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> 

Thanks.

> ... with one nit below.
> 
>> ---
>> mm/vma.c      | 2 +-
>> mm/vma_init.c | 3 +++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vma.c b/mm/vma.c
>> index 1be613044b4a..aa59005e2f42 100644
>> --- a/mm/vma.c
>> +++ b/mm/vma.c
>> @@ -538,6 +538,7 @@ __split_vma(struct vma_iterator *vmi, struct
>> vm_area_struct *vma,
>>             return err;
>>     }
>>
>> +    vma_start_write(vma);
>>     new = vm_area_dup(vma);
>>     if (!new)
>>         return -ENOMEM;
>> @@ -568,7 +569,6 @@ __split_vma(struct vma_iterator *vmi, struct
>> vm_area_struct *vma,
>>     if (new->vm_ops && new->vm_ops->open)
>>         new->vm_ops->open(new);
>>
>> -    vma_start_write(vma);
>>     vma_start_write(new);
>>
>>     init_vma_prep(&vp, vma);
>> diff --git a/mm/vma_init.c b/mm/vma_init.c
>> index baa7e82f47e3..d661f229a734 100644
>> --- a/mm/vma_init.c
>> +++ b/mm/vma_init.c
>> @@ -127,6 +127,9 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct
>> *orig)
>> {
>>     struct vm_area_struct *new = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
>>
>> +    /* Must hold a write lock to stabilise atomic VMA flags. */
> 
> should just say "stabilize atomic VMA flags".

You mean the s/stabilise/stabilize/ ?

I guess that's just the (uncommon in the kernel) UK english variant Lorenzo
tends do use.

-- 
Cheers,

David