Re: [PATCH] userfaultfd: mark uffd_wp regardless of VM_WRITE flag

[Peter Xu <peterx@redhat.com>]

On Thu, Feb 17, 2022 at 06:23:14PM -0800, Nadav Amit wrote:
> 
> 
> > On Feb 17, 2022, at 5:58 PM, Peter Xu <peterx@redhat.com> wrote:
> > 
> > Hello, Nadav,
> > 
> > On Thu, Feb 17, 2022 at 09:16:02PM +0000, Nadav Amit wrote:
> >> From: Nadav Amit <namit@vmware.com>
> >> 
> >> When a PTE is set by UFFD operations such as UFFDIO_COPY, the PTE is
> >> currently only marked as write-protected if the VMA has VM_WRITE flag
> >> set. This seems incorrect or at least would be unexpected by the users.
> >> 
> >> Consider the following sequence of operations that are being performed
> >> on a certain page:
> >> 
> >> 	mprotect(PROT_READ)
> >> 	UFFDIO_COPY(UFFDIO_COPY_MODE_WP)
> >> 	mprotect(PROT_READ|PROT_WRITE)
> > 
> > No objection to the patch, however I'm wondering why this is a valid use
> > case because mprotect seems to be conflict with uffd, because AFAICT
> > mprotect(PROT_READ|PROT_WRITE) can already grant write bit.
> > 
> > In change_pte_range():
> > 
> >        if (dirty_accountable && pte_dirty(ptent) &&
> >                        (pte_soft_dirty(ptent) ||
> >                                !(vma->vm_flags & VM_SOFTDIRTY))) {
> >                ptent = pte_mkwrite(ptent);
> >        }
> 
> I think you are right, and an additional patch is needed to prevent
> mprotect() from making an entry writable if the PTE has _PAGE_UFFD_WP
> set and uffd_wp_resolve was not provided. I missed that.

Perhaps we can simply make this "if" to be "else" so as to connect with the
previous "if"?  After all the three (wp, wp_resolv, dirty_acct) are never
used with more than one flag set.

> 
> I’ll post another patch for this one.
> 
> > 
> > PS: I always think here the VM_SOFTDIRTY check is wrong, IMHO it should be:
> > 
> >        if (dirty_accountable && pte_dirty(ptent) &&
> >                        (pte_soft_dirty(ptent) ||
> >                        (vma->vm_flags & VM_SOFTDIRTY))) {
> >                ptent = pte_mkwrite(ptent);
> >        }
> > 
> > Because when VM_SOFTDIRTY is cleared it means soft dirty enabled.  I wanted
> > to post a patch but I never yet.
> 
> Seems that you are right. Yet, having this wrong code around for
> some time raises the concern whether something will break. By the
> soft-dirty I saw so far, it seems that it is not commonly used.

I'll see whether I should prepare a patch and a test, maybe after yours.

> 
> > Could I ask why you need mprotect() with uffd?
> 
> Sure. I think I mentioned it before, that I want to use userfaultfd
> for other processes [1], by having one monitor UFFD for multiple
> processes that handles their swap/prefetch activities based on custom
> policies.
> 
> I try to set the least amount of constraints on what these processes
> might do, and mprotect() is something they are allowed to do.

I see.  I didn't expect mprotect() can work well with uffd, but it seems
fine at least in this case.

Have you thought about other use of mprotect() other than RO?  Say, I only
know a valid use case of PROT_NONE for region reservation purpose, which
normally will be followed up by a munmap() and remap on the same address.
That sounds okay.  But not sure whether this patch will cover all the
possible mprotect() uses in the tracee.

> 
> I would hopefully send the patches that are required for all of that
> and open source my code soon. In the meanwhile I try to upstream the
> least controversial parts.

Sure, I'm always happy to learn it.  Thanks,

> 
> [1] https://lore.kernel.org/linux-mm/YWZCClDorCCM7KMG@t490s/t/

-- 
Peter Xu