From: David Hildenbrand <david@redhat.com>
To: Qi Zheng <zhengqi.arch@bytedance.com>,
linux@armlinux.org.uk, ezra@easyb.ch, hughd@google.com,
ryan.roberts@arm.com, akpm@linux-foundation.org,
muchun.song@linux.dev
Cc: linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org,
Ezra Buehler <ezra.buehler@husqvarnagroup.com>,
stable@vger.kernel.org
Subject: Re: [PATCH v2] arm: pgtable: fix NULL pointer dereference issue
Date: Fri, 14 Feb 2025 09:11:54 +0100 [thread overview]
Message-ID: <f15bd993-20de-41ae-8631-9ce557cd9d20@redhat.com> (raw)
In-Reply-To: <20250214030349.45524-1-zhengqi.arch@bytedance.com>
On 14.02.25 04:03, Qi Zheng wrote:
> When update_mmu_cache_range() is called by update_mmu_cache(), the vmf
> parameter is NULL, which will cause a NULL pointer dereference issue in
> adjust_pte():
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000030 when read
> Hardware name: Atmel AT91SAM9
> PC is at update_mmu_cache_range+0x1e0/0x278
> LR is at pte_offset_map_rw_nolock+0x18/0x2c
> Call trace:
> update_mmu_cache_range from remove_migration_pte+0x29c/0x2ec
> remove_migration_pte from rmap_walk_file+0xcc/0x130
> rmap_walk_file from remove_migration_ptes+0x90/0xa4
> remove_migration_ptes from migrate_pages_batch+0x6d4/0x858
> migrate_pages_batch from migrate_pages+0x188/0x488
> migrate_pages from compact_zone+0x56c/0x954
> compact_zone from compact_node+0x90/0xf0
> compact_node from kcompactd+0x1d4/0x204
> kcompactd from kthread+0x120/0x12c
> kthread from ret_from_fork+0x14/0x38
> Exception stack(0xc0d8bfb0 to 0xc0d8bff8)
>
> To fix it, do not rely on whether 'ptl' is equal to decide whether to hold
> the pte lock, but decide it by whether CONFIG_SPLIT_PTE_PTLOCKS is
> enabled. In addition, if two vmas map to the same PTE page, there is no
> need to hold the pte lock again, otherwise a deadlock will occur. Just add
> the need_lock parameter to let adjust_pte() know this information.
>
> Reported-by: Ezra Buehler <ezra.buehler@husqvarnagroup.com>
> Closes: https://lore.kernel.org/lkml/CAM1KZSmZ2T_riHvay+7cKEFxoPgeVpHkVFTzVVEQ1BO0cLkHEQ@mail.gmail.com/
> Fixes: fc9c45b71f43 ("arm: adjust_pte() use pte_offset_map_rw_nolock()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> ---
> Changes in v2:
> - change Ezra's email address (Ezra Buehler)
> - some cleanups (David Hildenbrand)
>
> arch/arm/mm/fault-armv.c | 38 ++++++++++++++++++++++++++------------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mm/fault-armv.c b/arch/arm/mm/fault-armv.c
> index 2bec87c3327d2..ea4c4e15f0d31 100644
> --- a/arch/arm/mm/fault-armv.c
> +++ b/arch/arm/mm/fault-armv.c
> @@ -62,7 +62,7 @@ static int do_adjust_pte(struct vm_area_struct *vma, unsigned long address,
> }
>
> static int adjust_pte(struct vm_area_struct *vma, unsigned long address,
> - unsigned long pfn, struct vm_fault *vmf)
> + unsigned long pfn, bool need_lock)
> {
> spinlock_t *ptl;
> pgd_t *pgd;
> @@ -99,12 +99,11 @@ static int adjust_pte(struct vm_area_struct *vma, unsigned long address,
> if (!pte)
> return 0;
>
> - /*
> - * If we are using split PTE locks, then we need to take the page
> - * lock here. Otherwise we are using shared mm->page_table_lock
> - * which is already locked, thus cannot take it.
> - */
> - if (ptl != vmf->ptl) {
> + if (need_lock) {
> + /*
> + * Use nested version here to indicate that we are already
> + * holding one similar spinlock.
> + */
> spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
> if (unlikely(!pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
> pte_unmap_unlock(pte, ptl);
> @@ -114,7 +113,7 @@ static int adjust_pte(struct vm_area_struct *vma, unsigned long address,
>
> ret = do_adjust_pte(vma, address, pfn, pte);
>
> - if (ptl != vmf->ptl)
> + if (need_lock)
> spin_unlock(ptl);
> pte_unmap(pte);
>
> @@ -123,16 +122,18 @@ static int adjust_pte(struct vm_area_struct *vma, unsigned long address,
>
> static void
> make_coherent(struct address_space *mapping, struct vm_area_struct *vma,
> - unsigned long addr, pte_t *ptep, unsigned long pfn,
> - struct vm_fault *vmf)
> + unsigned long addr, pte_t *ptep, unsigned long pfn)
> {
> struct mm_struct *mm = vma->vm_mm;
> struct vm_area_struct *mpnt;
> unsigned long offset;
> + unsigned long pmd_start_addr, pmd_end_addr;
Nit: reverse christmas tree would make us put this line at the very top.
Maybe do the initialization directly:
const unsigned long pmd_start_addr = ALIGN_DOWN(addr, PMD_SIZE);
const unsigned long pmd_end_addr = pmd_start_addr + PMD_SIZE;
> pgoff_t pgoff;
> int aliases = 0;
>
> pgoff = vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT);
> + pmd_start_addr = ALIGN_DOWN(addr, PMD_SIZE);
> + pmd_end_addr = pmd_start_addr + PMD_SIZE;
>
> /*
> * If we have any shared mappings that are in the same mm
> @@ -141,6 +142,14 @@ make_coherent(struct address_space *mapping, struct vm_area_struct *vma,
> */
> flush_dcache_mmap_lock(mapping);
> vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
> + /*
> + * If we are using split PTE locks, then we need to take the pte
> + * lock. Otherwise we are using shared mm->page_table_lock which
> + * is already locked, thus cannot take it.
> + */
> + bool need_lock = IS_ENABLED(CONFIG_SPLIT_PTE_PTLOCKS);
> + unsigned long mpnt_addr;
> +
> /*
> * If this VMA is not in our MM, we can ignore it.
> * Note that we intentionally mask out the VMA
> @@ -151,7 +160,12 @@ make_coherent(struct address_space *mapping, struct vm_area_struct *vma,
> if (!(mpnt->vm_flags & VM_MAYSHARE))
> continue;
> offset = (pgoff - mpnt->vm_pgoff) << PAGE_SHIFT;
> - aliases += adjust_pte(mpnt, mpnt->vm_start + offset, pfn, vmf);
> + mpnt_addr = mpnt->vm_start + offset;
> +
> + /* Avoid deadlocks by not grabbing the same PTE lock again. */
> + if (mpnt_addr >= pmd_start_addr && mpnt_addr < pmd_end_addr)
> + need_lock = false;
> + aliases += adjust_pte(mpnt, mpnt_addr, pfn, need_lock);
> }
> flush_dcache_mmap_unlock(mapping);
> if (aliases)
> @@ -194,7 +208,7 @@ void update_mmu_cache_range(struct vm_fault *vmf, struct vm_area_struct *vma,
> __flush_dcache_folio(mapping, folio);
> if (mapping) {
> if (cache_is_vivt())
> - make_coherent(mapping, vma, addr, ptep, pfn, vmf);
> + make_coherent(mapping, vma, addr, ptep, pfn);
> else if (vma->vm_flags & VM_EXEC)
> __flush_icache_all();
> }
Apart from that LGTM. Hoping it will work :)
Acked-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
next prev parent reply other threads:[~2025-02-14 8:12 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-14 3:03 [PATCH v2] arm: pgtable: fix NULL pointer dereference issue Qi Zheng
2025-02-14 8:11 ` David Hildenbrand [this message]
2025-02-14 8:31 ` Qi Zheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f15bd993-20de-41ae-8631-9ce557cd9d20@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=ezra.buehler@husqvarnagroup.com \
--cc=ezra@easyb.ch \
--cc=hughd@google.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux@armlinux.org.uk \
--cc=muchun.song@linux.dev \
--cc=ryan.roberts@arm.com \
--cc=stable@vger.kernel.org \
--cc=zhengqi.arch@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.