Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()

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

Hi Peter,

Thank you for replying.

On 12/20/22 9:03 PM, Peter Xu wrote:
> On Mon, Dec 19, 2022 at 05:19:12PM +0500, Muhammad Usama Anjum wrote:
>> On 11/22/22 2:17 AM, Peter Xu wrote:
>>> On Mon, Nov 21, 2022 at 07:57:05PM +0500, Muhammad Usama Anjum wrote:
>>>> Hi Peter,
>>>>
>>>> Thank you so much for replying.
>>>>
>>>> On 11/19/22 4:14 AM, Peter Xu wrote:
>>>>> On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote:
>>>>>> Hi Peter and David,
>>>>>
>>>>> Hi, Muhammad,
>>>>>
>>>>>>
>>>>>> On 7/25/22 7:20 PM, Peter Xu wrote:
>>>>>>> The check wanted to make sure when soft-dirty tracking is enabled we won't
>>>>>>> grant write bit by accident, as a page fault is needed for dirty tracking.
>>>>>>> The intention is correct but we didn't check it right because VM_SOFTDIRTY
>>>>>>> set actually means soft-dirty tracking disabled.  Fix it.
>>>>>> [...]
>>>>>>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
>>>>>>> +{
>>>>>>> +	/*
>>>>>>> +	 * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty
>>>>>>> +	 * enablements, because when without soft-dirty being compiled in,
>>>>>>> +	 * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY)
>>>>>>> +	 * will be constantly true.
>>>>>>> +	 */
>>>>>>> +	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
>>>>>>> +		return false;
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * Soft-dirty is kind of special: its tracking is enabled when the
>>>>>>> +	 * vma flags not set.
>>>>>>> +	 */
>>>>>>> +	return !(vma->vm_flags & VM_SOFTDIRTY);
>>>>>>> +}
>>>>>> I'm sorry. I'm unable to understand the inversion here.
>>>>>>> its tracking is enabled when the vma flags not set.
>>>>>> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is
>>>>>> soft-dirty. When we write to clear_refs to clear soft-dirty bit,
>>>>>> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking
>>>>>> is enabled when the vma flags not set?
>>>>>
>>>>> Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and
>>>>> only until then the real tracking starts (by removing write bits on ptes).
>>>> But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are
>>>> still marked soft-dirty. Both are independent.
>>>>
>>>> It means tracking is enabled all the time in individual pages.
>> Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE
>> bit status setting. The internal behavior has changed. The test case was
>> shared by David
>> (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/).
>> The explanation is as following:
>>
>> _Before_ addition of this patch(76aefad628aae),
>> m = mmap(2 pages)
>> clear_softdirty()
>> mremap(m + pag_size)
>> mprotect(READ)
>> mprotect(READ | WRITE);
>> memset(m)
>> After memset(),
>> 			PAGE-1		PAGE-2
>> VM_SOFTDIRTY		set		set
>> PTE softdirty flag	set		set
>> /proc//pagemap view	set		set
>>
>>
>> _After_ addition of this patch(76aefad628aae)
>> m = mmap(2 pages)
>> clear_softdirty()
>> mremap(m + page_size)
>> mprotect(READ)
>> mprotect(READ | WRITE);
>> memset(m)
>> After memset(),
>> 			PAGE-1		PAGE-2
>> VM_SOFTDIRTY		set		set
>> PTE softdirty flag	*not set*	set
>> /proc//pagemap view	set		set
>>
>> The user's point of view hasn't changed. But internally after this patch,
>> the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The
>> soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect
>> is used. Why? Because soft-dirty tracking in the PTEs is always enabled
>> regardless of VM_SOFTDIRTY is set or not. Example:
>>
>> m = mem(2 pages)
>> At this point:
>> 			PAGE-1		PAGE-2
>> VM_SOFTDIRTY		set		set
>> PTE softdirty flag	not set		not set
>> /proc//pagemap view	set		set
>> memset(m)
>> At this point:
>> 			PAGE-1		PAGE-2
>> VM_SOFTDIRTY		set		set
>> PTE softdirty flag	set		set
>> /proc//pagemap view	set		set
>>
>> This example proves that soft-dirty flag on the PTE is set regardless of
>> the VM_SOFTDIRTY.
> 
> IMHO this is not a proof good enough - it's a kernel internal detail, and
> the userspace cannot detect it, right?  Then it looks fine to not keep the
> same behavior on the ptes I think.  After all currently the soft-dirty is
> designed as "taking either VM_SOFTDIRTY of pte soft-dirty as input of being
> dirty".  Nothing violates that.
Nothing has changed for the userspace. But when the default soft-dirty
feature always updates the soft-dirty flag in the PTEs regardless of
VM_SOFTDIRTY is set or not, why does other components of the mm stop caring
for soft-dirty flag in the PTE when VM_SOFTDIRTY is set?

