[PATCH 0/1] mm: restore full accuracy in COW page reuse

[PATCH 0/1] mm: restore full accuracy in COW page reuse

[Andrea Arcangeli]

Hello Andrew and everyone,

Once we agree that COW page reuse requires full accuracy, the next
step is to re-apply 17839856fd588f4ab6b789f482ed3ffd7c403e1f and to
return going in that direction.

Who is going to orthogonally secure vmsplice, Andy, Jason, Jens?  Once
vmsplice is secured from taking unconstrained unprivileged long term
GUP pins, the attack from child to parent can still happen in theory,
but statistically speaking once that huge window is closed, it won't
be a practical concern, so it'll give us time to perfect the full
solution by closing all windows the VM core. vmsplice has to be
orthogonally fixed anyway, even if all windows were closed in VM core
first.

Unfortunately it's still not clear exactly what failed with
17839856fd588f4ab6b789f482ed3ffd7c403e1f but the whole point is that
we need to discuss that together.

Thanks,
Andrea

// SPDX-License-Identifier: GPL-3.0-or-later
/*
 *  reproducer for v5.11 O_DIRECT mm corruption with page_count
 *  instead of mapcount in do_wp_page.
 *
 *  Copyright (C) 2021  Red Hat, Inc.
 *
 *  gcc -O2 -o page_count_do_wp_page page_count_do_wp_page.c -lpthread
 *  page_count_do_wp_page ./whateverfile
 *
 *  NOTE: CONFIG_SOFT_DIRTY=y is required in the kernel config.
 */

#define _GNU_SOURCE
#include <stdbool.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/mman.h>

#define PAGE_SIZE (1UL<<12)
#define HARDBLKSIZE 512

static void* writer(void *_mem)
{
	char *mem = (char *)_mem;
	for(;;) {
		usleep(random() % 1000);
		mem[PAGE_SIZE-1] = 0;
	}
}

static void* background_soft_dirty(void *data)
{
	long fd = (long) data;
	for (;;)
		if (write(fd, "4", 1) != 1)
			perror("write soft dirty"), exit(1);
}

int main(int argc, char *argv[])
{
	if (argc < 2)
		printf("%s <filename>", argv[0]), exit(1);

	char path[PAGE_SIZE];
	strcpy(path, "/proc/");
	sprintf(path + strlen(path), "%d", getpid());
	strcat(path, "/clear_refs");
	long soft_dirty_fd = open(path, O_WRONLY);
	if (soft_dirty_fd < 0)
		perror("open clear_refs"), exit(1);

	char *mem;
	if (posix_memalign((void **)&mem, PAGE_SIZE, PAGE_SIZE*3))
		perror("posix_memalign"), exit(1);
	/* THP is not using page_count so it would not corrupt memory */
	if (madvise(mem, PAGE_SIZE, MADV_NOHUGEPAGE))
		perror("madvise"), exit(1);
	bzero(mem, PAGE_SIZE * 3);
	memset(mem + PAGE_SIZE * 2, 0xff, HARDBLKSIZE);

	/*
	 * This is not specific to O_DIRECT. Even if O_DIRECT was
	 * forced to use PAGE_SIZE minimum granularity for reads, a
	 * recvmsg would create the same issue since it also use
	 * iov_iter_get_pages internally to create transient GUP pins
	 * on anon memory.
	 */
	int fd = open(argv[1], O_DIRECT|O_CREAT|O_RDWR|O_TRUNC, 0600);
	if (fd < 0)
		perror("open"), exit(1);
	if (write(fd, mem, PAGE_SIZE) != PAGE_SIZE)
		perror("write"), exit(1);

	pthread_t soft_dirty;
	if (pthread_create(&soft_dirty, NULL,
			   background_soft_dirty, (void *)soft_dirty_fd))
		perror("soft_dirty"), exit(1);

	pthread_t thread;
	if (pthread_create(&thread, NULL, writer, mem))
		perror("pthread_create"), exit(1);

	bool skip_memset = true;
	while (1) {
		if (pread(fd, mem, HARDBLKSIZE, 0) != HARDBLKSIZE)
			perror("read"), exit(1);
		if (memcmp(mem, mem+PAGE_SIZE, HARDBLKSIZE)) {
			if (memcmp(mem, mem+PAGE_SIZE*2, PAGE_SIZE)) {
				if (skip_memset)
					printf("unexpected memory "
					       "corruption detected\n");
				else
					printf("memory corruption detected, "
					       "dumping page\n");
				int end = PAGE_SIZE;
				if (!memcmp(mem+HARDBLKSIZE, mem+PAGE_SIZE,
					    PAGE_SIZE-HARDBLKSIZE))
					end = HARDBLKSIZE;
				for (int i = 0; i < end; i++)
					printf("%x", mem[i]);
				printf("\n");
			} else
				printf("memory corruption detected\n");
		}
		skip_memset = !skip_memset;
		if (!skip_memset)
			memset(mem, 0xff, HARDBLKSIZE);
	}

	return 0;
}

Andrea Arcangeli (1):
  mm: restore full accuracy in COW page reuse

 include/linux/ksm.h |  7 ++++++
 mm/ksm.c            | 25 +++++++++++++++++++
 mm/memory.c         | 59 ++++++++++++++++++++++++++++++++-------------
 3 files changed, 74 insertions(+), 17 deletions(-)

[PATCH 1/1] mm: restore full accuracy in COW page reuse

[Andrea Arcangeli]

This reverts commit 09854ba94c6aad7886996bfbee2530b3d8a7f4f4.
This reverts commit be068f29034fb00530a053d18b8cf140c32b12b3.
This reverts commit 1a0cf26323c80e2f1c58fc04f15686de61bfab0c.

This example below uses O_DIRECT, on a process under
clear_refs. There's no FOLL_LONGTERM involvement. All GUP pins are
transient. fork is never called and even when clear_refs is cleared by
an external program, fork() would not be involved.

thread0				thread1			other process
							(or thread 3)
=============			=============		===========
read syscall
O_DIRECT
read DMA to vaddr+0
len = 512 bytes
GUP(FOLL_WRITE)
DMA writing to RAM started
							clear_refs
							pte_wrprotect
				write vaddrA+512
				page_count == 2
				wp_copy_page
read syscall returns
read lost

Notwithstanding the fact that failing O_DIRECT at sub-PAGE_SIZE
granularity is an ABI break by itself, this is of course not just
about O_DIRECT. recvmsg kernel TLS and plenty of other GUP FOLL_WRITE
iov_iter_get_pages users would write to the memory with sub-PAGE_SIZE
granularity, depending on the msghdr iov tiny buffers. Those recvmsg
are already silently lost too as well as the above O_DIRECT already
get lost.

The fact COW must not happen too much is documented in commit
6d0a07edd17cfc12fdc1f36de8072fa17cc3666f:

==
This will provide fully accuracy to the mapcount calculation in the
write protect faults, so page pinning will not get broken by false
positive copy-on-writes.
==

And in the current comment above the THP mapcount:

==
 * [..] we only use
 * page_trans_huge_mapcount() in the copy-on-write faults where we
 * need full accuracy to avoid breaking page pinning, [..]
==

Quote from the Link tagged:

==
In other words, the page_count check in do_wp_page, extends the old
fork vs gup vs threads race, so that it also happens within a single
thread without fork() involvement.
===

In addition FOLL_LONGTERM breaks for readonly DMA, despite
FOLL_LONGTERM pages are now treated specially and copied in fork().

FOLL_LONGTERM is in no way special with regard to the VM core and it
can't be. From the VM standpoint all GUP in are equal and breaking
one, means breaking them all the same.

It is not possible to resolve this by looking only at FOLL_LONGTERM,
that only hides the problem, but it doesn't fully resolve it as shown
above.

In addition this avoids storms of extra false positive COWs if the THP
are shared by different processes, which causes extra overhead, but
the several performance considerations are only a secondary concern.

After this commit, the copy in fork() for FOLL_LONGTERM also must be
extended to all GUP pins including transient pins or we shouldn't even
copy FOLL_LONGTERM pinned pages. Treating FOLL_LONGTERM any different
from transient GUP pin is sign that something is fundamentally flawed.

FOLL_LONGTERM can be treated different in writeback and during GUP
runtime itself, but not in the VM-core when deciding when to COW or
not to COW an anonymous page.

This revert causes no theoretical security regression because THP is
not yet using page_count in do_huge_pmd_wp_page.

The alternative of this patch would be to replace the mapcount with
the page_count in do_huge_pmd_wp_page too in order to really close the
vmsplice attack from child to parent that way.

However since a single transient GUP pin on a tail page, would elevate
the page_count for all other tail pages (unlike the mapcount which is
subpage granular), the above "fork-less" race would span over a
HPAGE_PMD_SIZE range, it would even cross different vmas and the
effect would happen at a distance in vma of different processes
allowing a child to corrupt memory in the parent. That's a problem
that could happen not-maliciously too. So the scenario described
above, if extended to THP new refcounting, would be more concerning
than the vmsplice issue, that can be tackled first in vmsplice itself,
and only at a second stage in the COW code.

Link: https://lkml.kernel.org/r/20210107200402.31095-1-aarcange@redhat.com
Cc: stable@kernel.org
Fixes: 09854ba94c6a ("mm: do_wp_page() simplification")
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 include/linux/ksm.h |  7 ++++++
 mm/ksm.c            | 25 +++++++++++++++++++
 mm/memory.c         | 59 ++++++++++++++++++++++++++++++++-------------
 3 files changed, 74 insertions(+), 17 deletions(-)

diff --git a/include/linux/ksm.h b/include/linux/ksm.h
index 161e8164abcf..e48b1e453ff5 100644
--- a/include/linux/ksm.h
+++ b/include/linux/ksm.h
@@ -53,6 +53,8 @@ struct page *ksm_might_need_to_copy(struct page *page,
 
 void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc);
 void ksm_migrate_page(struct page *newpage, struct page *oldpage);
+bool reuse_ksm_page(struct page *page,
+			struct vm_area_struct *vma, unsigned long address);
 
 #else  /* !CONFIG_KSM */
 
@@ -86,6 +88,11 @@ static inline void rmap_walk_ksm(struct page *page,
 static inline void ksm_migrate_page(struct page *newpage, struct page *oldpage)
 {
 }
+static inline bool reuse_ksm_page(struct page *page,
+			struct vm_area_struct *vma, unsigned long address)
+{
+	return false;
+}
 #endif /* CONFIG_MMU */
 #endif /* !CONFIG_KSM */
 
diff --git a/mm/ksm.c b/mm/ksm.c
index 9694ee2c71de..9503e11ab5b4 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2665,6 +2665,31 @@ void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
 		goto again;
 }
 
