Re: [PATCH] arm64/mm: restrict transparent contpte mappings to valid PTEs

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/19/26 09:06, Zhao Gongyi wrote:
> On 10/14/26 5:41 PM, David Hildenbrand (Arm) wrote:
>> CCing Ryan:
>>
>> https://lore.kernel.org/r/20261013093600.54995-1-zhaogongyi@bytedance.com
>>
>> On 10/13/26 11:36, Zhao Gongyi wrote:
>>> The transparent contpte layer treats PTE_CONT as software state: when a
>>> PTE has PTE_CONT set, the contpte helpers may align the operation to a
>>> CONT_PTE_SIZE block and operate on the whole block. That invariant only
>>> makes sense for valid mappings. Explicit users such as hugetlb manage
>>> their contiguous encodings separately.
>>>
>>> contpte_set_ptes() currently sets PTE_CONT from alignment alone. Batched
>>> set_ptes() users can therefore install present-invalid PTEs with
>>> PTE_CONT set, for example mprotect(PROT_NONE), NUMA hinting through
>>> modify_prot_commit_ptes(), fork() copying a PROT_NONE large folio, or a
>>> PMD split installing PAGE_NONE PTEs.
>>>
>>> This is not a hardware programming problem: invalid descriptors are not
>>> translated, so the hardware ignores descriptor bits such as PTE_CONT. It
>>> is a software-state problem. Some arm64 fast paths intentionally test
>>> pte_cont() directly instead of pte_valid_cont(), for example commit
>>> 9970a9a27ffc ("arm64: mm: implement the architecture-specific
>>> test_and_clear_young_ptes()"). Fixing the producer side preserves these
>>> fast paths and covers all consumers.
>>>
>>> In the observed failure, mprotect(PROT_NONE) installed a full
>>> CONT_PTE_SIZE range of present-invalid PTEs with PTE_CONT set.
>>> MADV_GUARD_INSTALL then zapped one PTE. The nr == 1 clear_full_ptes()
>>> path only unfolds pte_valid_cont() entries, so the neighbouring
>>> present-invalid entries kept PTE_CONT while the target entry became a
>>> guard marker.
>>
>> Hm, is there a good reason why we cannot just use the cont-pte bit for all
>> *present* PTEs, even if HW would not be prepared to benefit (!pte_valid())?
>>
>> I guess we really only have to make sure to shatter properly, right?
>>
> 
> 
> Hi David,
> 
> Right, there is no architectural restriction preventing PTE_CONT from
> being used as software grouping state for present-invalid PTEs. For this
> particular failure, making contpte_try_unfold() recognize something like
> 
>          pte_present(pte) && pte_cont(pte)
> 
> would make clear_full_ptes(nr == 1) unfold the block before replacing one
> entry with the guard marker.
> 
> My reason for preferring the producer-side fix is that the existing
> transparent contpte helpers mostly use pte_valid_cont() as the membership
> test, including contpte_try_unfold(), ptep_get[_lockless](),
> pte_batch_hint() and contpte_is_consistent(), while some range fast paths
> test pte_cont() directly.
> 
> If present-invalid PTEs are also considered members of a transparent
> contpte block, I think we should audit the single-PTE modification paths,
> rather than changing only this clear path. We would likely also need a
> pte_present_cont() helper: the pte_present() guard matters because the
> PTE_CONT bit position is part of the swap offset for non-present entries.
> 
> There is no hardware benefit while the descriptors are invalid, and
> set_ptes() can recreate the contiguous encoding when an eligible range is
> made valid again. For a stable fix, I therefore preferred to stop
> contpte_set_ptes() from creating present-invalid transparent contpte
> mappings, which also matches the pte_valid_cont() contract used by most
> of the existing helpers.
> 
> I agree that saying this invariant "only makes sense" for valid mappings
> is too strong. I will reword that to describe the current helper
> contract instead.
> 
> If preserving the software grouping across PROT_NONE is the intended
> semantics, I can rework this as a broader unfold-path audit instead.

Hi,

sorry for the late reply.

If we have the spare PTE bit, why shouldn't we just use it?

It will make batching over PTEs faster. And we care about that for present PTEs.

So I don't quite see why the stable fix couldn't just do it right away? Do you
have a prototype that shows that it would require significant code changes?

-- 
Cheers,

David