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

[Nadav Amit <nadav.amit@gmail.com>]

> 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.

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.

> 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 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.

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