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

[Ryan Roberts <ryan.roberts@arm.com>]

On 17/06/2025 04:59, Dev Jain wrote:
> 
> On 17/06/25 8:24 am, Dev Jain wrote:
>>
>> On 16/06/25 11:37 pm, Ryan Roberts wrote:
>>> On 16/06/2025 11: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@arm.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(-)
>>>>

[...]

>>>> +    pud_clear(pudp);
>>> How can this possibly be correct; you're clearing the pud without any
>>> synchronisation. So you could have this situation:
>>>
>>> CPU1 (vmalloc)            CPU2 (ptdump)
>>>
>>>                 static_branch_enable()
>>>                   mmap_write_lock()
>>>                     pud = pudp_get()
>>
>> When you do pudp_get(), you won't be dereferencing a NULL pointer.
>> pud_clear() will nullify the pud entry. So pudp_get() will boil
>> down to retrieving a NULL entry. Or, pudp_get() will retrieve an
>> entry pointing to the now isolated PMD table. Correct me if I am
>> wrong.
>>
>>> pud_free_pmd_page()
>>>    pud_clear()
>>>                     access the table pointed to by pud
>>>                     BANG!
> 
> I am also confused thoroughly now : ) This should not go bang as the
> 
> table pointed to by pud is still there, and our sequence guarantees that
> 
> if the ptdump walk is using the pmd table, then pud_free_pmd_page won't
> 
> free the PMD table yet.

You're right... I'm not sure what I was smoking last night. For some reason I
read the pXd_clear() as "free". This approach looks good to me - very clever!
And you even managed to ensure the WRITE_ONCE() in pXd_clear() doesn't get
reordered after taking the lock via the existing dsb in the tlb maintenance
operation - I like it!

I'll send a separate review with some nits, but I'm out today, so that might
have to wait until tomorrow.

Thanks, and sorry again for the noise!
Ryan