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

[Peter Xu <peterx@redhat.com>]

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.

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.

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
> 

-- 
Peter Xu