linux-kernel - Re: [PATCH] arm64/mm: Elide TLB flush in certain pte protection transitions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0968ad4d-8804-4d87-8e74-45ecf213b9d1@arm.com>
Date: Fri, 19 Sep 2025 10:45:11 +0530
From: Dev Jain <dev.jain@....com>
To: Ryan Roberts <ryan.roberts@....com>, catalin.marinas@....com,
 will@...nel.org
Cc: anshuman.khandual@....com, wangkefeng.wang@...wei.com, baohua@...nel.org,
 pjaroszynski@...dia.com, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64/mm: Elide TLB flush in certain pte protection
 transitions


On 18/09/25 8:52 pm, Ryan Roberts wrote:
> On 18/09/2025 16:04, Dev Jain wrote:
>> On 18/09/25 6:19 pm, Ryan Roberts wrote:
>>> On 18/09/2025 11:36, Dev Jain wrote:
>>>> Currently arm64 does an unconditional TLB flush in mprotect(). This is not
>>>> required for some cases, for example, when changing from PROT_NONE to
>>>> PROT_READ | PROT_WRITE (a real usecase - glibc malloc does this to emulate
>>>> growing into the non-main heaps), and unsetting uffd-wp in a range.
>>>>
>>>> Therefore, implement pte_needs_flush() for arm64, which is already
>>>> implemented by some other arches as well.
>>>>
>>>> Running a userspace program changing permissions back and forth between
>>>> PROT_NONE and PROT_READ | PROT_WRITE, and measuring the average time taken
>>>> for the none->rw transition, I get a reduction from 3.2 microseconds to
>>>> 2.95 microseconds, giving an 8.5% improvement.
>>>>
>>>> Signed-off-by: Dev Jain <dev.jain@....com>
>>>> ---
>>>> mm-selftests pass. Based on 6.17-rc6.
>>>>
>>>>    arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++
>>>>    1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/
>>>> tlbflush.h
>>>> index 18a5dc0c9a54..4a566d589100 100644
>>>> --- a/arch/arm64/include/asm/tlbflush.h
>>>> +++ b/arch/arm64/include/asm/tlbflush.h
>>>> @@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct
>>>> arch_tlbflush_unmap_batch *b
>>>>    {
>>>>        __flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3);
>>>>    }
>>>> +
>>>> +static inline bool __pte_flags_need_flush(pteval_t oldval, pteval_t newval)
>>> ptdesc_t is the preferred any-level type.
>> I keep forgetting this :)
>>
>>>> +{
>>>> +    pteval_t diff = oldval ^ newval;
>>>> +
>>>> +    /* invalid to valid transition requires no flush */
>>>> +    if (!(oldval & PTE_VALID) || (oldval & PTE_PRESENT_INVALID))
>>> Is the PTE_PRESENT_INVALID really required? If the oldval was invalid, there
>>> can't be a TLB entry for it, so no flush is required; that's it, I think?
>>>
>>> In fact, PTE_PRESENT_INVALID is overlaid with PTE_NG; it only means
>>> PTE_PRESENT_INVALID when PTE_INVALID=0, so I think this is broken as is. Valid
>>> user-space PTEs always have PTE_NG set, so you will never flush.
>> Not sure I get you. The condition as I wrote means
>>
>> 1. If PTE_VALID is not set, then do not flush.
>> 2. If PTE_VALID is set, *and* PTE_PRESENT_INVALID is set, then do not flush.
>>
>> So when you say "it only means PTE_PRESENT_INVALID when PTE_INVALID=0", the
>> second condition meets that.
> Sorry I meant PTE_VALID=0. Your second condition is wrong; PTE_PRESENT_INVALID
> is only defined when PTE_VALID=0.
>
> Think about it; the PTE is valid from the HW's perspective if and only if
> PTE_VALID=1. So that's the only condition that needs to be checked.
>
>
> See this comment in the code for more info:
>
> /*
>   * PTE_PRESENT_INVALID=1 & PTE_VALID=0 indicates that the pte's fields should be
>   * interpreted according to the HW layout by SW but any attempted HW access to
>   * the address will result in a fault. pte_present() returns true.
>   */
> #define PTE_PRESENT_INVALID	(PTE_NG)     /* only when !PTE_VALID */

Got it, thanks.

>
>>>> +        return false;
>>>> +
>>>> +    /* Transition in the SW bits and access flag requires no flush */
>>>> +    diff &= ~(PTE_SWBITS_MASK | PTE_AF);
>>> Could you explain your thinking on why PTE_AF changes don't need a flush? I
>>> would have thought if we want to clear the access flag, that would definitely
>>> require a flush? Otherwise how would the MMU know to set the acccess bit on next
>>> access if it already has a TLB entry?
>> You are correct, but AFAIK losing access bit information is not fatal, it will only
>> mess with page aging. So potentially reclaim will lose some accuracy.
> Sure, but it means that your change has a cost; reduced page aging accuracy.
> That part of the change should at least be separated into its own commit and
> probably backed up with performance numbers. Otherwise, I think we should retain
> the original behaviour.

You are right, I checked other callsites clearing the access bit and they do some
sort of TLB maintenance. I'll drop this.

>
>
>>>> +
>>>> +    if (!diff)
>>>> +        return false;
>>>> +    return true;
>>> Perhaps just "return !!diff;" here?
>> Sure.
>>
>>> Thanks,
>>> Ryan
>>>
>>>
>>>> +}
>>>> +
>>>> +static inline bool pte_needs_flush(pte_t oldpte, pte_t newpte)
>>>> +{
>>>> +    return __pte_flags_need_flush(pte_val(oldpte), pte_val(newpte));
>>>> +}
>>>> +#define pte_needs_flush pte_needs_flush
>>>> +
>>>> +static inline bool huge_pmd_needs_flush(pmd_t oldpmd, pmd_t newpmd)
>>>> +{
>>>> +    return __pte_flags_need_flush(pmd_val(oldpmd), pmd_val(newpmd));
>>>> +}
>>>> +#define huge_pmd_needs_flush huge_pmd_needs_flush
>>>> +
>>>>    #endif
>>>>      #endif