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/