+bool reuse_ksm_page(struct page *page,
+		    struct vm_area_struct *vma,
+		    unsigned long address)
+{
+#ifdef CONFIG_DEBUG_VM
+	if (WARN_ON(is_zero_pfn(page_to_pfn(page))) ||
+			WARN_ON(!page_mapped(page)) ||
+			WARN_ON(!PageLocked(page))) {
+		dump_page(page, "reuse_ksm_page");
+		return false;
+	}
+#endif
+
+	if (PageSwapCache(page) || !page_stable_node(page))
+		return false;
+	/* Prohibit parallel get_ksm_page() */
+	if (!page_ref_freeze(page, 1))
+		return false;
+
+	page_move_anon_rmap(page, vma);
+	page->index = linear_page_index(vma, address);
+	page_ref_unfreeze(page, 1);
+
+	return true;
+}
 #ifdef CONFIG_MIGRATION
 void ksm_migrate_page(struct page *newpage, struct page *oldpage)
 {
diff --git a/mm/memory.c b/mm/memory.c
index feff48e1465a..2587d6b5b0b2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3114,25 +3114,50 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 	 * not dirty accountable.
 	 */
 	if (PageAnon(vmf->page)) {
-		struct page *page = vmf->page;
-
-		/* PageKsm() doesn't necessarily raise the page refcount */
-		if (PageKsm(page) || page_count(page) != 1)
-			goto copy;
-		if (!trylock_page(page))
-			goto copy;
-		if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) {
-			unlock_page(page);
+		int total_map_swapcount;
+		if (PageKsm(vmf->page) && (PageSwapCache(vmf->page) ||
+					   page_count(vmf->page) != 1))
 			goto copy;
+		if (!trylock_page(vmf->page)) {
+			get_page(vmf->page);
+			pte_unmap_unlock(vmf->pte, vmf->ptl);
+			lock_page(vmf->page);
+			vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
+					vmf->address, &vmf->ptl);
+			if (!pte_same(*vmf->pte, vmf->orig_pte)) {
+				update_mmu_tlb(vma, vmf->address, vmf->pte);
+				unlock_page(vmf->page);
+				pte_unmap_unlock(vmf->pte, vmf->ptl);
+				put_page(vmf->page);
+				return 0;
+			}
+			put_page(vmf->page);
 		}
-		/*
-		 * Ok, we've got the only map reference, and the only
-		 * page count reference, and the page is locked,
-		 * it's dark out, and we're wearing sunglasses. Hit it.
-		 */
-		unlock_page(page);
-		wp_page_reuse(vmf);
-		return VM_FAULT_WRITE;
+		if (PageKsm(vmf->page)) {
+			bool reused = reuse_ksm_page(vmf->page, vmf->vma,
+						     vmf->address);
+			unlock_page(vmf->page);
+			if (!reused)
+				goto copy;
+			wp_page_reuse(vmf);
+			return VM_FAULT_WRITE;
+		}
+		if (reuse_swap_page(vmf->page, &total_map_swapcount)) {
+			if (total_map_swapcount == 1) {
+				/*
+				 * The page is all ours. Move it to
+				 * our anon_vma so the rmap code will
+				 * not search our parent or siblings.
+				 * Protected against the rmap code by
+				 * the page lock.
+				 */
+				page_move_anon_rmap(vmf->page, vma);
+			}
+			unlock_page(vmf->page);
+			wp_page_reuse(vmf);
+			return VM_FAULT_WRITE;
+		}
+		unlock_page(vmf->page);
 	} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
 					(VM_WRITE|VM_SHARED))) {
 		return wp_page_shared(vmf);

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Sat, Jan 9, 2021 at 4:44 PM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> Once we agree that COW page reuse requires full accuracy, [...]

You have completely and utterly ignored every single argument against that.

Instead, you just continue to push your agenda.

The thing is, GUP works fine.

COW works fine.

The thing that is broken is clear_refs.

Yet you try to now "fix" the two fine cases, because you don't want to
fix clear_refs.

What part of "clear_refs is the _least_ important of the three cases"
are you not willing to understand?

                 Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]

On Sat, Jan 9, 2021 at 4:55 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> What part of "clear_refs is the _least_ important of the three cases"
> are you not willing to understand?

In fact, I couldn't even turn on that code with my normal config,
because it depends on CONFIG_CHECKPOINT_RESTORE that I didn't even
have enabled.

IOW, that code is some special-case stuff, and instead of messing up
the rest of the VM, it should be made to conform to all the normal VM
rules and requirements.

Here's two patches to basically start doing that.

The first one is the same one I already sent out earlier, fixing the
locking. And yes, it can be improved upon, but before improving on it,
let's _fix_ the code.

The second is a trivial "oh, look, I can see that the page is pinned,
soft-dirty cannot work so don't do it then". Again, it can be improved
upon, most particularly by doing the same (simple) tests for the
hugepage case too, which I didn't do.

Note: I have not a single actual user of this code that I can test
with, so this is all ENTIRELY untested.

IOW, I am in no way claiming that these patches are perfect and
correct, and the only way to do things.

But what I _am_ claiming is that this clear_refs code (and the UFFD
code) is of secondary importance, and instead of messing up the core
VM, we should fix these special cases to not do bad things.

It really is that simple.

And no, I didn't make the UFFDIO_WRITEPROTECT code take the mmap_sem
for writing. For whoever wants to look at that, it's
mwriteprotect_range() in mm/userfaultfd.c and the fix is literally to
turn the read-lock (and unlock) into a write-lock (and unlock).

           Linus

[-- Attachment #2: 0001-mm-fix-clear_refs_write-locking.patch --]
[-- Type: text/x-patch, Size: 2315 bytes --]

From dacb5de62b654f1f5df1147e263b5b4e5fe2af44 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 8 Jan 2021 13:13:41 -0800
Subject: [PATCH 1/2] mm: fix clear_refs_write locking

Turning page table entries read-only requires the mmap_sem held for
writing.

So stop doing the odd games with turning things from read locks to write
locks and back.  Just get the write lock.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/proc/task_mmu.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ee5a235b3056..ab7d700b2caa 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1215,41 +1215,26 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 			.type = type,
 		};
 
+		if (mmap_write_lock_killable(mm)) {
+			count = -EINTR;
+			goto out_mm;
+		}
 		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
-			if (mmap_write_lock_killable(mm)) {
-				count = -EINTR;
-				goto out_mm;
-			}
-
 			/*
 			 * Writing 5 to /proc/pid/clear_refs resets the peak
 			 * resident set size to this mm's current rss value.
 			 */
 			reset_mm_hiwater_rss(mm);
-			mmap_write_unlock(mm);
-			goto out_mm;
+			goto out_unlock;
 		}
 