> 
> Your approach introduced PAGEMAP_NO_REUSED_REGIONS but that special
> information is not remembered in vma, IIUC that's why you find things
> messed up.  Fundamentally, it's because you're trying to reuse soft-dirty
> design but it's not completely soft-dirty anymore.
Correct, that's why I'm trying to find a way to correct the soft-dirty
support instead of using anything else. We should try and correct it. I've
sent a RFC to track the soft-dirty flags for sub regions in the VMA.

> 
> That's also why I mentioned the other async uffd-wp approach because with
> that there's no fiddling with vma flags (since it'll be always set as
> pre-requisite), and this specific problem shouldn't exist either because
> uffd-wp was originally designed to be pte-based as I mentioned, so we can't
> grant write if pte is not checked.
> 
> Your below change will resolve your problem for now, but it's definitely
> not wanted because it has a much broader impact on the whole system, for
> example, on vma_wants_writenotify().  We may still have some paths using
> default vm_page_prot (especially for file memories, not for the generic PF
> path but some others) that will start to lose write bits where we used to
> have them set.  That's bad for performance because resolving each of them
> needs one more page fault after the change as it mostly invalidated the
> write bit in vm_page_prot.
> 
> You can also introduce yet another flag in the vma so you can detect which
> vma has NEW soft-dirty enabled (your new approach) rather than the OLD
> (which still relies on vma flags besides ptes) but that'll really be ugly
> and making soft-dirty code unnecessarily complicated.
> 
>>
>> The simplest hack to get rid this changed behavior and revert to the
>> previous behaviour is as following:
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -860,6 +860,8 @@ static inline bool vma_soft_dirty_enabled(struct
>> vm_area_struct *vma)
>>         if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
>>                 return false;
>>
>> +       return true;
>> +
>>         /*
>>          * Soft-dirty is kind of special: its tracking is enabled when the
>>          * vma flags not set.
>> I was trying to verify this hack. But I couldn't previously until @Paul has
>> mentioned this again. I've verified with limited tests that this hack
>> in-deed works. We are unaware that does this hack create problems in other
>> areas or not. We can think of better way to solve this. Once we get the
>> comments from the community.
>>
>> This internal behavior change is affecting the new feature addition to the
>> soft-dirty flag which is already delicate and has issues.
>> (https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/)
>>
>>>
>>> IMHO it depends on how we define "tracking enabled" - before clear_refs
>>> even if no pages written they'll also be reported as dirty, then the
>>> information is useless.
>>>
>>>> Only the soft-dirty bit status in individual page isn't significant if
>>>> VM_SOFTDIRTY already is set. Right?
>>>
>>> Yes.  But I'd say it makes more sense to say "tracking enabled" if we
>>> really started tracking (by removing the write bits in ptes, otherwise we
>>> did nothing).  When vma created we didn't track anything.
>>>
>>> I don't know the rational of why soft-dirty was defined like that.  I think
>>> it's somehow related to the fact that we allow false positive dirty pages
>>> not false negative.  IOW, it's a bug to leak a page being dirtied, but not
>>> vice versa if we report clean page dirty.
>>>
>>
>> -- 
>> BR,
>> Muhammad Usama Anjum
>>
> 

-- 
BR,
Muhammad Usama Anjum