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(-)
On Sat, Jan 9, 2021 at 4:44 PM Andrea Arcangeli <[email protected]> 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
On Sat, Jan 9, 2021 at 4:55 PM Linus Torvalds
<[email protected]> 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
On Sat, Jan 9, 2021 at 5:19 PM Linus Torvalds
<[email protected]> 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
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(¤t->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
On Sat, Jan 09, 2021 at 05:37:09PM -0800, Linus Torvalds wrote:
> On Sat, Jan 9, 2021 at 5:19 PM Linus Torvalds
> <[email protected]> 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
On Sat, Jan 9, 2021 at 6:51 PM Andrea Arcangeli <[email protected]> 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
On Sat, Jan 9, 2021 at 7:51 PM Linus Torvalds
<[email protected]> 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
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
On 1/10/21 11:30 AM, Linus Torvalds wrote:
> On Sat, Jan 9, 2021 at 7:51 PM Linus Torvalds
> <[email protected]> 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
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.
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
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
On Sun, Jan 10, 2021 at 11:27 PM John Hubbard <[email protected]> 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
On Mon, Jan 11, 2021 at 11:19 AM Linus Torvalds
<[email protected]> wrote:
>
> On Sun, Jan 10, 2021 at 11:27 PM John Hubbard <[email protected]> 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
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
On Mon, Jan 11, 2021 at 2:18 PM Linus Torvalds
<[email protected]> wrote:
>
> On Mon, Jan 11, 2021 at 11:19 AM Linus Torvalds
> <[email protected]> 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
On Mon, Jan 11, 2021 at 02:18:13PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2021 at 11:19 AM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Sun, Jan 10, 2021 at 11:27 PM John Hubbard <[email protected]> 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
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.
On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <[email protected]> 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
On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <[email protected]> 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
On 13.01.21 04:31, Linus Torvalds wrote:
> On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <[email protected]> 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/[email protected]
--
Thanks,
David / dhildenb
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 <[email protected]> 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/[email protected]
>
Sorry, found it, it's in v5.10
commit e320d3012d25b1fb5f3df4edb7bd44a1c362ec10
Author: Matthew Wilcox (Oracle) <[email protected]>
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) <[email protected]>
Date: Mon Dec 14 19:11:09 2020 -0800
mm/page_alloc: add __free_pages() documentation
is in v5.11-rc1
--
Thanks,
David / dhildenb
On Tue, Jan 12, 2021 at 07:31:07PM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2021 at 6:16 PM Matthew Wilcox <[email protected]> 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
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 <[email protected]> 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
On Wed, Jan 13, 2021 at 4:56 AM Matthew Wilcox <[email protected]> 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
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/[email protected]/
[2] https://lore.kernel.org/lkml/20200914024321.GG26874@shao2-debian/
[3] https://lore.kernel.org/lkml/[email protected]/
--
Peter Xu
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
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
>> 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
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
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/[email protected]
thanks,
--
John Hubbard
NVIDIA
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