-		if (mmap_read_lock_killable(mm)) {
-			count = -EINTR;
-			goto out_mm;
-		}
 		tlb_gather_mmu(&tlb, mm, 0, -1);
 		if (type == CLEAR_REFS_SOFT_DIRTY) {
 			for (vma = mm->mmap; vma; vma = vma->vm_next) {
 				if (!(vma->vm_flags & VM_SOFTDIRTY))
 					continue;
-				mmap_read_unlock(mm);
-				if (mmap_write_lock_killable(mm)) {
-					count = -EINTR;
-					goto out_mm;
-				}
-				for (vma = mm->mmap; vma; vma = vma->vm_next) {
-					vma->vm_flags &= ~VM_SOFTDIRTY;
-					vma_set_page_prot(vma);
-				}
-				mmap_write_downgrade(mm);
-				break;
+				vma->vm_flags &= ~VM_SOFTDIRTY;
+				vma_set_page_prot(vma);
 			}
 
 			mmu_notifier_range_init(&range, MMU_NOTIFY_SOFT_DIRTY,
@@ -1261,7 +1246,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		if (type == CLEAR_REFS_SOFT_DIRTY)
 			mmu_notifier_invalidate_range_end(&range);
 		tlb_finish_mmu(&tlb, 0, -1);
-		mmap_read_unlock(mm);
+out_unlock:
+		mmap_write_unlock(mm);
 out_mm:
 		mmput(mm);
 	}
-- 
2.29.2.157.g1d47791a39


[-- Attachment #3: 0002-mm-don-t-play-games-with-pinned-pages-in-clear_page_.patch --]
[-- Type: text/x-patch, Size: 1818 bytes --]

From b40950b647509f7222e1f7174d61045d15f56f1c Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 9 Jan 2021 17:09:10 -0800
Subject: [PATCH 2/2] mm: don't play games with pinned pages in clear_page_refs

Turnign a pinned page read-only breaks the pinning after COW. Don't do it.

The whole "track page soft dirty" state doesn't work with pinned pages
anyway, since the page might be dirtied by the pinning entity without
ever being noticed in the page tables.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/proc/task_mmu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ab7d700b2caa..0377081021b7 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1035,6 +1035,25 @@ struct clear_refs_private {
 };
 
 #ifdef CONFIG_MEM_SOFT_DIRTY
+
+#define is_cow_mapping(flags) (((flags) & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE)
+
+static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
+{
+	struct page *page;
+
+	if (!is_cow_mapping(vma->vm_flags))
+		return false;
+	if (likely(!atomic_read(&vma->vm_mm->has_pinned)))
+		return false;
+	page = vm_normal_page(vma, addr, pte);
+	if (!page)
+		return false;
+	if (page_mapcount(page) != 1)
+		return false;
+	return page_maybe_dma_pinned(page);
+}
+
 static inline void clear_soft_dirty(struct vm_area_struct *vma,
 		unsigned long addr, pte_t *pte)
 {
@@ -1049,6 +1068,8 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
 	if (pte_present(ptent)) {
 		pte_t old_pte;
 
+		if (pte_is_pinned(vma, addr, ptent))
+			return;
 		old_pte = ptep_modify_prot_start(vma, addr, pte);
 		ptent = pte_wrprotect(old_pte);
 		ptent = pte_clear_soft_dirty(ptent);
-- 
2.29.2.157.g1d47791a39

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Sat, Jan 9, 2021 at 5:19 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And no, I didn't make the UFFDIO_WRITEPROTECT code take the mmap_sem
> for writing. For whoever wants to look at that, it's
> mwriteprotect_range() in mm/userfaultfd.c and the fix is literally to
> turn the read-lock (and unlock) into a write-lock (and unlock).

Oh, and if it wasn't obvious, we'll have to debate what to do with
trying to mprotect() a pinned page. Do we just ignore the pinned page
(the way my clear_refs patch did)? Or do we turn it into -EBUSY? Or
what?

So it's not *just* the locking that needs to be fixed. But just take a
look at that suggested clear_refs patch of mine - it sure isn't
complicated.

              Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Andrea Arcangeli]

Hello Linus,

On Sat, Jan 09, 2021 at 05:19:51PM -0800, Linus Torvalds wrote:
> +#define is_cow_mapping(flags) (((flags) & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE)
> +
> +static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +{
> +	struct page *page;
> +
> +	if (!is_cow_mapping(vma->vm_flags))
> +		return false;
> +	if (likely(!atomic_read(&vma->vm_mm->has_pinned)))
> +		return false;
> +	page = vm_normal_page(vma, addr, pte);
> +	if (!page)
> +		return false;
> +	if (page_mapcount(page) != 1)
> +		return false;
> +	return page_maybe_dma_pinned(page);
> +}

I just don't see the simplification coming from
09854ba94c6aad7886996bfbee2530b3d8a7f4f4. Instead of checking
page_mapcount above as an optimization, to me it looks much simpler to
check it in a single place, in do_wp_page, that in addition of
optimizing away the superfluous copy, would optimize away the above
complexity as well.

And I won't comment if it's actually safe to skip random pages or
not. All I know is for mprotect and uffd-wp, definitely the above
approach wouldn't work.

In addition I dislike has_pinned and FOLL_PIN.

has_pinned        450 include/linux/mm_types.h 		 * for instance during page table copying for fork().
has_pinned       1021 kernel/fork.c    	atomic64_set(&mm->pinned_vm, 0);
has_pinned       1239 mm/gup.c         		atomic_set(&mm->has_pinned, 1);
has_pinned       2579 mm/gup.c         		atomic_set(&current->mm->has_pinned, 1);
has_pinned       1099 mm/huge_memory.c 		     atomic_read(&src_mm->has_pinned) &&
has_pinned       1213 mm/huge_memory.c 		     atomic_read(&src_mm->has_pinned) &&
has_pinned        819 mm/memory.c      	if (likely(!atomic_read(&src_mm->has_pinned)))

Are we spending 32bit in mm_struct atomic_t just to call atomic_set(1)
on it? Why isn't it a MMF_HAS_PINNED that already can be set
atomically under mmap_read_lock too? There's bit left free there, we
didn't run out yet to justify wasting another 31 bits. I hope I'm
overlooking something.

The existence of FOLL_LONGTERM is good and makes a difference at times
for writeback if it's on a MAP_SHARED, or it makes difference during
GUP to do a page_migrate before taking the pin, but for the whole rest
of the VM it's irrelevant if it's long or short term, so I'm also
concerned from what Jason mentioned about long term pins being treated
differently within the VM. That to me looks fundamentally as flawed as
introducing inaccuracies in do_wp_page, that even ignoring the
performance implications caused by the inaccuracy, simply prevent to
do some useful things.

I obviously agree all common workloads with no GUP pins and no
clear_refs and no uffd, are way more important to be optimal, but I
haven't seen a single benchmark showing the benefit of not taking the
PG_lock before a page copy or any other runtime benefit coming from
page_count in do_wp_page.

To the contrary now I see additional branches in fork and in various
other paths.

The only thing again where I see page_count provides a tangible
benefit, is the vmsplice attack from child to parent, but that has not
been fully fixed and if page_count is added to fix it in all COW
faults, it'll introduce extra inefficiency to the the very common
important workloads, not only to the special GUP/clear_refs/uffd-wp
workloads as your patch above shows.

Thanks,
Andrea

Re: [PATCH 1/1] mm: restore full accuracy in COW page reuse

[Andrea Arcangeli]

Hello,

On Sat, Jan 09, 2021 at 07:44:35PM -0500, Andrea Arcangeli wrote:
> allowing a child to corrupt memory in the parent. That's a problem
> that could happen not-maliciously too. So the scenario described

I updated the above partly quoted sentence since in the previous
version it didn't have full accuracy:

https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/commit/?id=fc5a76b1c14e5e6cdc64ece306fc03773662d98a

"However since a single transient GUP pin on a tail page, would elevate
the page_count for all other tail pages (unlike the mapcount which is
subpage granular), the COW page reuse inaccuracy would then cross
different vmas and the effect would happen at a distance in vma of
different processes. A single GUP pin taken on a subpage mapped in a
different process could trigger 511 false positive COWs copies in the
local process, after a fork()."

This a best effort to try to document all side effects, but it'd be
great to hear from Kirill too on the above detail to have
confirmation.

Thanks and have a great weekend everyone,
Andrea

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Andrea Arcangeli]

On Sat, Jan 09, 2021 at 05:37:09PM -0800, Linus Torvalds wrote:
> On Sat, Jan 9, 2021 at 5:19 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > And no, I didn't make the UFFDIO_WRITEPROTECT code take the mmap_sem
> > for writing. For whoever wants to look at that, it's
> > mwriteprotect_range() in mm/userfaultfd.c and the fix is literally to
> > turn the read-lock (and unlock) into a write-lock (and unlock).
> 
> Oh, and if it wasn't obvious, we'll have to debate what to do with
> trying to mprotect() a pinned page. Do we just ignore the pinned page
> (the way my clear_refs patch did)? Or do we turn it into -EBUSY? Or
> what?

Agreed, I assume mprotect would have the same effect.

mprotect in parallel of a read or recvmgs may be undefined, so I
didn't bring it up, but it was pretty clear.

The moment the write bit is cleared (no matter why and from who) and
the PG lock relased, if there's any GUP pin, GUP currently loses
synchrony.

In any case I intended to help exercising the new page_count logic
with the testcase, possibly to make it behave better somehow, no
matter how.

I admit I'm also wondering myself the exact semantics of O_DIRECT on
clear_refs or uffd-wp tracking, but the point is that losing reads and
getting unexpected data in the page, still doesn't look a good
behavior and it had to be at least checked.

To me ultimately the useful use case that is become impossible with
page_count isn't even clear_refs nor uffd-wp.

The useful case that I can see zero fundamental flaws in it, is a RDMA
or some other device computing in pure readonly DMA on the data while
a program runs normally and produces it. It could be even a
framebuffer that doesn't care about coherency. You may want to
occasionally wrprotect the memory under readonly long term GUP pin for
consistency even against bugs of the program itself. Why should
wrprotecting make the device lose synchrony? And kind of performance
we gain to the normal useful cases by breaking the special case? Is
there a benchmark showing it?

> So it's not *just* the locking that needs to be fixed. But just take a
> look at that suggested clear_refs patch of mine - it sure isn't
> complicated.

If we can skip the wrprotection it's fairly easy, I fully agree, even
then it still looks more difficult than using page_mapcount in
do_wp_page in my view, so I also don't see the simplification. And
overall the amount of kernel code had a net increase as result.

Thanks,
Andrea

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Sat, Jan 9, 2021 at 6:51 PM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> I just don't see the simplification coming from
> 09854ba94c6aad7886996bfbee2530b3d8a7f4f4. Instead of checking
> page_mapcount above as an optimization, to me it looks much simpler to
> check it in a single place, in do_wp_page, that in addition of
> optimizing away the superfluous copy, would optimize away the above
> complexity as well.

Here's the difference:

 (a) in COW, "page_mapcount()" is pure and utter garbage, and has zero meaning.

Why? Because MAPCOUNT DOES NOT MATTER FOR COW.

COW is about "I'm about to write to this page, and that means I need
an _exclusive_ page so that I don't write to a page that somebody else
is using".

Can you admit that fundamental fact?

Notice how "page_mapcount()" has absolutely NOTHING to do with
"exclusive page". There are lots of other ways the page can be used
aside from mapcount. The page cache may have a reference to the page.
Somebody that did a GUP may have a reference to the page.

So what actually matters at COW time? The only thing that matters is
"am I the exclusive owner". And guess what? We have a count of page
references. It's "page_count()". That's *EXACTLY* the thing that says
"are there maybe other references to this page".

In other words, COW needs to use page_count(). It really is that easy.
End of story.

So, given that, why do I then do

> +     if (page_mapcount(page) != 1)
> +             return false;

in my patch, when I just told you that "page_mapcount()" is irrelevant for COW?

Guess what? The above isn't about COW. The above isn't about whether
we need to do a copy in order to be able to write to the page without
anybody else being affected by it.

No, at fork time, and at this clear_refs time, the question is
entirely different. The question is not "Do I have exclusive access to
the page", but it is "Did I _already_ made sure that I have exclusive
access to the page because I pinned it"?

See how different the question is?

Because *if* you have done a pinned COW for soem direct-IO read, and
*if* that page is dirty, then you know it's mapped only in your
address space. You're basically doing the _reverse_ of the COW test,
and asking yourself "is this my own private pinned page"?

And then it's actually perfectly sane to do a check that says
"obviously, if somebody else has this page mapped, then that's not the
case".

See?

For COW, "page_mapcount()" is pure and utter garbage, and entirely
meaningless. How many places it's mapped in doesn't matter. You may
have to COW even if it's only mapped in your address space (page
cache, GUP, whatever).

But for "did I already make this exclusive", then it's actually
meaningful to say "is it mapped somewhere else". We know it has other
users - that "page_may_be_pinned()" in fact *guarantees* that it has
other users. But we're double-checking that the other users aren't
other mappings.

That said, I did just realize that that "page_mapcount()" check is
actually pointless.

Because we do have a simpler one. Instead of checking whether all
those references that made us go "page_might_be_pinned()" aren't other
mappings, the simple check for "pte_writable()" would already have
told us that we had already done the COW.

So you are actually right that the page_mapcount() test in my patch is
not the best way to check for this. By the time we see
"page_may_be_pinned()", we might as well just say "Oh, it's a private
mapping and the pte is already writable, so we know we were the
exclusive mapper, because COW and fork() already guarantee that".

> And I won't comment if it's actually safe to skip random pages or
> not. All I know is for mprotect and uffd-wp, definitely the above
> approach wouldn't work.

Why do you say that?

You say ":definitely know", but I think you're full of it.

The fact is, if you have a pinned page, why wouldn't we say "you can't
turn it read-only"?

It's pinned in the VM address space - and it's pinned writable. Simple
and clear semantics. You can *remove* it, but you can't change the
pinning.

              Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Sat, Jan 9, 2021 at 7:51 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> COW is about "I'm about to write to this page, and that means I need
> an _exclusive_ page so that I don't write to a page that somebody else
> is using".

So this kind of fundamentally explains why I hate the games we used to
play wrt page_mapcount(): they were fundamentally fragile. I _much_
prefer just having the rule that we use page_count(), which the above
simple and straightforward single sentence explains 100%.

This gets back to the fact that especially considering how we've had
subtle bugs here (the "wrong-way COW" issue has existed since
literally the first GUP ever, so it goes back decades), I want the
core VM rules to be things that can be explained basically from simple
"first principles".

And the reason I argue for the current direction that I'm pushing, is
exactly that the above is a very simple "first principle" for why COW
exists.

If the rule for COW is simply "I will always COW if it's not clear
that I'm the exclusive user", then COW itself is very simple to think
about.

The other rule I want to stress is that COW is common, and that argues
against the model we used to have of "let's lock the page to make sure
that everything else is stable". That model was garbage anyway, since
page locking doesn't even guarantee any stability wrt exclusive use in
the first place (ie GUP being another example), but it's why I truly
detested the old model that depended so much on the page lock to
serialize things.

So if you start off with the rule that "I will always COW unless I can
trivially see I'm the only owner", then I think we have really made
for a really clear and unambiguous rule.

And remember: COW is only an issue for private mappings. So pretty
much BY DEFINITION, doing a COW is always safe for all normal
circumstances.

Now, this is where it does get subtle: that "all normal circumstances"
part. The one special case is a cache-coherent GUP. It's arguable
whether "pinned" should matter or not, and it would obviously be
better if "pinned" simply didn't matter at all (and the only issue
with any long-term pinning would simply be about resource counting).

