linux-kernel - Re: [PATCH v3] arm64: Enable vmalloc-huge with ptdump

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <6455797b-44b6-4b81-93e8-e7d193b05333@arm.com>
Date: Mon, 16 Jun 2025 22:04:44 +0530
From: Dev Jain <dev.jain@....com>
To: David Hildenbrand <david@...hat.com>, catalin.marinas@....com,
 will@...nel.org
Cc: anshuman.khandual@....com, quic_zhenhuah@...cinc.com,
 ryan.roberts@....com, kevin.brodsky@....com, yangyicong@...ilicon.com,
 joey.gouly@....com, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] arm64: Enable vmalloc-huge with ptdump


On 16/06/25 8:30 pm, David Hildenbrand wrote:
> On 16.06.25 12:33, Dev Jain wrote:
>> arm64 disables vmalloc-huge when kernel page table dumping is enabled,
>> because an intermediate table may be removed, potentially causing the
>> ptdump code to dereference an invalid address. We want to be able to
>> analyze block vs page mappings for kernel mappings with ptdump, so to
>> enable vmalloc-huge with ptdump, synchronize between page table 
>> removal in
>> pmd_free_pte_page()/pud_free_pmd_page() and ptdump pagetable walking. We
>> use mmap_read_lock and not write lock because we don't need to 
>> synchronize
>> between two different vm_structs; two vmalloc objects running this same
>> code path will point to different page tables, hence there is no race.
>>
>> For pud_free_pmd_page(), we isolate the PMD table to avoid taking the 
>> lock
>> 512 times again via pmd_free_pte_page().
>>
>> We implement the locking mechanism using static keys, since the chance
>> of a race is very small. Observe that the synchronization is needed
>> to avoid the following race:
>>
>> CPU1                            CPU2
>>                         take reference of PMD table
>> pud_clear()
>> pte_free_kernel()
>>                         walk freed PMD table
>>
>> and similar race between pmd_free_pte_page and ptdump_walk_pgd.
>>
>> Therefore, there are two cases: if ptdump sees the cleared PUD, then
>> we are safe. If not, then the patched-in read and write locks help us
>> avoid the race.
>>
>> To implement the mechanism, we need the static key access from mmu.c and
>> ptdump.c. Note that in case !CONFIG_PTDUMP_DEBUGFS, ptdump.o won't be a
>> target in the Makefile, therefore we cannot initialize the key there, as
>> is being done, for example, in the static key implementation of
>> hugetlb-vmemmap. Therefore, include asm/cpufeature.h, which includes
>> the jump_label mechanism. Declare the key there and define the key to 
>> false
>> in mmu.c.
>>
>> No issues were observed with mm-selftests. No issues were observed while
>> parallelly running test_vmalloc.sh and dumping the kernel pagetable 
>> through
>> sysfs in a loop.
>>
>> v2->v3:
>>   - Use static key mechanism
>>
>> v1->v2:
>>   - Take lock only when CONFIG_PTDUMP_DEBUGFS is on
>>   - In case of pud_free_pmd_page(), isolate the PMD table to avoid 
>> taking
>>     the lock 512 times again via pmd_free_pte_page()
>>
>> Signed-off-by: Dev Jain <dev.jain@....com>
>> ---
>>   arch/arm64/include/asm/cpufeature.h |  1 +
>>   arch/arm64/mm/mmu.c                 | 51 ++++++++++++++++++++++++++---
>>   arch/arm64/mm/ptdump.c              |  5 +++
>>   3 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h 
>> b/arch/arm64/include/asm/cpufeature.h
>> index c4326f1cb917..3e386563b587 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -26,6 +26,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/cpumask.h>
>>   +DECLARE_STATIC_KEY_FALSE(ptdump_lock_key);
>>   /*
>>    * CPU feature register tracking
>>    *
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 8fcf59ba39db..e242ba428820 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -41,11 +41,14 @@
>>   #include <asm/tlbflush.h>
>>   #include <asm/pgalloc.h>
>>   #include <asm/kfence.h>
>> +#include <asm/cpufeature.h>
>>     #define NO_BLOCK_MAPPINGS    BIT(0)
>>   #define NO_CONT_MAPPINGS    BIT(1)
>>   #define NO_EXEC_MAPPINGS    BIT(2)    /* assumes FEAT_HPDS is not 
>> used */
>>   +DEFINE_STATIC_KEY_FALSE(ptdump_lock_key);
>> +
>>   enum pgtable_type {
>>       TABLE_PTE,
>>       TABLE_PMD,
>> @@ -1267,8 +1270,9 @@ int pmd_clear_huge(pmd_t *pmdp)
>>       return 1;
>>   }
>>   -int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>> +static int __pmd_free_pte_page(pmd_t *pmdp, unsigned long addr, bool 
>> lock)
>>   {
>> +    bool lock_taken = false;
>>       pte_t *table;
>>       pmd_t pmd;
>>   @@ -1279,15 +1283,29 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned 
>> long addr)
>>           return 1;
>>       }
>>   +    /* See comment in pud_free_pmd_page for static key logic */
>>       table = pte_offset_kernel(pmdp, addr);
>>       pmd_clear(pmdp);
>>       __flush_tlb_kernel_pgtable(addr);
>> +    if (static_branch_unlikely(&ptdump_lock_key) && lock) {
>> +        mmap_read_lock(&init_mm);
>> +        lock_taken = true;
>> +    }
>> +    if (unlikely(lock_taken))
>> +        mmap_read_unlock(&init_mm);
>> +
>
> I'm missing something important: why not
>
> if (static_branch_unlikely(&ptdump_lock_key) && lock) {
>     mmap_read_lock(&init_mm);
>     mmap_read_unlock(&init_mm);
> }
>

The thing you are missing is that I unlocked a new personal record in

dumbassery : ) I was focussed so much on the static key logic that I

forgot to change this code from the previous revision.