Re: [PATCH 5/5] mm/mprotect: use huge_ptep_get() for hugetlb

[Muchun Song]

On 2026/6/25 19:29, Dev Jain wrote:
> prot_none_hugetlb_entry() is the hugetlb callback for the early
> mprotect(PROT_NONE) PFN permission walk on x86.
>
> The callback passes the decoded PFN to pfn_modify_allowed(). For a
> hugetlb callback, the pte pointer refers to a hugetlb entry. On
> architectures where hugetlb entries need huge_ptep_get(), reading that
> entry with ptep_get() can make the permission check use the wrong PFN.
>
> Use huge_ptep_get() before decoding the hugetlb PFN.
>
> Currently there is no path which can trigger a bug: huge_ptep_get() is a
> simple ptep_get() for x86, and the prot_none walk occurs only for x86.
> But use the correct helper anyways.
>
> Fixes: 42e4089c7890 ("x86/speculation/l1tf: Disallow non privileged high MMIO PROT_NONE mappings")
> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> ---
>   mm/mprotect.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 9cbf932b028cf..23779632d18bf 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -699,14 +699,20 @@ static int prot_none_pte_entry(pte_t *pte, unsigned long addr,
>   		0 : -EACCES;
>   }
>   
> +#ifdef CONFIG_HUGETLB_PAGE
>   static int prot_none_hugetlb_entry(pte_t *pte, unsigned long hmask,
>   				   unsigned long addr, unsigned long next,
>   				   struct mm_walk *walk)
>   {
> -	return pfn_modify_allowed(pte_pfn(ptep_get(pte)),
> +	pte_t entry = huge_ptep_get(walk->mm, addr, pte);
> +
> +	return pfn_modify_allowed(pte_pfn(entry),
>   				  *(pgprot_t *)(walk->private)) ?
>   		0 : -EACCES;
>   }
> +#else
> +#define prot_none_hugetlb_entry	NULL

This is very strange, because we defined a stub as NULL for a helper
function. How about  the following diff?

diff --git a/mm/mprotect.c b/mm/mprotect.c
index 9cbf932b028c..4d8c1551fbce 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -716,7 +716,9 @@ static int prot_none_test(unsigned long addr, 
unsigned long next,

 static const struct mm_walk_ops prot_none_walk_ops = {
        .pte_entry              = prot_none_pte_entry,
+#ifdef CONFIG_HUGETLB_PAGE
        .hugetlb_entry          = prot_none_hugetlb_entry,
+#endif
        .test_walk              = prot_none_test,
        .walk_lock              = PGWALK_WRLOCK,
 };

Thanks,
Muchun

> +#endif
>   
>   static int prot_none_test(unsigned long addr, unsigned long next,
>   			  struct mm_walk *walk)