[PATCH] mm: set the vma flags dirty before testing if it is mergeable

[PATCH] mm: set the vma flags dirty before testing if it is mergeable

[Muhammad Usama Anjum]

The VM_SOFTDIRTY should be set in the vma flags to be tested if new
allocation should be merged in previous vma or not. With this patch,
the new allocations are merged in the previous VMAs.

I've tested it by reverting the commit 34228d473efe ("mm: ignore
VM_SOFTDIRTY on VMA merging") and after adding this following patch,
I'm seeing that all the new allocations done through mmap() are merged
in the previous VMAs. The number of VMAs doesn't increase drastically
which had contributed to the crash of gimp. If I run the same test after
reverting and not including this patch, the number of VMAs keep on
increasing with every mmap() syscall which proves this patch.

The commit 34228d473efe ("mm: ignore VM_SOFTDIRTY on VMA merging")
seems like a workaround. But it lets the soft-dirty and non-soft-dirty
VMA to get merged. It helps in avoiding the creation of too many VMAs.
But it creates the problem while adding the feature of clearing the
soft-dirty status of only a part of the memory region.

Cc: <stable@vger.kernel.org>
Fixes: d9104d1ca966 ("mm: track vma changes with VM_SOFTDIRTY bit")
Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
We need more testing of this patch.

While implementing clear soft-dirty bit for a range of address space, I'm
facing an issue. The non-soft dirty VMA gets merged sometimes with the soft
dirty VMA. Thus the non-soft dirty VMA become dirty which is undesirable.
When discussed with the some other developers they consider it the
regression. Why the non-soft dirty page should appear as soft dirty when it
isn't soft dirty in reality? I agree with them. Should we revert
34228d473efe or find a workaround in the IOCTL?

* Revert may cause the VMAs to expand in uncontrollable situation where the
soft dirty bit of a lot of memory regions or the whole address space is
being cleared again and again. AFAIK normal process must either be only
clearing a few memory regions. So the applications should be okay. There is
still chance of regressions if some applications are already using the
soft-dirty bit. I'm not sure how to test it.

* Add a flag in the IOCTL to ignore the dirtiness of VMA. The user will
surely lose the functionality to detect reused memory regions. But the
extraneous soft-dirty pages would not appear. I'm trying to do this in the
patch series [1]. Some discussion is going on that this fails with some
mprotect use case [2]. I still need to have a look at the mprotect selftest
to see how and why this fails. I think this can be implemented after some
more work probably in mprotect side.

[1] https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/
[2] https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/
---
 mm/mmap.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index f9b96b387a6f..6934b8f61fdc 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1708,6 +1708,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		vm_flags |= VM_ACCOUNT;
 	}
 
+	/*
+	 * New (or expanded) vma always get soft dirty status.
+	 * Otherwise user-space soft-dirty page tracker won't
+	 * be able to distinguish situation when vma area unmapped,
+	 * then new mapped in-place (which must be aimed as
+	 * a completely new data area).
+	 */
+	vm_flags |= VM_SOFTDIRTY;
+
 	/*
 	 * Can we just expand an old mapping?
 	 */
@@ -1823,15 +1832,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	if (file)
 		uprobe_mmap(vma);
 
-	/*
-	 * New (or expanded) vma always get soft dirty status.
-	 * Otherwise user-space soft-dirty page tracker won't
-	 * be able to distinguish situation when vma area unmapped,
-	 * then new mapped in-place (which must be aimed as
-	 * a completely new data area).
-	 */
-	vma->vm_flags |= VM_SOFTDIRTY;
-
 	vma_set_page_prot(vma);
 
 	return addr;
