Re: [PATCH] mm: thp: Deny THP for guest_memfd and secretmem in file_thp_enabled()

[Ackerley Tng]

"David Hildenbrand (Arm)" <david@xxxxxxxxxx> writes:

Going to try and summarize the findings/discussions here, copying from a
few earlier emails. David, you can jump directly to [Question].

> On 2/11/26 16:38, Ackerley Tng wrote:
>> "David Hildenbrand (Arm)" <david@xxxxxxxxxx> writes:
>>
>>> On 2/11/26 00:00, Ackerley Tng wrote:
>>>> "David Hildenbrand (Arm)" <david@xxxxxxxxxx> writes:
>>>>
>>>>
>>>> Seems like on 5.15.199 there's a hugepage_vma_check(), which will return
>>>> false since secretmem has vma->vm_ops defined [1], so secretmem VMAs are
>>>> skipped.
>>>
>>> Are you sure? We check for CONFIG_READ_ONLY_THP_FOR_FS before that:
>>>
>>
>> Ah... I was working on a reproducer then I realized 5.15 doesn't have
>> MADV_COLLAPSE, then I tried to hack in an ioctl to trigger
>> khugepaged. That turned out to be awkward but it got me to look at
>> hugepage_vma_check(), and then I went down the rabbit hole to keep
>> looking for the similar check function throughout the other stable
>> kernels... and amongst all of that forgot that
>> CONFIG_READ_ONLY_THP_FOR_FS was unset :(
>>
>> You're probably right about VM_EXEC.
>>

[Bug]
khugepaged (and MADV_COLLAPSE) will try to collapse secretmem pages with
MADV_HUGEPAGE applied. There is no crash, but there is a false memory
failure printout that looks like

    [ 1068.322578] Memory failure: 0x106d96f: recovery action for
clean unevictable LRU page: Recovered

The correct Fixes tag should be:

    Fixes: 7fbb5e188248 ("mm: remove VM_EXEC requirement for THP eligibility")

I was able to reproduce this on 6.12, 6.18 and HEAD

[Stable Backports]
The first stable version this affects is 6.12.

In 6.12, S_ANON_INODE does not yet exist, so I think in
file_thp_enabled() we can return false if vma_is_secretmem(vma).

6.18 needs a fix for both secretmem and guest_memfd.

[Solution]
For 6.18 and later, David's suggestion of using IS_ANON_FILE() seems to
work. This affects more filesystems than just secretmem and guest_memfd
though.

[Question]
I'm not familiar with the concept of anonymous inodes. What does that
entail? Why is it suitable in deciding THP eligibility?

[Next Steps]
I'm going to be traveling over the next few weeks, so perhaps Deepanshu
can help with the fixup patches for 6.12, 6.18 and HEAD?

[Details]
Here's a reproducer for 6.18 for guest_memfd x MADV_COLLAPSE

diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c
b/tools/testing/selftests/kvm/guest_memfd_test.c
index e7d9aeb418d3..8760fe6fa482 100644
--- a/tools/testing/selftests/kvm/guest_memfd_test.c
+++ b/tools/testing/selftests/kvm/guest_memfd_test.c
@@ -371,10 +371,45 @@ static void test_guest_memfd_guest(void)
        kvm_vm_free(vm);
 }

