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