@@ -2998,6 +2998,8 @@ static int do_brk_flags(unsigned long addr, unsigned long len, unsigned long fla
 	if (security_vm_enough_memory_mm(mm, len >> PAGE_SHIFT))
 		return -ENOMEM;
 
+	flags |= VM_SOFTDIRTY;
+
 	/* Can we just expand an old private anonymous mapping? */
 	vma = vma_merge(mm, prev, addr, addr + len, flags,
 			NULL, NULL, pgoff, NULL, NULL_VM_UFFD_CTX, NULL);
@@ -3026,7 +3028,6 @@ static int do_brk_flags(unsigned long addr, unsigned long len, unsigned long fla
 	mm->data_vm += len >> PAGE_SHIFT;
 	if (flags & VM_LOCKED)
 		mm->locked_vm += (len >> PAGE_SHIFT);
-	vma->vm_flags |= VM_SOFTDIRTY;
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH] mm: set the vma flags dirty before testing if it is mergeable

[David Hildenbrand]

On 22.11.22 09:24, Muhammad Usama Anjum wrote:
> The VM_SOFTDIRTY should be set in the vma flags to be tested if new
> allocation should be merged in previous vma or not. With this patch,
> the new allocations are merged in the previous VMAs.
> 
> I've tested it by reverting the commit 34228d473efe ("mm: ignore
> VM_SOFTDIRTY on VMA merging") and after adding this following patch,
> I'm seeing that all the new allocations done through mmap() are merged
> in the previous VMAs. The number of VMAs doesn't increase drastically
> which had contributed to the crash of gimp. If I run the same test after
> reverting and not including this patch, the number of VMAs keep on
> increasing with every mmap() syscall which proves this patch.
> 
> The commit 34228d473efe ("mm: ignore VM_SOFTDIRTY on VMA merging")
> seems like a workaround. But it lets the soft-dirty and non-soft-dirty
> VMA to get merged. It helps in avoiding the creation of too many VMAs.
> But it creates the problem while adding the feature of clearing the
> soft-dirty status of only a part of the memory region.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: d9104d1ca966 ("mm: track vma changes with VM_SOFTDIRTY bit")
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> We need more testing of this patch.
> 
> While implementing clear soft-dirty bit for a range of address space, I'm
> facing an issue. The non-soft dirty VMA gets merged sometimes with the soft
> dirty VMA. Thus the non-soft dirty VMA become dirty which is undesirable.
> When discussed with the some other developers they consider it the
> regression. Why the non-soft dirty page should appear as soft dirty when it
> isn't soft dirty in reality? I agree with them. Should we revert
> 34228d473efe or find a workaround in the IOCTL?
> 
> * Revert may cause the VMAs to expand in uncontrollable situation where the
> soft dirty bit of a lot of memory regions or the whole address space is
> being cleared again and again. AFAIK normal process must either be only
> clearing a few memory regions. So the applications should be okay. There is
> still chance of regressions if some applications are already using the
> soft-dirty bit. I'm not sure how to test it.
> 
> * Add a flag in the IOCTL to ignore the dirtiness of VMA. The user will
> surely lose the functionality to detect reused memory regions. But the
> extraneous soft-dirty pages would not appear. I'm trying to do this in the
> patch series [1]. Some discussion is going on that this fails with some
> mprotect use case [2]. I still need to have a look at the mprotect selftest
> to see how and why this fails. I think this can be implemented after some
> more work probably in mprotect side.
> 
> [1] https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/
> [2] https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/
> ---
>   mm/mmap.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f9b96b387a6f..6934b8f61fdc 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1708,6 +1708,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>   		vm_flags |= VM_ACCOUNT;
>   	}
>   
> +	/*
> +	 * New (or expanded) vma always get soft dirty status.
> +	 * Otherwise user-space soft-dirty page tracker won't
> +	 * be able to distinguish situation when vma area unmapped,
> +	 * then new mapped in-place (which must be aimed as
> +	 * a completely new data area).
> +	 */
> +	vm_flags |= VM_SOFTDIRTY;
> +
>   	/*
>   	 * Can we just expand an old mapping?
>   	 */
> @@ -1823,15 +1832,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>   	if (file)
>   		uprobe_mmap(vma);
>   
> -	/*
> -	 * New (or expanded) vma always get soft dirty status.
> -	 * Otherwise user-space soft-dirty page tracker won't
> -	 * be able to distinguish situation when vma area unmapped,
> -	 * then new mapped in-place (which must be aimed as
> -	 * a completely new data area).
> -	 */
> -	vma->vm_flags |= VM_SOFTDIRTY;
> -
>   	vma_set_page_prot(vma);

vma_set_page_prot(vma) has to be called after adjusting vma->vm_flags.

Did not look into the details here, but that jumped at me.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: set the vma flags dirty before testing if it is mergeable

[Muhammad Usama Anjum]