The current approach I'm advocating is "coherency means that it must
have been writable", and then the way to solve the whole "Oh, it's
shared with something else" is to simply never accept making it
read-only, because BY DEFINITION it's not _really_ read-only (because
we know we've created that other alias of the virtual address that is
*not* controlled by the page table protection bits).

Notice how this is all both conceptually fairly simple (ie I can
explain the rules in plain English without really making any complex
argument) and it is arguably internally fairly self-consistent (ie the
whole notion of "oh, there's another thing that has write access that
page but doesn't go through the page table, so trying to make it
read-only in the page tables is a nonsensical operation").

Are the end results wrt something like soft-dirty a bit odd? Not
really. If you do soft-dirty, such a GUP-shared page would simply
always show up as dirty. That's still consistent with the rules. If
somebody else may be writing to it because of GUP, that page really
*isn't* clean, and us marking it read-only would be just lying about
things.

I'm admittedly not very happy about mprotect() itself, though. It's
actually ok to do the mprotect(PROT_READ) and turn the page read-only:
that will also disable COW itself (because a page fault will now be a
SIGSEGV, not a COW).

But if you then make it writable again with mprotect(PROT_WRITE), you
*have* lost the WP bit, and you'll COW on a write, and lose the
coherency.

Now, I'm willing to just say: "if you do page pinning, and then do
mprotect(PROT_READ), and then do mprotect(PROT_WRITE) and then write
to the page, you really do get to keep both broken pieces". IOW, I'm
perfectly happy to just say you get what you deserve.

But I'd also be perfectly happy to make the whole "I'm the exclusive
user" logic a bit more extensive. Right now it's basically _purely_
page_count(), and the other part of "I'm the exclusive owner" is that
the RW bit in the page table is simply not clear. That makes things
really easy for COW: it just won't happen in the first place if you
broke the "obviously exclusive" rule with GUP.

But we _could_ do something slightly smarter. But "page_mapcount()" is
not that "slightly smarter" thing, because we already know it's broken
wrt lots of other uses (GUP, page cache, whatever).

Just having a bit in the page flags for "I already made this
exclusive, and fork() will keep it so" is I feel the best option. In a
way, "page is writable" right now _is_ that bit. By definition, if you
have a writable page in an anonymous mapping, that is an exclusive
user.

But because "writable" has these interactions with other operations,
it would be better if it was a harder bit than that "maybe_pinned()",
though. It would be lovely if a regular non-pinning write-GUP just
always set it, for example.

"maybe_pinned()" is good enough for the fork() case, which is the one
that matters for long-term pinning. But it's admittedly not perfect.

                  Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Jason Gunthorpe]

On Sun, Jan 10, 2021 at 11:30:57AM -0800, Linus Torvalds wrote:

> Notice how this is all both conceptually fairly simple (ie I can
> explain the rules in plain English without really making any complex
> argument) and it is arguably internally fairly self-consistent (ie the
> whole notion of "oh, there's another thing that has write access that
> page but doesn't go through the page table, so trying to make it
> read-only in the page tables is a nonsensical operation").

Yes exactly!
 
> Are the end results wrt something like soft-dirty a bit odd? Not
> really. If you do soft-dirty, such a GUP-shared page would simply
> always show up as dirty. That's still consistent with the rules. If
> somebody else may be writing to it because of GUP, that page really
> *isn't* clean, and us marking it read-only would be just lying about
> things.
> 
> I'm admittedly not very happy about mprotect() itself, though. It's
> actually ok to do the mprotect(PROT_READ) and turn the page read-only:
> that will also disable COW itself (because a page fault will now be a
> SIGSEGV, not a COW).

I would say even mprotect should not set write protect on page under
DMA, it seems like the sort of thing that will trip up other parts of
the mm that might sensibly assume read-only actually means read only,
not 'probably read-only but there might be DMA writes anyhow'

So copy the page before write protecting it? Then the explicit
mprotect is one of the system calls that can cause de-coherence of the
DMA - like munmap.

If we had reliable pinned detection I'd say to fail the mprotect..

And I think you are right about clear refs too. Clear refs must be
aware of FOLL_LONGTERM and handle it properly - which includes not
write protecting such a page.

Jason

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[John Hubbard]

On 1/10/21 11:30 AM, Linus Torvalds wrote:
> On Sat, Jan 9, 2021 at 7:51 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>
> Just having a bit in the page flags for "I already made this
> exclusive, and fork() will keep it so" is I feel the best option. In a
> way, "page is writable" right now _is_ that bit. By definition, if you
> have a writable page in an anonymous mapping, that is an exclusive
> user.
> 
> But because "writable" has these interactions with other operations,
> it would be better if it was a harder bit than that "maybe_pinned()",
> though. It would be lovely if a regular non-pinning write-GUP just
> always set it, for example.
> 
> "maybe_pinned()" is good enough for the fork() case, which is the one
> that matters for long-term pinning. But it's admittedly not perfect.
> 

There is at least one way to improve this part of it--maybe.

IMHO, a lot of the bits in page _refcount are still being wasted (even
after GUP_PIN_COUNTING_BIAS overloading), because it's unlikely that
there are many callers of gup/pup per page. If anyone points out that
that is wrong, then the rest of this falls apart, but...if we were to
make a rule that "only a very few FOLL_GET or FOLL_PIN pins are ever
simultaneously allowed on a given page", then several things become
possible:

1) "maybe dma pinned" could become "very likely dma-pinned", by allowing
perhaps 23 bits for normal page refcounts (leaving just 8 bits for dma
pins), thus all but ensuring no aliasing between normal refcounts and
dma pins. The name change below to "likely" is not actually necessary,
I'm just using it to make the point clear:


diff --git a/include/linux/mm.h b/include/linux/mm.h
index ecdf8a8cd6ae..28f7f64e888f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1241,7 +1241,7 @@ static inline void put_page(struct page *page)
   * get_user_pages and page_mkclean and other calls that race to set up page
   * table entries.
   */
-#define GUP_PIN_COUNTING_BIAS (1U << 10)
+#define GUP_PIN_COUNTING_BIAS (1U << 23)

  void unpin_user_page(struct page *page);
  void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
@@ -1274,7 +1274,7 @@ void unpin_user_pages(struct page **pages, unsigned long npages);
   * @Return:    True, if it is likely that the page has been "dma-pinned".
   *             False, if the page is definitely not dma-pinned.
   */
-static inline bool page_maybe_dma_pinned(struct page *page)
+static inline bool page_likely_dma_pinned(struct page *page)
  {
         if (hpage_pincount_available(page))
                 return compound_pincount(page) > 0;


2) Doing (1) allows, arguably, failing mprotect calls if
page_likely_dma_pinned() returns true, because the level of confidence
is much higher now.

3) An additional counter, for normal gup (FOLL_GET) is possible: divide
up the top 8 bits into two separate counters of just 4 bits each. Only
allow either FOLL_GET or FOLL_PIN (and btw, I'm now mentally equating
FOLL_PIN with FOLL_LONGTERM), not both, on a given page. Like this:

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ecdf8a8cd6ae..ce5af27e3057 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1241,7 +1241,8 @@ static inline void put_page(struct page *page)
   * get_user_pages and page_mkclean and other calls that race to set up page
   * table entries.
   */
-#define GUP_PIN_COUNTING_BIAS (1U << 10)
+#define DMA_PIN_COUNTING_BIAS (1U << 23)
+#define GUP_PIN_COUNTING_BIAS (1U << 27)

  void unpin_user_page(struct page *page);
  void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,

So:

FOLL_PIN: would use DMA_PIN_COUNTING_BIAS to increment page refcount.
These are long term pins for dma.

FOLL_GET: would use GUP_PIN_COUNTING_BIAS to increment page refcount.
These are not long term pins.

Doing (3) would require yet another release call variant:
unpin_user_pages() would need to take a flag to say which refcount to
release: FOLL_GET or FOLL_PIN. However, I think that's relatively easy
(compared to the original effort of teasing apart generic put_page()
calls, into unpin_user_pages() calls). We already have all the
unpin_user_pages() calls in place, and so it's "merely" a matter of
adding a flag to 74 call sites.

The real question is whether we can get away with supporting only a very
low count of FOLL_PIN and FOLL_GET pages. Can we?


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Matthew Wilcox]

On Sun, Jan 10, 2021 at 11:26:57PM -0800, John Hubbard wrote:
> IMHO, a lot of the bits in page _refcount are still being wasted (even
> after GUP_PIN_COUNTING_BIAS overloading), because it's unlikely that
> there are many callers of gup/pup per page. If anyone points out that
> that is wrong, then the rest of this falls apart, but...if we were to
> make a rule that "only a very few FOLL_GET or FOLL_PIN pins are ever
> simultaneously allowed on a given page", then several things become
> possible:

There's "the normal case" and then there's "the attacker case" where
someone's deliberately trying to wrap page->_refcount.  There are lots of
interesting games people can play with an anon page, like stuffing it into
(lots of) pipes, forking lots of children, starting lots of O_DIRECT I/O
against it to a FUSE filesystem that's deliberately engineered to be slow.

We have some protection against that, but I'm not 100% sure it's working,
and making it easier to increase refcount in large chunks makes it more
likely that we would defeat that protection.

Re: [PATCH 1/1] mm: restore full accuracy in COW page reuse

[Kirill A. Shutemov]

On Sat, Jan 09, 2021 at 09:54:05PM -0500, Andrea Arcangeli wrote:
> Hello,
> 
> On Sat, Jan 09, 2021 at 07:44:35PM -0500, Andrea Arcangeli wrote:
> > allowing a child to corrupt memory in the parent. That's a problem
> > that could happen not-maliciously too. So the scenario described
> 
> I updated the above partly quoted sentence since in the previous
> version it didn't have full accuracy:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/commit/?id=fc5a76b1c14e5e6cdc64ece306fc03773662d98a
> 
> "However since a single transient GUP pin on a tail page, would elevate
> the page_count for all other tail pages (unlike the mapcount which is
> subpage granular), the COW page reuse inaccuracy would then cross
> different vmas and the effect would happen at a distance in vma of
> different processes. A single GUP pin taken on a subpage mapped in a
> different process could trigger 511 false positive COWs copies in the
> local process, after a fork()."
> 
> This a best effort to try to document all side effects, but it'd be
> great to hear from Kirill too on the above detail to have
> confirmation.

Yes, this side effect is possible. But I wouldn't worry about too much. If
it routinely happens in a real workloads (I doubt it does), the workload
can tune it with MADV_NOHUGEPAGE/MADV_DONTFORK/MADV_WIPEONFORK or
something.

-- 
 Kirill A. Shutemov

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Jason Gunthorpe]

On Sat, Jan 09, 2021 at 09:51:14PM -0500, Andrea Arcangeli wrote:

