userfaultfd: mark uffd_wp regardless of VM_WRITE flag

From: Nadav Amit <[email protected]>

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)

At this point the user would expect to still get UFFD notification when the page is accessed for write, but the user would not get one, since the PTE was not marked as UFFD_WP during UFFDIO_COPY.

Fix it by always marking PTEs as UFFD_WP regardless on the write-permission in the VMA flags.

---
 mm/userfaultfd.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index 0780c2a57ff1..885e5adb0168 100644
@@ -72,12 +72,15 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
 	_dst_pte = pte_mkdirty(_dst_pte);
 	if (page_in_cache && !vm_shared)
 		writable = false;
-	if (writable) {
-		if (wp_copy)
-			_dst_pte = pte_mkuffd_wp(_dst_pte);
-		else
-			_dst_pte = pte_mkwrite(_dst_pte);
-	}
+
+	/*
+	 * Always mark a PTE as write-protected when needed, regardless of
+	 * VM_WRITE, which the user might change.
+	 */
+	if (wp_copy)
+		_dst_pte = pte_mkuffd_wp(_dst_pte);
+	else if (writable)
+		_dst_pte = pte_mkwrite(_dst_pte);
 
 	dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
 

2.25.1

One more note - if you want you can try the following reproducer:

#define _GNU_SOURCE
#include <linux/userfaultfd.h>
#include <errno.h>
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <pthread.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <stdio.h>

volatile int ok;

static void * fault_handler_thread(void *arg)

{
	struct uffd_msg msg = {0};
	int nread;
	int uffd = (int)(long)arg;
	struct uffdio_writeprotect uffd_wp = {0};

	nread = read(uffd, &msg, sizeof(msg));
	if (nread == 0) {
		printf("EOF on userfaultfd!\n");
		exit(EXIT_FAILURE);
	}

	ok = 1;

	uffd_wp.range.start = msg.arg.pagefault.address & ~(4095ull);
	uffd_wp.range.len = 4096;

	if (ioctl(uffd, UFFDIO_WRITEPROTECT, &uffd_wp) == -1) {
		perror("uffd-w-unprotect");
		exit(EXIT_FAILURE);
	}
	return NULL;
}

char page[4096];

int main(void)

{
	struct uffdio_api uffdio_api = {0};
	struct uffdio_register uffdio_register = {0};
	struct uffdio_writeprotect uffdio_wp = {0};
	struct uffdio_copy uffdio_copy = {0};
	volatile char *pc;
	pthread_t thr;
	int err;
	int uffd;
	void *p;

	uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
	if (uffd == -1) {
		perror("userfaultfd");
		exit(-1);
	}

	uffdio_api.api = UFFD_API;
	uffdio_api.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP;
	if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
		perror("api");
		exit(EXIT_FAILURE);
	}

	p = mmap(NULL, 4096, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

	if (p == MAP_FAILED) {
		perror("mmap");
		exit(EXIT_FAILURE);
	}

	uffdio_register.range.start = (unsigned long)p;
	uffdio_register.range.len = 4096;
	uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP;
	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
		perror("register");
		exit(EXIT_FAILURE);
	}

	uffdio_copy.src = (unsigned long)page;
	uffdio_copy.dst = (unsigned long)p;
	uffdio_copy.len = 4096;
	uffdio_copy.mode = UFFDIO_COPY_MODE_WP;
	uffdio_copy.copy = 0;
	if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == -1) {
		perror("uffd-copy");
		exit(EXIT_FAILURE);
	}

	err = pthread_create(&thr, NULL, fault_handler_thread, (void *)(long)uffd);
	if (err != 0) {
		exit(EXIT_FAILURE);
	}

	if (mprotect(p, 4096, PROT_READ|PROT_WRITE) < 0) {
		perror("mprotect(PROT_READ|PROT_WRITE)");
		exit(EXIT_FAILURE);
	}

	pc = (volatile char *)p;
	*pc = 1;
	printf("%s\n", ok ? "ok" : "failed”);
	return 0;
}

Hello, Nadav,

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);
        }

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.

Could I ask why you need mprotect() with uffd?

Thanks,

Peter Xu

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.

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.

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/

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 see whether I should prepare a patch and a test, maybe after yours.

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.

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

Peter Xu

I know it is off-topic (not directly related to my patch), but I tried to understand the logic - both of the existing code and of your suggested change - and I failed.

