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

[Peter Xu]

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.

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.

-- 
Peter Xu