> Are we spending 32bit in mm_struct atomic_t just to call atomic_set(1)
> on it? Why isn't it a MMF_HAS_PINNED that already can be set
> atomically under mmap_read_lock too? There's bit left free there, we
> didn't run out yet to justify wasting another 31 bits. I hope I'm
> overlooking something.

It needs to be atomic because it is set under gup fast, no mmap
lock. Peter and myself did not find another place to put this that was
already atomic

> The existence of FOLL_LONGTERM is good and makes a difference at times
> for writeback if it's on a MAP_SHARED, or it makes difference during
> GUP to do a page_migrate before taking the pin, but for the whole rest
> of the VM it's irrelevant if it's long or short term, so I'm also
> concerned from what Jason mentioned about long term pins being treated
> differently within the VM. 

Why? They are different. write protect doesn't stop modification of
the data. How is that not a relavent and real difference?

Jason

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Jason Gunthorpe]

On Sun, Jan 10, 2021 at 11:26:57PM -0800, John Hubbard wrote:

> So:
> 
> FOLL_PIN: would use DMA_PIN_COUNTING_BIAS to increment page refcount.
> These are long term pins for dma.
> 
> FOLL_GET: would use GUP_PIN_COUNTING_BIAS to increment page refcount.
> These are not long term pins.

Do we have any places yet that care about the long-term-ness?

The only place long term would matter is if something is waiting for
the page ref count to drop.

Jason

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Michal Hocko]

On Mon 11-01-21 12:05:49, Jason Gunthorpe wrote:
> On Sun, Jan 10, 2021 at 11:26:57PM -0800, John Hubbard wrote:
> 
> > So:
> > 
> > FOLL_PIN: would use DMA_PIN_COUNTING_BIAS to increment page refcount.
> > These are long term pins for dma.
> > 
> > FOLL_GET: would use GUP_PIN_COUNTING_BIAS to increment page refcount.
> > These are not long term pins.
> 
> Do we have any places yet that care about the long-term-ness?

I was hoping to use that information to distinguish ephemeral migration
failures due to page reference from long term pins. The later would be a
hard failure for the migration.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Sun, Jan 10, 2021 at 11:27 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> There is at least one way to improve this part of it--maybe.

It's problematic..

> IMHO, a lot of the bits in page _refcount are still being wasted (even
> after GUP_PIN_COUNTING_BIAS overloading), because it's unlikely that
> there are many callers of gup/pup per page.

It may be unlikely under real loads.

But we've actually had overflow issues on this because rather than
real loads you can do attack loads (ie "lots of processes, lots of
pipe file descriptors, lots of vmsplice() operations on the same
page".

We had to literally add that conditional "try_get_page()" that
protects against overflow..

           Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Mon, Jan 11, 2021 at 11:19 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Jan 10, 2021 at 11:27 PM John Hubbard <jhubbard@nvidia.com> wrote:
> > IMHO, a lot of the bits in page _refcount are still being wasted (even
> > after GUP_PIN_COUNTING_BIAS overloading), because it's unlikely that
> > there are many callers of gup/pup per page.
>
> It may be unlikely under real loads.
>
> But we've actually had overflow issues on this because rather than
> real loads you can do attack loads (ie "lots of processes, lots of
> pipe file descriptors, lots of vmsplice() operations on the same
> page".
>
> We had to literally add that conditional "try_get_page()" that
> protects against overflow..

Actually, what I think might be a better model is to actually
strengthen the rules even more, and get rid of GUP_PIN_COUNTING_BIAS
entirely.

What we could do is just make a few clear rules explicit (most of
which we already basically hold to). Starting from that basic

 (a) Anonymous pages are made writable (ie COW) only when they have a
page_count() of 1

That very simple rule then automatically results in the corollary

 (b) a writable page in a COW mapping always starts out reachable
_only_ from the page tables

and now we could have a couple of really simple new rules:

 (c) we never ever make a writable page in a COW mapping read-only
_unless_ it has a page_count() of 1

 (d) we never create a swap cache page out of a writable COW mapping page

Now, if you combine these rules, the whole need for the
GUP_PIN_COUNTING_BIAS basically goes away.

Why? Because we know that the _only_ thing that can elevate the
refcount of a writable COW page is GUP - we'll just make sure nothing
else touches it.

The whole "optimistic page references throigh page cache" etc are
complete non-issues, because the whole point is that we already know
it's not a page cache page. There is simply no other way to reach that
page than through GUP.

Ergo: any writable pte in a COW mapping that has a page with a
page_count() > 1 is pinned by definition, and thus our

   page_maybe_dma_pinned(page)

could remove that "maybe" part, and simply check for

    page_count(page) > 1

(although the rule would be that this is only valid for a cow_mapping
pte, and only while holding the page table lock! So maybe it would be
good to pass in the vma and have an assert for that lock too).

And the thing is, none of the above rules are complicated.

The only new one would be the requirement that you cannot add a page
to the swap cache unless it is read-only in the page tables. That may
be the biggest hurdle here.

The way we handle swap cache is that we *first* add it to the swap
cache, and then we do a "try_to_unmap()" on it. So we currently don't
actually try to walk the page tables until we have already done that
swap cache thing.

But I do think that the only major problem spot is that
shrink_page_list() -> add_to_swap() case, and I think we could make
add_to_swap() just do the rmap walk and turn it read-only first.

(And it's worth pointing out that I'm only talking about regular
non-huge pages above, the rules for splitting hugepages may impact
those cases differently, I didn't really even try to think about those
cases).

But thatadd_to_swap() case might make it too painful. It _would_
simplify our rules wrt anonymous mappings enormously, though.

               Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Andy Lutomirski]

On Mon, Jan 11, 2021 at 2:18 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Jan 11, 2021 at 11:19 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:

> Actually, what I think might be a better model is to actually
> strengthen the rules even more, and get rid of GUP_PIN_COUNTING_BIAS
> entirely.
>
> What we could do is just make a few clear rules explicit (most of
> which we already basically hold to). Starting from that basic
>
>  (a) Anonymous pages are made writable (ie COW) only when they have a
> page_count() of 1

Seems reasonable to me.

>
> That very simple rule then automatically results in the corollary
>
>  (b) a writable page in a COW mapping always starts out reachable
> _only_ from the page tables

Seems reasonable.  I guess that if the COW is triggered by GUP, then
it starts out reachable only from the page tables but then because
reachable through GUP very soon thereafter.

>
> and now we could have a couple of really simple new rules:
>
>  (c) we never ever make a writable page in a COW mapping read-only
> _unless_ it has a page_count() of 1

I don't love this.  Having mprotect() fail in a multithreaded process
because another thread happens to be doing a short-lived IO seems like
it may result in annoying intermittent bugs.

As I understand it, the issue is that the way we determine that we
need to COW a COWable page is that we see that it's read-only.  It
would be nice if we could separately track "the VMA allows writes" and
"this PTE points to a page that is private to the owning VMA", but
maybe there's no bit available for the latter other than looking at RO
vs RW directly.

>
>  (d) we never create a swap cache page out of a writable COW mapping page
>
> Now, if you combine these rules, the whole need for the
> GUP_PIN_COUNTING_BIAS basically goes away.
>
> Why? Because we know that the _only_ thing that can elevate the
> refcount of a writable COW page is GUP - we'll just make sure nothing
> else touches it.

How common is !FOLL_WRITE GUP?  We could potentially say that a
short-term !FOLL_WRITE GUP is permitted on an RO COW page and that a
subsequent COW on the page will wait for the GUP to go away.  This
might be too big a can of worms for the benefit it would provide,
though.

--Andy

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Jerome Glisse]

On Mon, Jan 11, 2021 at 02:18:13PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2021 at 11:19 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sun, Jan 10, 2021 at 11:27 PM John Hubbard <jhubbard@nvidia.com> wrote:
> > > IMHO, a lot of the bits in page _refcount are still being wasted (even
> > > after GUP_PIN_COUNTING_BIAS overloading), because it's unlikely that
> > > there are many callers of gup/pup per page.
> >
> > It may be unlikely under real loads.
> >
> > But we've actually had overflow issues on this because rather than
> > real loads you can do attack loads (ie "lots of processes, lots of
> > pipe file descriptors, lots of vmsplice() operations on the same
> > page".
> >
> > We had to literally add that conditional "try_get_page()" that
> > protects against overflow..
> 
> Actually, what I think might be a better model is to actually
> strengthen the rules even more, and get rid of GUP_PIN_COUNTING_BIAS
> entirely.
> 
> What we could do is just make a few clear rules explicit (most of
> which we already basically hold to). Starting from that basic
> 
>  (a) Anonymous pages are made writable (ie COW) only when they have a
> page_count() of 1
> 
> That very simple rule then automatically results in the corollary
> 
>  (b) a writable page in a COW mapping always starts out reachable
> _only_ from the page tables
> 
> and now we could have a couple of really simple new rules:
> 
>  (c) we never ever make a writable page in a COW mapping read-only
> _unless_ it has a page_count() of 1

This breaks mprotect(R_ONLY) i do not think we want to do that. This
might break security scheme for user space application which expect
mprotect to make CPU mapping reads only.

Maybe an alternative would be to copy page on mprotect for pages that
do not have a page_count of 1 ? But that makes me uneasy toward short
lived GUP (direct IO racing with a mprotect or maybe simply even page
migration) versus unbound one (like RDMA).


Also I want to make sure i properly understand what happens on fork()
on a COW mapping for a page that has a page_count > 1 ? We copy the
page instead of write protecting the page ?

I believe better here would be to protect the page on the CPU but
forbid child to reuse the page ie if the child ever inherit the page
(parent unmapped the page for instance) it will have to make a copy
and the GUP reference (taken before the fork) might linger on a page
that is no longer associated with any VM. This way we keep fast fork.


Jérôme

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Matthew Wilcox]

On Mon, Jan 11, 2021 at 02:18:13PM -0800, Linus Torvalds wrote:
> The whole "optimistic page references throigh page cache" etc are
> complete non-issues, because the whole point is that we already know
> it's not a page cache page. There is simply no other way to reach that
> page than through GUP.

The thing about the speculative page cache references is that they can
temporarily bump a refcount on a page which _used_ to be in the page
cache and has now been reallocated as some other kind of page.

Now, this is obviously rare, so if it's only a performance question,
it'll be fine.  If there's a correctness issue with copying pages that
would otherwise not have been copied, then it's a problem.

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> The thing about the speculative page cache references is that they can
> temporarily bump a refcount on a page which _used_ to be in the page
> cache and has now been reallocated as some other kind of page.

Right you are. Yeah, scratch the "we could make it absolute on 1", and
we do need the PIN count elevation.

           Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> The thing about the speculative page cache references is that they can
> temporarily bump a refcount on a page which _used_ to be in the page
> cache and has now been reallocated as some other kind of page.

Oh, and thinking about this made me think we might actually have a
serious bug here, and it has nothing what-so-ever to do with COW, GUP,
or even the page count itself.