+#define ALIGNED_ADDRESS ((void *)0x400000000UL)
+
+static void repro(void)
+{
+       struct kvm_vcpu *vcpu;
+       struct kvm_vm *vm;
+       uint8_t *mem;
+       int fd, i;
+
+       vm = __vm_create_shape_with_one_vcpu(VM_SHAPE_DEFAULT, &vcpu,
1, guest_code);
+
+       fd = vm_create_guest_memfd(vm, SZ_2M, GUEST_MEMFD_FLAG_MMAP |
+                                             GUEST_MEMFD_FLAG_INIT_SHARED);
+
+       mem = mmap(ALIGNED_ADDRESS, SZ_2M, PROT_READ | PROT_WRITE,
MAP_FIXED | MAP_SHARED, fd, 0);
+       TEST_ASSERT_EQ(mem, ALIGNED_ADDRESS);
+
+       for (i = 0; i < SZ_2M; i += getpagesize())
+               READ_ONCE(mem[i]);
+
+       TEST_ASSERT_EQ(madvise(mem, SZ_2M, MADV_HUGEPAGE), 0);
+
+       TEST_ASSERT_EQ(madvise(mem, SZ_2M, MADV_COLLAPSE), 0);
+
+       TEST_ASSERT_EQ(madvise(mem, SZ_2M, MADV_DONTNEED), 0);
+
+       READ_ONCE(mem[0]);
+
+       close(fd);
+       kvm_vm_free(vm);
+}
+
 int main(int argc, char *argv[])
 {
        unsigned long vm_types, vm_type;

+       repro();
+       return 1;
+
        TEST_REQUIRE(kvm_has_cap(KVM_CAP_GUEST_MEMFD));

        page_size = getpagesize();

console shows warning:

[  558.315452] WARNING: CPU: 1 PID: 252 at
arch/x86/kvm/../../../virt/kvm/guest_memfd.c:372
kvm_gmem_fault_user_mapping+0x120/0x1c0

stdout output:

# /mnt/host/kvm/guest_memfd_test
Random seed: 0x6b8b4567
__vm_create: mode='PA-bits:ANY, VA-bits:48,  4K pages' type='0', pages='657'
Guest physical address width detected: 46
Bus error
#

Here's a more complete reproducer for 6.12 for secretmem x MADV_COLLAPSE

diff --git a/tools/testing/selftests/mm/memfd_secret.c
b/tools/testing/selftests/mm/memfd_secret.c
index 9a0597310a76..2a0c5cc9fe20 100644
--- a/tools/testing/selftests/mm/memfd_secret.c
+++ b/tools/testing/selftests/mm/memfd_secret.c
@@ -21,6 +21,7 @@
 #include <errno.h>
 #include <stdio.h>
 #include <fcntl.h>
+#include <stddef.h>

 #include "../kselftest.h"

@@ -299,10 +300,145 @@ static void prepare(void)

 #define NUM_TESTS 6

+#define SZ_2M (2UL << 20)
+#define ALIGNED_ADDRESS ((void *)0x400000000UL)
+#define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
+
+uint64_t get_pfn(void *addr) {
+       uint64_t pagemap_entry;
+       static int fd = -1;
+       uintptr_t offset;
+       uintptr_t vaddr;
+
+       if (fd < 0) {
+               fd = open("/proc/self/pagemap", O_RDONLY);
+               if (fd < 0)
+                       ksft_exit_fail_msg("open pagemap\n");
+       }
+
+       vaddr = (uintptr_t)addr;
+       offset = (vaddr / getpagesize()) * sizeof(uint64_t);
+
+       if (pread(fd, &pagemap_entry, sizeof(uint64_t), offset) !=
sizeof(uint64_t))
+               ksft_exit_fail_msg("pread pagemap\n");
+
+
+       /* Bit 63 is "present" */
+       if (!(pagemap_entry & (1ULL << 63)))
+               ksft_exit_fail_msg("Page not present in userspace pagemap\n");
+
+       /* Bits 0-54 are the PFN */
+       return pagemap_entry & ((1ULL << 55) - 1);
+}
+
+bool in_direct_map(uint64_t pfn) {
+       static int devmem_fd = -1;
+       uint8_t bounce;
+
+       if (devmem_fd < 0) {
+               devmem_fd = open("/dev/mem", O_RDONLY);
+               if (devmem_fd < 0)
+                       ksft_exit_fail_msg("Can't open /dev/mem:
%s\n", strerror(errno));
+       }
+
+       if (pread(devmem_fd, &bounce, 1, pfn * getpagesize()) == 1) {
+               return true;
+       } else {
+               if (errno == EFAULT)
+                       return false;
+               else if (errno == EPERM)
+                       ksft_exit_fail_msg("Access probably blocked:
%s\n", strerror(errno));
+               else
+                       perror("pread /dev/mem");
+
+               return false;
+       }
+}
+
+void check(void)
+{
+       uint64_t pfn;
+       uint8_t *mem;
+
+       mem = mmap(NULL, SZ_2M, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
+       if (mem == MAP_FAILED)
+               ksft_exit_fail_msg("Couldn't allocate memory\n");
+
+       mem[0] = 'A';
+
+       pfn = get_pfn(mem);
+       printf("%d pfn=%lx in_direct_map=%d\n", __LINE__, pfn,
in_direct_map(pfn));
+
+       munmap(mem, SZ_2M);
+}
+
+void repro(void)
+{
+       uint64_t pfn;
+       uint8_t *mem;
+       int ret;
+       int fd;
+       int i;
+
+       printf("%d triggering secretmem\n", __LINE__);
+
+       fd = memfd_secret(0);
+       if (fd < 0) {
+               if (errno == ENOSYS)
+                       ksft_exit_skip("memfd_secret is not supported\n");
+               else
+                       ksft_exit_fail_msg("memfd_secret failed: %s\n",
+                                          strerror(errno));
+       }
+
+       if (ftruncate(fd, SZ_2M))
+               ksft_exit_fail_msg("ftruncate failed: %s\n", strerror(errno));
+
+       mem = mmap(ALIGNED_ADDRESS, SZ_2M, PROT_READ | PROT_WRITE,
MAP_FIXED | MAP_SHARED, fd, 0);
+       if (mem != ALIGNED_ADDRESS)
+               ksft_exit_fail_msg("Couldn't allocate memory\n");
+
+       for (i = 0; i < SZ_2M; i += getpagesize())
+               READ_ONCE(mem[i]);
+
+       pfn = get_pfn(mem);
+       printf("%d pfn=%lx in_direct_map=%d\n", __LINE__, pfn,
in_direct_map(pfn));
+
+       ret = madvise(mem, SZ_2M, MADV_HUGEPAGE);
+       if (ret)
+               ksft_exit_fail_msg("MADV_HUGEPAGE failed mem=%p ret=%d
errno=%d\n", mem, ret, errno);
+
+       ret = madvise(mem, SZ_2M, MADV_COLLAPSE);
+       if (ret != -1 || errno != EINVAL)
+               ksft_exit_fail_msg("MADV_COLLAPSE should have failed
ret=%d errno=%d\n", ret, errno);
+
+       /*
+        * Sleep allows memory_failure to complete, IIUC. If memory
+         * failure handling doesn't complete, faulting in memory in
+         * the next step fails with SIGBUS, as expected.
+        */
+       sleep(1);
+
+       for (i = 0; i < SZ_2M; i += getpagesize())
+               READ_ONCE(mem[i]);
+
+       printf("%d pfn=%lx in_direct_map=%d\n", __LINE__, pfn,
in_direct_map(pfn));
+
+       pfn = get_pfn(mem);
+       printf("%d new pfn=%lx in_direct_map=%d\n", __LINE__, pfn,
in_direct_map(pfn));
+
+       munmap(mem, SZ_2M);
+       close(fd);
+}
+
 int main(int argc, char *argv[])
 {
        int fd;

+       check();
+       repro();
+       return 1;
+
        prepare();

        ksft_print_header();

Special configs:

+ Enable CONFIG_READ_ONLY_THP_FOR_FS
+ Disable CONFIG_STRICT_DEVMEM (so that reading /dev/mem will return
  -EFAULT for memory not in the direct map, just for testing)

stdout output with annotations:

# /mnt/host/mm/memfd_secret
370 pfn=106a600 in_direct_map=1      <<== my check that direct map check works
383 triggering secretmem
405 pfn=106f568 in_direct_map=0      <<== secretmem is indeed not in
the direct map
425 pfn=106f568 in_direct_map=1      <<== after memory failure
handling, folio is restored to direct map
428 new pfn=106be67 in_direct_map=0  <<== next fault: secretmem has a
new folio not in the direct map
#

>>
>> [...snip...]
>>