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

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 21, 2022 at 09:28:47AM +0200, David Hildenbrand wrote:
> Modifying your test to map page from a file MAP_SHARED gives me under
> 5.18.11-100.fc35.x86_64:
> 
> 
> 53,54d52
> <       FILE *file = fopen("tmpfile", "w+");
> <       int file_fd;
> 56d53
> <       assert(file);
> 59,61d55
> <
> <       file_fd = fileno(file);
> <       ftruncate(file_fd, psize);
> 63c57
> <                   MAP_SHARED, file_fd, 0);
> ---
> >                   MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> 
> 
> t480s: ~  $ sudo ./tmp
> ERROR: Wrote page again, soft-dirty=0 (expect: 1
> 
> 
> 
> IMHO, while the check in vma_wants_writenotify() is correct and makes
> sure that the pages are kept R/O by the generic machinery. We get
> vma_wants_writenotify(), so we activate MM_CP_TRY_CHANGE_WRITABLE. The
> wrong logic in can_change_pte_writable(), however, maps the page
> writable again without caring about softdirty.
> 
> At least that would be my explanation for the failure. But maybe I
> messes up something else :)

Correct, I missed that part.  I verified that the same test also fails for
me easily on a xfs file test of an old kernel.

Let me touch up the commit message for that.   Though I think I'll still
keep the Fixes since the patch won't apply to before the commit, but I'll
mention that's only for tracking purpose.

[...]

> Can we turn that into a vm selftest in
> tools/testing/selftests/vm/soft-dirty.c, and also extend it by
> MAP_SHARED froma  file as above?

Sure.  I'll post a v3 with that.

[...]

> > @@ -48,8 +48,11 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,
> >  	if (pte_protnone(pte) || !pte_dirty(pte))
> >  		return false;
> >  
> > -	/* Do we need write faults for softdirty tracking? */
> > -	if ((vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
> > +	/*
> > +	 * Do we need write faults for softdirty tracking?  Note,
> > +	 * soft-dirty is enabled when !VM_SOFTDIRTY.
> > +	 */
> > +	if (!(vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
> >  		return false;
> 
> I wonder if we now want, just as in vma_wants_writenotify(), an early
> check for IS_ENABLED(CONFIG_MEM_SOFT_DIRTY), to optimize this out
> completely.

Hmm, it may not even be an optimization issue, since when
!CONFIG_MEM_SOFT_DIRTY we have VM_SOFTDIRTY defined as 0x0.

It means !(vma->vm_flags & VM_SOFTDIRTY) will be constantly true even if
soft dirty not compiled in.

I'll add that check too.  Thanks,

-- 
Peter Xu