It's unlikely enough that I think it's mostly theoretical, but tell me
I'm wrong.

PLEASE tell me I'm wrong:

CPU1 does page_cache_get_speculative under RCU lock

CPU2 frees and re-uses the page

    CPU1                CPU2
    ----                ----

    page = xas_load(&xas);
    if (!page_cache_get_speculative(page))
            goto repeat;
    .. succeeds ..

                        remove page from XA
                        release page
                        reuse for something else

    .. and then re-check ..
    if (unlikely(page != xas_reload(&xas))) {
            put_page(page);
            goto repeat;
    }

ok, the above all looks fine. We got the speculative ref, but then we
noticed that its' not valid any more, so we put it again. All good,
right?

Wrong.

What if that "reuse for something else" was actually really quick, and
both allocated and released it?

That still sounds good, right? Yes, now the "put_page()" will be the
one that _actually_ releases the page, but we're still fine, right?

Very very wrong.

The "reuse for something else" on CPU2 might have gotten not an
order-0 page, but a *high-order* page. So it allocated (and then
immediately free'd) maybe an order-2 allocation with _four_ pages, and
the re-use happened when we had coalesced the buddy pages.

But when we release the page on CPU1, we will release just _one_ page,
and the other three pages will be lost forever.

IOW, we restored the page count perfectly fine, but we screwed up the
page sizes and buddy information.

Ok, so the above is so unlikely from a timing standpoint that I don't
think it ever happens, but I don't see why it couldn't happen in
theory.

Please somebody tell me I'm missing some clever thing we do to make
sure this can actually not happen..

         Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[David Hildenbrand]

On 13.01.21 04:31, Linus Torvalds wrote:
> On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <willy@infradead.org> wrote:
>>
>> The thing about the speculative page cache references is that they can
>> temporarily bump a refcount on a page which _used_ to be in the page
>> cache and has now been reallocated as some other kind of page.
> 
> Oh, and thinking about this made me think we might actually have a
> serious bug here, and it has nothing what-so-ever to do with COW, GUP,
> or even the page count itself.
> 
> It's unlikely enough that I think it's mostly theoretical, but tell me
> I'm wrong.
> 
> PLEASE tell me I'm wrong:
> 
> CPU1 does page_cache_get_speculative under RCU lock
> 
> CPU2 frees and re-uses the page
> 
>     CPU1                CPU2
>     ----                ----
> 
>     page = xas_load(&xas);
>     if (!page_cache_get_speculative(page))
>             goto repeat;
>     .. succeeds ..
> 
>                         remove page from XA
>                         release page
>                         reuse for something else
> 
>     .. and then re-check ..
>     if (unlikely(page != xas_reload(&xas))) {
>             put_page(page);
>             goto repeat;
>     }
> 
> ok, the above all looks fine. We got the speculative ref, but then we
> noticed that its' not valid any more, so we put it again. All good,
> right?
> 
> Wrong.
> 
> What if that "reuse for something else" was actually really quick, and
> both allocated and released it?
> 
> That still sounds good, right? Yes, now the "put_page()" will be the
> one that _actually_ releases the page, but we're still fine, right?
> 
> Very very wrong.
> 
> The "reuse for something else" on CPU2 might have gotten not an
> order-0 page, but a *high-order* page. So it allocated (and then
> immediately free'd) maybe an order-2 allocation with _four_ pages, and
> the re-use happened when we had coalesced the buddy pages.
> 
> But when we release the page on CPU1, we will release just _one_ page,
> and the other three pages will be lost forever.
> 
> IOW, we restored the page count perfectly fine, but we screwed up the
> page sizes and buddy information.
> 
> Ok, so the above is so unlikely from a timing standpoint that I don't
> think it ever happens, but I don't see why it couldn't happen in
> theory.
> 
> Please somebody tell me I'm missing some clever thing we do to make
> sure this can actually not happen..

Wasn't that tackled by latest (not merged AFAIKs) __free_pages() changes?

I'm only able to come up with the doc update, not with the oroginal
fix/change

https://lkml.kernel.org/r/20201027025523.3235-1-willy@infradead.org

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[David Hildenbrand]

On 13.01.21 09:52, David Hildenbrand wrote:
> On 13.01.21 04:31, Linus Torvalds wrote:
>> On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <willy@infradead.org> wrote:
>>>
>>> The thing about the speculative page cache references is that they can
>>> temporarily bump a refcount on a page which _used_ to be in the page
>>> cache and has now been reallocated as some other kind of page.
>>
>> Oh, and thinking about this made me think we might actually have a
>> serious bug here, and it has nothing what-so-ever to do with COW, GUP,
>> or even the page count itself.
>>
>> It's unlikely enough that I think it's mostly theoretical, but tell me
>> I'm wrong.
>>
>> PLEASE tell me I'm wrong:
>>
>> CPU1 does page_cache_get_speculative under RCU lock
>>
>> CPU2 frees and re-uses the page
>>
>>     CPU1                CPU2
>>     ----                ----
>>
>>     page = xas_load(&xas);
>>     if (!page_cache_get_speculative(page))
>>             goto repeat;
>>     .. succeeds ..
>>
>>                         remove page from XA
>>                         release page
>>                         reuse for something else
>>
>>     .. and then re-check ..
>>     if (unlikely(page != xas_reload(&xas))) {
>>             put_page(page);
>>             goto repeat;
>>     }
>>
>> ok, the above all looks fine. We got the speculative ref, but then we
>> noticed that its' not valid any more, so we put it again. All good,
>> right?
>>
>> Wrong.
>>
>> What if that "reuse for something else" was actually really quick, and
>> both allocated and released it?
>>
>> That still sounds good, right? Yes, now the "put_page()" will be the
>> one that _actually_ releases the page, but we're still fine, right?
>>
>> Very very wrong.
>>
>> The "reuse for something else" on CPU2 might have gotten not an
>> order-0 page, but a *high-order* page. So it allocated (and then
>> immediately free'd) maybe an order-2 allocation with _four_ pages, and
>> the re-use happened when we had coalesced the buddy pages.
>>
>> But when we release the page on CPU1, we will release just _one_ page,
>> and the other three pages will be lost forever.
>>
>> IOW, we restored the page count perfectly fine, but we screwed up the
>> page sizes and buddy information.
>>
>> Ok, so the above is so unlikely from a timing standpoint that I don't
>> think it ever happens, but I don't see why it couldn't happen in
>> theory.
>>
>> Please somebody tell me I'm missing some clever thing we do to make
>> sure this can actually not happen..
> 
> Wasn't that tackled by latest (not merged AFAIKs) __free_pages() changes?
> 
> I'm only able to come up with the doc update, not with the oroginal
> fix/change
> 
> https://lkml.kernel.org/r/20201027025523.3235-1-willy@infradead.org
> 

Sorry, found it, it's in v5.10

commit e320d3012d25b1fb5f3df4edb7bd44a1c362ec10
Author: Matthew Wilcox (Oracle) <willy@infradead.org>
Date:   Tue Oct 13 16:56:04 2020 -0700

    mm/page_alloc.c: fix freeing non-compound pages

and

commit 7f194fbb2dd75e9346b305b8902e177b423b1062
Author: Matthew Wilcox (Oracle) <willy@infradead.org>
Date:   Mon Dec 14 19:11:09 2020 -0800

    mm/page_alloc: add __free_pages() documentation

is in v5.11-rc1

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Kirill A. Shutemov]

On Tue, Jan 12, 2021 at 07:31:07PM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > The thing about the speculative page cache references is that they can
> > temporarily bump a refcount on a page which _used_ to be in the page
> > cache and has now been reallocated as some other kind of page.
> 
> Oh, and thinking about this made me think we might actually have a
> serious bug here, and it has nothing what-so-ever to do with COW, GUP,
> or even the page count itself.
> 
> It's unlikely enough that I think it's mostly theoretical, but tell me
> I'm wrong.
> 
> PLEASE tell me I'm wrong:
> 
> CPU1 does page_cache_get_speculative under RCU lock
> 
> CPU2 frees and re-uses the page
> 
>     CPU1                CPU2
>     ----                ----
> 
>     page = xas_load(&xas);
>     if (!page_cache_get_speculative(page))
>             goto repeat;
>     .. succeeds ..
> 
>                         remove page from XA
>                         release page
>                         reuse for something else

How can it be reused if CPU1 hold reference to it?

> 
>     .. and then re-check ..
>     if (unlikely(page != xas_reload(&xas))) {
>             put_page(page);
>             goto repeat;
>     }
> 

-- 
 Kirill A. Shutemov

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Matthew Wilcox]

On Wed, Jan 13, 2021 at 03:32:32PM +0300, Kirill A. Shutemov wrote:
> On Tue, Jan 12, 2021 at 07:31:07PM -0800, Linus Torvalds wrote:
> > On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > The thing about the speculative page cache references is that they can
> > > temporarily bump a refcount on a page which _used_ to be in the page
> > > cache and has now been reallocated as some other kind of page.
> > 
> > Oh, and thinking about this made me think we might actually have a
> > serious bug here, and it has nothing what-so-ever to do with COW, GUP,
> > or even the page count itself.
> > 
> > It's unlikely enough that I think it's mostly theoretical, but tell me
> > I'm wrong.
> > 
> > PLEASE tell me I'm wrong:
> > 
> > CPU1 does page_cache_get_speculative under RCU lock
> > 
> > CPU2 frees and re-uses the page
> > 
> >     CPU1                CPU2
> >     ----                ----
> > 
> >     page = xas_load(&xas);
> >     if (!page_cache_get_speculative(page))
> >             goto repeat;
> >     .. succeeds ..
> > 
> >                         remove page from XA
> >                         release page
> >                         reuse for something else
> 
> How can it be reused if CPU1 hold reference to it?

Yes, Linus mis-stated it:

    page = xas_load(&xas);
                        remove page from XA
                        release page
                        reuse for something else
    if (!page_cache_get_speculative(page))
            goto repeat;
    if (unlikely(page != xas_reload(&xas))) {
            put_page(page);

... but as David pointed out, I fixed this in e320d3012d25

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Linus Torvalds]

On Wed, Jan 13, 2021 at 4:56 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> Yes, Linus mis-stated it:

Yeah, I got the order wrong.

> ... but as David pointed out, I fixed this in e320d3012d25

.. and I must have seen it, but not really internalized it.

And now that I look at it more closely, I'm actually surprised that
other than the magic "speculative first page" case we don't seem to
use page reference counting for non-order-0 pages (which would break
that hack horribly).

             Linus

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Peter Xu]

On Sun, Jan 10, 2021 at 11:30:57AM -0800, Linus Torvalds wrote:
> So if you start off with the rule that "I will always COW unless I can
> trivially see I'm the only owner", then I think we have really made
> for a really clear and unambiguous rule.

I must confess that's the major reason that when I saw the COW simplification
patch I felt it great.  Not only because it's an easier way to understand all
these, but also that it helped unbreak uffd-wp at that moment by dropping the
patch of "COW for read gups" instead of introducing more things to fix stuff,
like the FOLL_BREAK_COW idea [1].

