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

[Dev Jain]

On 26/06/26 9:51 am, Muchun Song wrote:
> 
> 
>> On Jun 26, 2026, at 12:08, Dev Jain <dev.jain@xxxxxxx> wrote:
>>
>>
>>
>> On 26/06/26 9:10 am, Muchun Song wrote:
>>>
>>>
>>> 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
>>
>> I was following pattern elsewhere, search for ".hugetlb_entry" in the
>> codebase and you will find others doing the same.
> 
> Okay, I understand why you want to do it that way, but I would still
> recommend not following that format.

Okay then I'll update v2 with the below diff.

> 
> Thanks.
> 
>>
>>
>>> 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)
> 
> 
>