IIUC dirty_accountable (whose value is taken from vma_wants_writenotify()) means that the writes *should* be tracked, and therefore the page should remain read-only.

So basically the condition should have been based on !dirty_accountable, i.e. the inverted value of dirty_accountable.

The problem is that dirty_accountable also reflects VM_SOFTDIRTY considerations, so looking on the PTE does not tell you whether the PTE should remain write-protected even if it is dirty.

Am I missing something?

Just as I pressed send, I understood your suggestion.

It is still confusing for me how setting write-permissions for dirty_accountable PTEs is safe (as long as they are dirty), and yet in the general case it is not safe. I need to further think of it.

As I understand it, this patch (below) is still good to merge upstream, although Peter hasn't acked it (please).

And a whole bunch of followup patches are being thought about, but have yet to eventuate.

Do we feel that this patch warrants the cc:stable? I'm suspecting "no", as it isn't clear that the use-case is really legitimate at this time?

Thanks.

From: Nadav Amit <[email protected]>
Subject: userfaultfd: mark uffd_wp regardless of VM_WRITE flag

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)

At this point the user would expect to still get UFFD notification when the page is accessed for write, but the user would not get one, since the PTE was not marked as UFFD_WP during UFFDIO_COPY.

Fix it by always marking PTEs as UFFD_WP regardless on the write-permission in the VMA flags.

---

 mm/userfaultfd.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

@@ -72,12 +72,15 @@ int mfill_atomic_install_pte(struct mm_s
 	_dst_pte = pte_mkdirty(_dst_pte);
 	if (page_in_cache && !vm_shared)
 		writable = false;
-	if (writable) {
-		if (wp_copy)
-			_dst_pte = pte_mkuffd_wp(_dst_pte);
-		else
-			_dst_pte = pte_mkwrite(_dst_pte);
-	}
+
+	/*
+	 * Always mark a PTE as write-protected when needed, regardless of
+	 * VM_WRITE, which the user might change.
+	 */
+	if (wp_copy)
+		_dst_pte = pte_mkuffd_wp(_dst_pte);
+	else if (writable)
+		_dst_pte = pte_mkwrite(_dst_pte);
 
 	dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
 

_

Hi, Andrew,

Thanks for asking. I didn't ack because I saw that it's queued a long time ago into -mm, and also it's in -next for a long time too (my new uffd-wp patchset is rebased to this one already).

I normally assume that means you read and ack that patch already, so if I didn't see anything obviously wrong I'll just keep silent. Please let me know if that's not the expected behavior..

So here it is..

Is there a pointer/subject?

Right. Uffd-wp+mprotect usage is IMHO not a major use case for uffd-wp per my knowledge. At least I didn't even expect both work together, not until Nadav started working on some of the problems..

Said that, for this specific case it should be literally only changing the behavior of anonymous UFFD-WP && !WRITE case but nothing else (please correct me otherwise..), then IMHO it's pretty safe to copy stable too especially for the cleanly applicable branches.

Thanks,

Peter Xu

Acks and reviews are always welcome. If they come in late, git tree maintainer might not want to update and rebase, but it's still there in the archives for people who click on the Link:.

Thanks.

The messages in this thread. Several followup patches were discussed.

OK, thanks.

Right.

My understanding is that the dirty bits (especially if both set) means we've tracked dirty on this pte already so we don't need to, hence we can set the dirty bit here. E.g., continuous mprotect(RO), mprotect(RW) upon a full dirty pte.

When something wants to enable tracking again, it needs to clear the dirty bit, either the real one or soft-dirty one. So it's a pure performance enhancement to conditionally set write bit here, when we're sure we won't need any further tracking on this pte.

One thing to mention is that this path only applies to VM_SHARED|VM_WRITE, because that's what checked the first in vma_wants_writenotify():

	/* If it was private or non-writable, the write bit is already clear */
	if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED)))
		return 0;

IOW private mappings are not optimized in current tree yet.

Peter Collingbourne proposed a patch some time ago to optimize it but it didn't get merged somehow. Meanwhile even with his latest version it should still miss the thp case, so if to reference the private optimization Andrea's tree would be the best:

https://github.com/aagit/aa/commit/fadb5e04d94472614c76819acd979b2f60e4eff6

Hope it clarifies things a bit. Thanks,

Peter Xu

Thanks for the clarification. That’s what I suspected - I did not encounter it since I only used private anonymous mappings. I will try to create a test-case and send an additional fix for this issue.

Regards,
Nadav