But it's a pity that only until now, after I read all the threads... I found we
might start to lose things for the beautifulness the COW patch brought.

It turns out the simplicity is just not for free; there is always a cost.  The
complexity around GUP always existed probably because GUP is hard itself! It's
just a matter of where the complexity resides: either in the do_wp_page(), or
in the rest of the codes in some other ways, e.g., a complicated version of
page_trans_huge_map_swapcount().

For example, in the future we'll have no way to wr-protect any pinned pages, no
matter for soft-dirty or uffd-wp or else: it'll be illegal by definition!
While it's not extremely easy to get the reasoning from instinct of human being
I'd say...  "DMA pinned pages could be written after all by device", that's
true, but how about read DMA pins?  Oh read DMA pin does not exist because if
we try read DMA pin it broke... but is that a strong enough reason?

We probably want to fix mprotect() too with pinned pages, because even if
mprotect(READ) is fine then in mprotect(READ|WRITE) we'll need to make sure the
write bit being solid if the page is pinned (because now we'll assume all
pinned pages should always be with the write bit set in ptes), or another
alternative could be we'll just fail some mprotect() upon pinned pages.

Limitations just start to pile up, it seems to me..

We also unveiled the vmsplice issue with thp, because for now on COW we don't
have symmetric behavior for small/huge pages any more: for small pages, we'll
do page_count() check to make that copy decision; while we're still with
page_mapcount() for thps.  Do we finally need to convert do_huge_pmd_wp_page()
to also use the same logic as the small pages?  The risk in front is not clear
to me, though.

GUP(write=0) will be extremely special from now on, simply because read won't
trigger COW... I must confess, from instinction, GUP(write=0) should simply
mean that "I want to get this page, but for reading".  Then I realized it's not
extremelyl obvious to me why it should be treated totally differently against a
write GUP on this matter.

Due to my lack on memory management experience, I just started to notice that a
huge amount of work that previously done hard on maintaining page_mapcount()
simply just to make sure GUP pages keep its meaning by instinction, which is:
when we GUP a page, it'll be always coherent with the page we've got in the
process page table.  Now that instinct will go away too, because the GUPed page
now could be some different page rather than the one that sits in the pgtable.

Then I remembered something very nice about the COW simplification change: As
the test case reported [2], there is a huge boost with 31.4% after COW
simplification.  Today I went back and try to understand why, because I
suddenly found I cannot fully understand it - I thought it was majorly because
of the page lock, but maybe not, since the page lock is actually a fine granule
lock in this test that covers 1G mem.

The test was carried out on a host with 192G mem + 104 cores, what it did was
simply:

  ./usemem --runtime 300 -n 104 --prealloc --prefault 959297984

./usemem is the memory workload that does memory accesses, it fork()s into 104
processes with a shared 1G mem region for those accesses so after it's all done
it could use up to 104G pages after all the COWs.  When it runs, COW happens
very frequently across merely all the cores, triggering the do_wp_page() path.

However 104 processes won't easily collapse on the same 4K page at the same
time which shares the lock.  I just noticed maybe it's simply the overhead of
reuse_swap_page().

But how heavy would reuse_swap_page() be?  It'll be heavier if THP is the case
because page_trans_huge_map_swapcount() has a complicated loop for those small
pages, then I found that indeed the test was carried out with THP enabled and
also by default on:

        CONFIG_TRANSPARENT_HUGEPAGE=y
        CONFIG_TRANSPARENT_HUGEPAGE_ALWAYS=y

IIUC that'll make reuse_swap_page() very heavy, I think.  Because initially the
100+ processes share some THPs, first writes on the THPs will split the THPs,
then reuse_swap_page() will always go the slowest path on looping over each
small pages for each small page writes of COW.

So that 31.4% number got "shrinked" a bit after I noticed all these - this is
already a specific test which merely do COW only but nothing else.  It'll be
hard to tell how many performance gain we can get by this simplification of COW
on other real-life workloads.  Not to mention that removing the
reuse_swap_page() seems to also delayed swap recycle (which is something we'll
need to do sooner or later; so if COW got fast something else got slower,
e.g. the page reclaim logic) and I noticed Ying tried to partly recover it [3].
It's not clear to me where it's the best place to do the recycle, but that
sounds like a different problem.

The major two benefits to me with the current COW simplification are simplicity
of logic and performance gains. But they start to fade away with above.  Not
really that much, but big enough to let me start questioning myself on whether
it's the best approach from pure technical pov...

Then I do also notice that actually the other path of FOLL_BREAK_COW [1] might
be able to fix all of above frustrations like mentioned by others.  It's still
complicated, I really don't like that part.  But again, it just seems to be the
matter of where the complexity would be, there's just no way to avoid those
complexity.  The thing that I'm afraid is that the complexity is by nature and
we can't change it.  I'm also afraid that if we go the current way it'll be
even more complicated at last.  So it seems wise to think more about the
direction since we're at a cross road before starting to fix all the todos
e.g. soft-dirty and mprotect against pinned pages, and all the rest of things
we'd need to fix with current COW page reuse with current solution.

I felt extremely sorry to have dumped my brain somehow, especially considering
above could be completely garbage so I wasted time for a lot of people reading
it or simply scanning it over...  However I expressed it just in case it could
help in some form.

Thanks,

[1] https://lore.kernel.org/lkml/20200811183950.10603-1-peterx@redhat.com/
[2] https://lore.kernel.org/lkml/20200914024321.GG26874@shao2-debian/
[3] https://lore.kernel.org/lkml/20210113024241.179113-1-ying.huang@intel.com/

-- 
Peter Xu

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[David Hildenbrand]

On 10.01.21 01:44, Andrea Arcangeli wrote:
> Hello Andrew and everyone,
> 
> Once we agree that COW page reuse requires full accuracy, the next
> step is to re-apply 17839856fd588f4ab6b789f482ed3ffd7c403e1f and to
> return going in that direction.


After stumbling over the heated discussion around this, I wanted to
understand the details and the different opinions. I tried to summarize
in my simple words (bear with me) what happened and how I think we can
proceed from here. Maybe that helps.

====

What happened:

1) We simplified handling of faults on write-protected pages (page table
entries): we changed the logic when we can reuse a page ("simply
unprotecting it"), and when we have to copy it instead (COW). The
essence of the simplification is, that we only reuse a page if we are
the only single user of the page, meaning page_count(page) == 1, and the
page is mapped into a single process (page_mapcount(page) == 1);
otherwise we copy it. Simple.

2) The old code was complicated and there are GUP (e.g., RDMA, VFIO)
cases that were broken in various ways in the old code already: most
prominently fork(). As one example, it would have been possible for
mprotect(READ) memory to still get modified by GUP users like RDMA.
Write protection (AFAIU via any mechanism) after GUP pinned a page was
not effective; the page was not copied.

3) Speculative pagecache reference can temporarily bump up the
page_count(page), resulting in false positives. We could see
page_count(page) > 1, although we're the single instance that actually
uses a page. In the simplified code, we might copy a page although not
necessary (I cannot tell how often that actually happens).

4) clear_refs(4) ("measure approximately how much memory a process is
using"), uffd-wp (let's call it "lightweight write-protection, handling
the actual fault in user space"), and mprotect(READ) all write-protect
page table entries to generate faults on next write access. With the
simplified code, we will COW whenever we find the page_count(page) > 1.

The simplification seemed to regress clear_refs and uffdio-wp code
(AFAIU in case of uffd-wp, it results in memory corruption). But looks
like we can mostly fix it by adding more extensive locking.

5) Mechanisms like GUP (AFAIU including Direct I/O) also takes
references on pages, increasing page_count(). With the simplification,
we might now end up copying a page, although there is "somewhat" only a
single user/"process" involved.

One example is RDMA: if we read memory using RDMA and mprotect(READ)
such memory, we might end up copying the underlying page on the next
write: suddenly, RDMA is disconnected and will no longer read what is
getting written. Not to mention, we consume more memory. AFAIU, other
examples include direct I/O (e.g., write() with O_DIRECT).

AFAIU, a more extreme case is probably VFIO: A VM with VFIO (e.g.,
passthrough of a PCI device) can essentially be corrupted by "echo 4 >
/proc/[pid]/clear_refs".

6) While some people think it is okay to break GUP further, as it is
already broken in various other ways, other people think this is
changing something that used to work (AFAIU a user-visible change) with
little benefit.

7) There is no easy way to detect if a page really was pinned: we might
have false positives. Further, there is no way to distinguish if it was
pinned with FOLL_WRITE or not (R vs R/W). To perform reliable tracking
we most probably would need more counters, which we cannot fit into
struct page. (AFAIU, for huge pages it's easier).

However, AFAIU, even being able to detect if (and how) a page was pinned
would not completely help to solve the puzzle.

8) We have a vmsplice security issue that has to be fixed by touching
the code in question. A forked child process can read memory content of
its parent, which was modified by the parent after fork. AFAIU, the fix
will further lock us in into the direction of the code we are heading.

9) The simplification is part of v5.10, which is a LTS release. AFAIU,
that one needs fixing, too.



I see the following possible directions we can head

A) Keep the simplification. Try fixing the fallout. Keep the GUP cases
broken or make mprotect() fail when detecting such a scenario;
AFAIU, both are user-visible changes.

B) Keep the simplification. Try fixing the fallout. Fix GUP cases that
used to work; AFAIU fixing this is the hard/impossible part, and is
undesired by some people..

C) Revert the simplification for now. Go back to the drawing board and
use what we learned to come up with a simplification that (all?  )
people are happy with.

D) Revert the simplification: turns out the code could not get
simplified to this extend. We learned a lot, though.

======

Please let me know in case I messed up anything and/or missed important
points.

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Jason Gunthorpe]

On Fri, Jan 15, 2021 at 09:59:23AM +0100, David Hildenbrand wrote:

> AFAIU, a more extreme case is probably VFIO: A VM with VFIO (e.g.,
> passthrough of a PCI device) can essentially be corrupted by "echo 4 >
> /proc/[pid]/clear_refs".

I've been told when doing migration with RDMA the VM's memory also
ends up pinned, and then it does the stuff of #4. So it deliberately
does clear_refs(4) on RDMA pinned memory and requires no COW. This is
now a real world uABI break, unfortunately.

> 7) There is no easy way to detect if a page really was pinned: we might
> have false positives. Further, there is no way to distinguish if it was
> pinned with FOLL_WRITE or not (R vs R/W). To perform reliable tracking
> we most probably would need more counters, which we cannot fit into
> struct page. (AFAIU, for huge pages it's easier).

I think this is the real issue. We can only store so much information,
so we have to decide which things work and which things are broken. So
far someone hasn't presented a way to record everything at least..
 
> However, AFAIU, even being able to detect if (and how) a page was pinned
> would not completely help to solve the puzzle.

At least for COW reuuse, uf we assign labels to every page user, and
imagine we can track everything, I think we get this list:
 - # of ptes referencing the page (mapcount?)
 - # of page * pointer references that don't touch data 
   (ie the speculative page cache ref)
 - # of DMA/CPU readers
 - # of DMA/CPU writers
 - # of long term data accesses
 - # of other reader/writers (specifically process incoherent reader/writers,
   not "DMA with the CPU" like vmsplice/iouring)

