On Thu, Feb 2, 2023 at 8:09 AM Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
>
> On Wed, Feb 01, 2023 at 12:41:37PM +0900, David Stevens wrote:
> > From: David Stevens <stevensd@xxxxxxxxxxxx>
> >
> > Collapsing memory in a vma that has an armed userfaultfd results in
> > zero-filling any missing pages, which breaks user-space paging for those
> > filled pages. Avoid khugepage bypassing userfaultfd by not collapsing
> > pages in shmem reached via scanning a vma with an armed userfaultfd if
> > doing so would zero-fill any pages.
>
> Could you elaborate on the failure? Will zero-filling the page prevent
> userfaultfd from catching future access?
Yes, zero-filling the page causes future major faults to be lost,
since it populates the pages in the backing shmem. The path for
anonymous memory in khugepaged does properly handle userfaultfd_armed,
but the path for shmem does not.
> A test-case would help a lot.
Here's a sample program that demonstrates the issue. On a v6.1 kernel,
no major fault is observed by the monitor thread. I used MADV_COLLAPSE
to exercise khugepaged_scan_file, but you would get the same effect by
replacing the madvise with a sleep and waiting for khugepaged to scan
the test process.
#define _GNU_SOURCE
#include <inttypes.h>
#include <stdio.h>
#include <linux/userfaultfd.h>
#include <threads.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/ioctl.h>
#include <assert.h>
int had_fault;
int monitor_thread(void *arg) {
int ret, uffd = (int) (uintptr_t) arg;
struct uffd_msg msg;
ret = read(uffd, &msg, sizeof(msg));
assert(ret > 0);
assert(msg.event == UFFD_EVENT_PAGEFAULT);
had_fault = 1;
struct uffdio_zeropage zeropage;
zeropage.range.start = msg.arg.pagefault.address & ~0xfff;
zeropage.range.len = 4096;
zeropage.mode = 0;
ret = ioctl(uffd, UFFDIO_ZEROPAGE, &zeropage);
assert(ret >= 0);
}
int main() {
int ret;
int uffd = syscall(__NR_userfaultfd, 0);
assert(uffd >= 0);
struct uffdio_api uffdio_api;
uffdio_api.api = UFFD_API;
uffdio_api.features = UFFD_FEATURE_MISSING_SHMEM;
ret = ioctl(uffd, UFFDIO_API, &uffdio_api);
assert(ret >= 0);
int memfd = memfd_create("memfd", MFD_CLOEXEC);
assert(memfd >= 0);
ret = ftruncate(memfd, 2 * 1024 * 1024);
assert(ret >= 0);
uint8_t *addr = mmap(NULL, 2 * 1024 * 1024, PROT_READ | PROT_WRITE,
MAP_SHARED, memfd, 0);
assert(addr != MAP_FAILED);
addr[0] = 0xff;
struct uffdio_register uffdio_register;
uffdio_register.range.start = (unsigned long) addr;
uffdio_register.range.len = 2 * 1024 * 1024;
uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
ret = ioctl(uffd, UFFDIO_REGISTER, &uffdio_register);
assert(ret >= 0);
thrd_t t;
ret = thrd_create(&t, monitor_thread, (void *) (uintptr_t) uffd);
assert(ret >= 0);
ret = madvise(addr, 2 * 1024 * 1024, 25 /* MADV_COLLAPSE */);
printf("madvise ret %d\n", ret);
addr[4096] = 0xff;
printf("%s major fault\n", had_fault ? "had" : "no");
return 0;
}
> And what prevents the same pages be filled (with zeros or otherwise) via
> write(2) bypassing VMA checks? I cannot immediately see it.
There isn't any such check. You can bypass userfaultfd on a shmem with
write syscalls, or simply by writing to the shmem through a different
vma. However, it is definitely possible for userspace to use shmem
plus userfaultfd in a safe way. And the kernel shouldn't come along
and break a setup that should be safe from the perspective of
userspace.
> BTW, there's already a check that prevent establishing PMD in the place if
> VM_UFFD_WP is set.
>
> Maybe just an update of the check in retract_page_tables() from
> userfaultfd_wp() to userfaultfd_armed() would be enough?
It seems like it will be a little more complicated than that, because
the target VM having an armed userfaultfd is a hard error condition.
However, it might not be that difficult to modify collapse_file to
rollback if necessary. I'll consider this approach for v2 of this
patch.
-David
> I have very limited understanding of userfaultfd(). Sorry in advance for
> stupid questions.
>
> --
> Kiryl Shutsemau / Kirill A. Shutemov