On 11/22/22 1:36 PM, David Hildenbrand wrote:
> On 22.11.22 09:24, Muhammad Usama Anjum wrote:
>> The VM_SOFTDIRTY should be set in the vma flags to be tested if new
>> allocation should be merged in previous vma or not. With this patch,
>> the new allocations are merged in the previous VMAs.
>>
>> I've tested it by reverting the commit 34228d473efe ("mm: ignore
>> VM_SOFTDIRTY on VMA merging") and after adding this following patch,
>> I'm seeing that all the new allocations done through mmap() are merged
>> in the previous VMAs. The number of VMAs doesn't increase drastically
>> which had contributed to the crash of gimp. If I run the same test after
>> reverting and not including this patch, the number of VMAs keep on
>> increasing with every mmap() syscall which proves this patch.
>>
>> The commit 34228d473efe ("mm: ignore VM_SOFTDIRTY on VMA merging")
>> seems like a workaround. But it lets the soft-dirty and non-soft-dirty
>> VMA to get merged. It helps in avoiding the creation of too many VMAs.
>> But it creates the problem while adding the feature of clearing the
>> soft-dirty status of only a part of the memory region.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: d9104d1ca966 ("mm: track vma changes with VM_SOFTDIRTY bit")
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>> We need more testing of this patch.
>>
>> While implementing clear soft-dirty bit for a range of address space, I'm
>> facing an issue. The non-soft dirty VMA gets merged sometimes with the soft
>> dirty VMA. Thus the non-soft dirty VMA become dirty which is undesirable.
>> When discussed with the some other developers they consider it the
>> regression. Why the non-soft dirty page should appear as soft dirty when it
>> isn't soft dirty in reality? I agree with them. Should we revert
>> 34228d473efe or find a workaround in the IOCTL?
>>
>> * Revert may cause the VMAs to expand in uncontrollable situation where the
>> soft dirty bit of a lot of memory regions or the whole address space is
>> being cleared again and again. AFAIK normal process must either be only
>> clearing a few memory regions. So the applications should be okay. There is
>> still chance of regressions if some applications are already using the
>> soft-dirty bit. I'm not sure how to test it.
>>
>> * Add a flag in the IOCTL to ignore the dirtiness of VMA. The user will
>> surely lose the functionality to detect reused memory regions. But the
>> extraneous soft-dirty pages would not appear. I'm trying to do this in the
>> patch series [1]. Some discussion is going on that this fails with some
>> mprotect use case [2]. I still need to have a look at the mprotect selftest
>> to see how and why this fails. I think this can be implemented after some
>> more work probably in mprotect side.
>>
>> [1]
>> https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/
>> [2]
>> https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/
>> ---
>>   mm/mmap.c | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index f9b96b387a6f..6934b8f61fdc 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1708,6 +1708,15 @@ unsigned long mmap_region(struct file *file,
>> unsigned long addr,
>>           vm_flags |= VM_ACCOUNT;
>>       }
>>   +    /*
>> +     * New (or expanded) vma always get soft dirty status.
>> +     * Otherwise user-space soft-dirty page tracker won't
>> +     * be able to distinguish situation when vma area unmapped,
>> +     * then new mapped in-place (which must be aimed as
>> +     * a completely new data area).
>> +     */
>> +    vm_flags |= VM_SOFTDIRTY;
>> +
>>       /*
>>        * Can we just expand an old mapping?
>>        */
>> @@ -1823,15 +1832,6 @@ unsigned long mmap_region(struct file *file,
>> unsigned long addr,
>>       if (file)
>>           uprobe_mmap(vma);
>>   -    /*
>> -     * New (or expanded) vma always get soft dirty status.
>> -     * Otherwise user-space soft-dirty page tracker won't
>> -     * be able to distinguish situation when vma area unmapped,
>> -     * then new mapped in-place (which must be aimed as
>> -     * a completely new data area).
>> -     */
>> -    vma->vm_flags |= VM_SOFTDIRTY;
>> -
>>       vma_set_page_prot(vma);
> 
> vma_set_page_prot(vma) has to be called after adjusting vma->vm_flags.
> 
> Did not look into the details here, but that jumped at me.
vma_set_page_prot() also needs to be removed from here as it was being
called after updating the vm_flags. I'll remove it. vma_set_page_prot() was
added in a separate commit 64e455079e1b. I'll send a v2 in a while.

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH] mm: set the vma flags dirty before testing if it is mergeable

[Muhammad Usama Anjum]

