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

[Ackerley Tng <ackerleytng@google.com>]

Ackerley Tng <ackerleytng@google.com> writes:

> "David Hildenbrand (Arm)" <david@kernel.org> writes:
>
>>>> BUT, something just occurred to me.
>>>>
>>>> We added the mc-handling in
>>>>
>>>> commit 98c76c9f1ef7599b39bfd4bd99b8a760d4a8cd3b
>>>> Author: Jiaqi Yan <jiaqiyan@google.com>
>>>> Date:   Wed Mar 29 08:11:19 2023 -0700
>>>>
>>>>       mm/khugepaged: recover from poisoned anonymous memory
>>>>
>>>> ..
>>>>
>>>> So I assume kernels before that would crash when collapsing?
>>>>
>>>> Looking at 5.15.199, it does not contain 98c76c9f1e [1].
>>>>
>>>> So I suspect we need a fix+stable backport.
>>>>
>>>> Who volunteers to try a secretmem reproducer on a stable kernel? :)
>>>>
>>>
>>> I could give this a shot. 5.15.199 doesn't have AS_INACCESSIBLE. Should
>>> we backport AS_INACCESSIBLE there or could the fix for 5.15.199 just be
>>> special-casing secretmem like you suggested below?
>>
>> Yes. If there is no guest_memfd we wouldn't need it.
>>
>
> 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.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/khugepaged.c?h=v5.15.199#n469
>

On 6.1.162, secretmem VMAs are skipped since secretmem VMAs are not
anonymous [2].

[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/huge_memory.c?h=v6.1.162#n135

Same for 6.6.123 [3].

[3] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/huge_memory.c?h=v6.6.123#n125

It breaks in 6.12.69 [4].

[4] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/huge_memory.c?h=v6.12.69#n159

IIUC the patch that enabled khugepaged for secretmem is

commit 7a81751fcdeb833acc858e59082688e3020bfe12
Author: Zach O'Keefe <zokeefe@google.com>
Date:   Mon Sep 25 13:01:10 2023 -0700

    mm/thp: fix "mm: thp: kill __transhuge_page_enabled()"

...

@@ -132,12 +132,18 @@ bool hugepage_vma_check(struct vm_area_struct
*vma, unsigned long vm_flags,
                                           !hugepage_flags_always())))
                return false;

-       /* Only regular file is valid */
-       if (!in_pf && file_thp_enabled(vma))
-               return true;
-
-       if (!vma_is_anonymous(vma))
+       if (!vma_is_anonymous(vma)) {
+               /*
+                * Trust that ->huge_fault() handlers know what they are doing
+                * in fault path.
+                */
+               if (((in_pf || smaps)) && vma->vm_ops->huge_fault)
+                       return true;
+               /* Only regular file is valid in collapse path */
+               if (((!in_pf || smaps)) && file_thp_enabled(vma))
+                       return true;
                return false;
+       }

        if (vma_is_temporary_stack(vma))
                return false;

Because file_thp_enabled() would return true for secretmem.

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