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

[Dev Jain <dev.jain@arm.com>]

On 17/06/25 1:42 pm, Ryan Roberts wrote:
> 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!

Haha! It indeed was very confusing, the important observation separating this
from other cases was that ptdump only cares about reading the tables, not about
what it reads.

>
> 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!

Ah no it was not noise : ) Sure, enjoy.

> Ryan
>