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

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

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