On 11/22/22 1:49 PM, Muhammad Usama Anjum wrote:
> On 11/22/22 1:36 PM, David Hildenbrand wrote:
>> On 22.11.22 09:24, Muhammad Usama Anjum wrote:
>>> The VM_SOFTDIRTY should be set in the vma flags to be tested if new
>>> allocation should be merged in previous vma or not. With this patch,
>>> the new allocations are merged in the previous VMAs.
>>>
>>> I've tested it by reverting the commit 34228d473efe ("mm: ignore
>>> VM_SOFTDIRTY on VMA merging") and after adding this following patch,
>>> I'm seeing that all the new allocations done through mmap() are merged
>>> in the previous VMAs. The number of VMAs doesn't increase drastically
>>> which had contributed to the crash of gimp. If I run the same test after
>>> reverting and not including this patch, the number of VMAs keep on
>>> increasing with every mmap() syscall which proves this patch.
>>>
>>> The commit 34228d473efe ("mm: ignore VM_SOFTDIRTY on VMA merging")
>>> seems like a workaround. But it lets the soft-dirty and non-soft-dirty
>>> VMA to get merged. It helps in avoiding the creation of too many VMAs.
>>> But it creates the problem while adding the feature of clearing the
>>> soft-dirty status of only a part of the memory region.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Fixes: d9104d1ca966 ("mm: track vma changes with VM_SOFTDIRTY bit")
>>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>>> ---
>>> We need more testing of this patch.
>>>
>>> While implementing clear soft-dirty bit for a range of address space, I'm
>>> facing an issue. The non-soft dirty VMA gets merged sometimes with the soft
>>> dirty VMA. Thus the non-soft dirty VMA become dirty which is undesirable.
>>> When discussed with the some other developers they consider it the
>>> regression. Why the non-soft dirty page should appear as soft dirty when it
>>> isn't soft dirty in reality? I agree with them. Should we revert
>>> 34228d473efe or find a workaround in the IOCTL?
>>>
>>> * Revert may cause the VMAs to expand in uncontrollable situation where the
>>> soft dirty bit of a lot of memory regions or the whole address space is
>>> being cleared again and again. AFAIK normal process must either be only
>>> clearing a few memory regions. So the applications should be okay. There is
>>> still chance of regressions if some applications are already using the
>>> soft-dirty bit. I'm not sure how to test it.
>>>
>>> * Add a flag in the IOCTL to ignore the dirtiness of VMA. The user will
>>> surely lose the functionality to detect reused memory regions. But the
>>> extraneous soft-dirty pages would not appear. I'm trying to do this in the
>>> patch series [1]. Some discussion is going on that this fails with some
>>> mprotect use case [2]. I still need to have a look at the mprotect selftest
>>> to see how and why this fails. I think this can be implemented after some
>>> more work probably in mprotect side.
>>>
>>> [1]
>>> https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/
>>> [2]
>>> https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/
>>> ---
>>>   mm/mmap.c | 21 +++++++++++----------
>>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index f9b96b387a6f..6934b8f61fdc 100644
>>> --- a/mm/mmap.c
>>> +++ b/mm/mmap.c
>>> @@ -1708,6 +1708,15 @@ unsigned long mmap_region(struct file *file,
>>> unsigned long addr,
>>>           vm_flags |= VM_ACCOUNT;
>>>       }
>>>   +    /*
>>> +     * New (or expanded) vma always get soft dirty status.
>>> +     * Otherwise user-space soft-dirty page tracker won't
>>> +     * be able to distinguish situation when vma area unmapped,
>>> +     * then new mapped in-place (which must be aimed as
>>> +     * a completely new data area).
>>> +     */
>>> +    vm_flags |= VM_SOFTDIRTY;
>>> +
>>>       /*
>>>        * Can we just expand an old mapping?
>>>        */
>>> @@ -1823,15 +1832,6 @@ unsigned long mmap_region(struct file *file,
>>> unsigned long addr,
>>>       if (file)
>>>           uprobe_mmap(vma);
>>>   -    /*
>>> -     * New (or expanded) vma always get soft dirty status.
>>> -     * Otherwise user-space soft-dirty page tracker won't
>>> -     * be able to distinguish situation when vma area unmapped,
>>> -     * then new mapped in-place (which must be aimed as
>>> -     * a completely new data area).
>>> -     */
>>> -    vma->vm_flags |= VM_SOFTDIRTY;
>>> -
>>>       vma_set_page_prot(vma);
>>
>> vma_set_page_prot(vma) has to be called after adjusting vma->vm_flags.
>>
>> Did not look into the details here, but that jumped at me.
> vma_set_page_prot() also needs to be removed from here as it was being
> called after updating the vm_flags. I'll remove it. vma_set_page_prot() was
> added in a separate commit 64e455079e1b. I'll send a v2 in a while.
Just had another look. vm_flags is being modified just above
vma_set_page_prot(). So we don't need to remove it.

-- 
BR,
Muhammad Usama Anjum