Maybe there are more? This is what I've understood so far from
this thread?

Today's kernel makes the COW reuse decision as:

  # ptes == 1 &&
    # refs == 0 &&
    # DMA readers == 0 &&
    # DMA writers == 0 &&
    # of longterm == 0 &&
    # other reader/writers == 0

(in essence this is what _refcount == 1 is saying, I think)

From a GUP perspective I think the useful property is "a physical page
under GUP is not indirectly removed from the mm_struct that pinned
it". This is the idea that the process CPU page table and the ongoing
DMA remain synchronized. This is a generalized statement from the
clear_refs(4) and fork() regressions.

Therefore, COW should not copy a page just because it is under GUP, it
breaks the idea directly.

We've also said speculative #refs should not cause COW. Removing both
of those gets us to the COW reuse decision as:

  # ptes == 1 &&
    # other reader/writers == 0

And I think where Linus is coming from is '# ptes' (eg mapcount) alone
is not right because there are other relavent reader/writers too. (I'm
not sure what these are, has someone pointed at one?)

So, we have 64 bits for _refcount and _mapcount and we currently encode
things as:

 - # ptes                    (_mapcount)
 - # page pointers +         (low bits of _refcount)
   # DMA reader + writers +
   # other reader/writers +
   # ptes        # We incr both _mapcount and_refcount?
 - # long term data acesses  (high bits of _refcount

If we move '# other reader/writers' to _mapcount (maybe with a shift),
does it help?

We also talked about GUP as meaning wrprotect == 0, but we could also
change that to the idea that GUP means COW will always re-use, eg
'#ptes == 1 && # other reader/writers == 0'.

This gives some definition what mprotect(PROT_READ) means to pages
under DMA (though I still think PROT_READ of pages under DMA write is
weird)

> 8) We have a vmsplice security issue that has to be fixed by touching
> the code in question. A forked child process can read memory content of
> its parent, which was modified by the parent after fork. AFAIU, the fix
> will further lock us in into the direction of the code we are heading.

No, vmsplice is just wrong. vmsplice has to do
FOLL_LONGTERM|FOLL_FORCE|FOLL_WRITE for read only access to pages if
userspace controls the duration of the pin.

There are other bad bugs, like permanently locking
DAX/CMA/ZONE_MIGRATE memory if the above pattern is not used.

There was some debate over alternatives, but for a backport security
fix it has to be above. AFAIK.

Jason

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[David Hildenbrand]

>> 7) There is no easy way to detect if a page really was pinned: we might
>> have false positives. Further, there is no way to distinguish if it was
>> pinned with FOLL_WRITE or not (R vs R/W). To perform reliable tracking
>> we most probably would need more counters, which we cannot fit into
>> struct page. (AFAIU, for huge pages it's easier).
> 
> I think this is the real issue. We can only store so much information,
> so we have to decide which things work and which things are broken. So
> far someone hasn't presented a way to record everything at least..

I do wonder how many (especially long-term) GUP readers/writers we have
to expect, and especially, support for a single base page. Do we have a
rough estimate?

With RDMA, I would assume we only need a single one (e.g., once RDMA
device; I'm pretty sure I'm wrong, sounds too easy).
With VFIO I guess we need one for each VFIO container (~ in the worst
case one for each passthrough device).
With direct I/O, vmsplice and other GUP users ?? No idea.

If we could somehow put a limit on the #GUP we support, and fail further
GUP (e.g., -EAGAIN?) once a limit is reached, we could partition the
refcount into something like (assume max #15 GUP READ and #15 GUP R/W,
which is most probably a horribly bad choice)

[ GUP READ ][ GUP R/W ] [  ordinary ]
31  ...  28 27  ...  24 23   ....   0

But due to saturate handling in "ordinary", we would lose further 2 bits
(AFAIU), leaving us "only" 22 bits for "ordinary". Now, I have no idea
how many bits we actually need in practice.

Maybe we need less for GUP READ, because most users want GUP R/W? No idea.

Just wild ideas. Most probably that has already been discussed, and most
probably people figured that it's impossible :)

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[Jason Gunthorpe]

On Fri, Jan 15, 2021 at 08:46:48PM +0100, David Hildenbrand wrote:

> Just wild ideas. Most probably that has already been discussed, and most
> probably people figured that it's impossible :)

No, I think it is all fair topics.

There is no API reason for any of this to be limited, but in practice,
I doubt there is more than small 10's

Huge pages complicate things of course

Jason

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[John Hubbard]

On 1/15/21 11:46 AM, David Hildenbrand wrote:
>>> 7) There is no easy way to detect if a page really was pinned: we might
>>> have false positives. Further, there is no way to distinguish if it was
>>> pinned with FOLL_WRITE or not (R vs R/W). To perform reliable tracking
>>> we most probably would need more counters, which we cannot fit into
>>> struct page. (AFAIU, for huge pages it's easier).
>>
>> I think this is the real issue. We can only store so much information,
>> so we have to decide which things work and which things are broken. So
>> far someone hasn't presented a way to record everything at least..
> 
> I do wonder how many (especially long-term) GUP readers/writers we have
> to expect, and especially, support for a single base page. Do we have a
> rough estimate?
> 
> With RDMA, I would assume we only need a single one (e.g., once RDMA
> device; I'm pretty sure I'm wrong, sounds too easy).
> With VFIO I guess we need one for each VFIO container (~ in the worst
> case one for each passthrough device).
> With direct I/O, vmsplice and other GUP users ?? No idea.
> 
> If we could somehow put a limit on the #GUP we support, and fail further
> GUP (e.g., -EAGAIN?) once a limit is reached, we could partition the
> refcount into something like (assume max #15 GUP READ and #15 GUP R/W,
> which is most probably a horribly bad choice)
> 
> [ GUP READ ][ GUP R/W ] [  ordinary ]
> 31  ...  28 27  ...  24 23   ....   0
> 
> But due to saturate handling in "ordinary", we would lose further 2 bits
> (AFAIU), leaving us "only" 22 bits for "ordinary". Now, I have no idea
> how many bits we actually need in practice.
> 
> Maybe we need less for GUP READ, because most users want GUP R/W? No idea.
> 
> Just wild ideas. Most probably that has already been discussed, and most
> probably people figured that it's impossible :)
> 

I proposed this exact idea a few days ago [1]. It's remarkable that we both
picked nearly identical values for the layout! :)

But as the responses show, security problems prevent pursuing that approach.


[1] https://lore.kernel.org/r/45806a5a-65c2-67ce-fc92-dc8c2144d766@nvidia.com

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 0/1] mm: restore full accuracy in COW page reuse

[David Hildenbrand]

On 16.01.21 04:40, John Hubbard wrote:
> On 1/15/21 11:46 AM, David Hildenbrand wrote:
>>>> 7) There is no easy way to detect if a page really was pinned: we might
>>>> have false positives. Further, there is no way to distinguish if it was
>>>> pinned with FOLL_WRITE or not (R vs R/W). To perform reliable tracking
>>>> we most probably would need more counters, which we cannot fit into
>>>> struct page. (AFAIU, for huge pages it's easier).
>>>
>>> I think this is the real issue. We can only store so much information,
>>> so we have to decide which things work and which things are broken. So
>>> far someone hasn't presented a way to record everything at least..
>>
>> I do wonder how many (especially long-term) GUP readers/writers we have
>> to expect, and especially, support for a single base page. Do we have a
>> rough estimate?
>>
>> With RDMA, I would assume we only need a single one (e.g., once RDMA
>> device; I'm pretty sure I'm wrong, sounds too easy).
>> With VFIO I guess we need one for each VFIO container (~ in the worst
>> case one for each passthrough device).
>> With direct I/O, vmsplice and other GUP users ?? No idea.
>>
>> If we could somehow put a limit on the #GUP we support, and fail further
>> GUP (e.g., -EAGAIN?) once a limit is reached, we could partition the
>> refcount into something like (assume max #15 GUP READ and #15 GUP R/W,
>> which is most probably a horribly bad choice)
>>
>> [ GUP READ ][ GUP R/W ] [  ordinary ]
>> 31  ...  28 27  ...  24 23   ....   0
>>
>> But due to saturate handling in "ordinary", we would lose further 2 bits
>> (AFAIU), leaving us "only" 22 bits for "ordinary". Now, I have no idea
>> how many bits we actually need in practice.
>>
>> Maybe we need less for GUP READ, because most users want GUP R/W? No idea.
>>
>> Just wild ideas. Most probably that has already been discussed, and most
>> probably people figured that it's impossible :)
>>
> 
> I proposed this exact idea a few days ago [1]. It's remarkable that we both
> picked nearly identical values for the layout! :)

Heh! Somehow I missed that. But well, there were *a lot* of mails :)

> 
> But as the responses show, security problems prevent pursuing that approach.
It still feels kind of wrong to waste valuable space in the memmap.


In an ideal world (well, one that still only allows for a 64 byte memmap
:) ), we would:

1) Partition the refcount into separate fields that cannot overflow into
each other, similar to my example above, but maybe add even more fields.

2) Reject attempts that would result in an overflow to everything except
the "ordinary" field (e.g., GUP fields in my example above).

3) Put an upper limit on the "ordinary" field that we ever expect for
sane workloads (E.g., 10 bits). In addition, reserve some bits (like the
saturate logic) that we handle as a "red zone".

4) For the "ordinary" field, as soon as we enter the red zone, we know
we have an attack going on. We continue on paths that we cannot fail
(e.g., get_page()) but eventually try stopping the attacker(s). AFAIU,
we know the attacker(s) are something (e.g., one ore multiple processes)
that has direct access to the page in their address space. Of course,
the more paths we can reject, the better.


Now, we would:

a) Have to know what sane upper limits on the "ordinary" field are. I
have no idea which values we can expect. Attacker vs. sane workload.

b) Need a way to identify the attacker(s). In the simplest case, this is
a single process. In the hard case, this involves many processes.

c) Need a way to stop the attacker(s). Doing that out of random context
is problematic. Last resort is doing this asynchronously from another
thread, which leaves more time for the attacker to do harm.


Of course, problem gets more involved as soon as we might have a
malicious child process that uses a page from a well-behaving parent
process for the attack.

Imagine we kill relevant processes, we might end up killing someone
who's not responsible. And even if we don't kill, but instead reject
try_get_page(), we might degrade the well-behaving parent process AFAIKS.

Alternatives to killing the process might be unmapping the problematic
page from the address space.

Reminds me a little about handling memory errors for a page, eventually
killing all users of that page. mm/memory-failure.c:kill_procs().


Complicated problem :)

-- 
Thanks,

David / dhildenb