On 10/14/26 14:15, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] mm: NULL pointer dereference in zap_huge_pmd() for special PMDs on
> PowerPC
> --
>
> commit 5100f57130de5473973d1370273238b4778fae24
> Author: Konstantin Khorenko <[email protected]>
>
> mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()
>
> copy_huge_pmd() reads the source PMD without the page table lock and,
> for a special PMD, installs that value in the child without reading it
> again under the lock. This commit reads the PMD again under the lock to
> ensure it wasn't cleared concurrently by unmap_mapping_range().
>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 1e5d68acf62a5..fcea98f0d8865 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct
>> mm_struct *src_mm,
>> src_ptl = pmd_lockptr(src_mm, src_pmd);
>> spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>> /*
>> - * No need to recheck the pmd, it can't change with write
>> - * mmap lock held here.
>> - *
>> - * Meanwhile, making sure it's not a CoW VMA with writable
>> - * mapping, otherwise it means either the anon page wrongly
>> - * applied special bit, or we made the PRIVATE mapping be
>> - * able to wrongly write to the backend MMIO.
>> + * The mmap and VMA write locks of the source keep faults
>> + * out, but unmap_mapping_range() does not take the mmap
>> + * lock and may have cleared the pmd since the lockless
>> + * read. Let copy_pmd_range() skip it then.
>> + */
>> + pmd = pmdp_get(src_pmd);
>> + if (unlikely(!pmd_present(pmd) || !pmd_special(pmd))) {
>> + spin_unlock(src_ptl);
>> + spin_unlock(dst_ptl);
>> + return -EAGAIN;
>> + }
>> + /*
>> + * Make sure it's not a CoW VMA with writable mapping,
>> + * otherwise it means either the anon page wrongly applied
>> + * special bit, or we made the PRIVATE mapping be able to
>> + * wrongly write to the backend MMIO.
>> */
>> VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
>> goto set_pmd;
> [Severity: High]
> Does this code cause a NULL pointer dereference on PowerPC when a special
> PMD is unmapped by the child?
>
> Because the jump to set_pmd bypasses pgtable allocation and deposition
> for the child process's new special PMD, no pgtable is deposited.
> Later, when the child unmaps the VMA or exits, zap_pmd_range() calls
> zap_huge_pmd().
>
> On architectures like PowerPC (hash mode) where
> arch_needs_pgtable_deposit() is true, this leads to:
>
> mm/huge_memory.c:zap_huge_pmd() {
> ...
> if (has_deposit)
> zap_deposited_table(mm, pmd);
> ...
> }
>
> This will unconditionally assume a deposited table exists and try to
> withdraw it. Then in zap_deposited_table():
>
> mm/huge_memory.c:zap_deposited_table() {
> ...
> pgtable = pgtable_trans_huge_withdraw(mm, pmd);
> pte_free(mm, pgtable);
> ...
> }
>
> Since no page table was ever deposited, pgtable_trans_huge_withdraw()
> returns NULL, and passing NULL to pte_free() leads to a kernel crash.
Thanks, that is a real bug, but it is not introduced by this patch: the
special PMD branch of copy_huge_pmd() has never deposited a page table
for the child, since commit bc02afbd4d73 ("mm/fork: accept huge pfnmap
entries"). This patch only rereads the PMD under the lock in that
branch and does not change what it installs.
It is also worse than the zap: on ppc64 in hash mode the child's first
hash fault on that PMD already dereferences the missing table, in
__hash_page_thp() through get_hpte_slot_array(), and the zap then hits
the memset() in hash__pgtable_trans_huge_withdraw() before pte_free().
I will send a v2 with a separate patch on top of this one that
allocates and deposits the table when arch_needs_pgtable_deposit() is
true, as insert_pmd() does.
--
Best regards,
Konstantin Khorenko,
Virtuozzo Linux Kernel Team
>
> -- Sashiko AI review ยท
> https://sashiko.dev/#/patchset/20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-v1-1-d225c60bc...@virtuozzo.com?part=1
>
