On 2025/2/11 17:37, David Hildenbrand wrote:
On 11.02.25 10:29, Qi Zheng wrote:
On 2025/2/11 17:14, David Hildenbrand wrote:
On 11.02.25 04:45, Qi Zheng wrote:
Hi Russell,
On 2025/2/11 01:03, Russell King (Oracle) wrote:
On Mon, Feb 10, 2025 at 05:49:38PM +0100, Ezra Buehler wrote:
When running vanilla Linux 6.13 or newer (6.14-rc2) on the
AT91SAM9G25-based GARDENA smart Gateway, we are seeing a NULL pointer
dereference resulting in a kernel panic. The culprit seems to be
commit
fc9c45b71f43 ("arm: adjust_pte() usepte_offset_map_rw_nolock()").
Reverting the commit apparently fixes the issue.
The blamed commit is buggy:
arch/arm/include/asm/tlbflush.h:
#define update_mmu_cache(vma, addr, ptep) \
update_mmu_cache_range(NULL, vma, addr, ptep, 1)
So vmf can be NULL. This didn't used to matter before this commit,
because vmf was not used by ARM's update_mmu_cache_range(). However,
the commit introduced a dereference of it, which now causes a NULL
point dereference.
Not sure what the correct solution is, but at a guess, both:
if (ptl != vmf->ptl)
need to become:
if (!vmf || ptl != vmf->ptl)
No, we can't do that, because without using split PTE locks, we would
use shared mm->page_table_lock, which would create a deadlock.
Maybe we can simply special-case on CONFIG_SPLIT_PTE_PTLOCKS ?
if (IS_ENABLED(CONFIG_SPLIT_PTE_PTLOCKS)) {
In this case, if two vmas map the same PTE page, then the same PTE lock
will be held repeatedly. Right?
Hmm, the comment says:
/*
* This is called while another page table is mapped, so we
* must use the nested version. This also means we need to
* open-code the spin-locking.
*/
"another page table" implies that it cannot be the same. But maybe that
comment was also wrong?
I don't see make_coherent() ensuring this when traversing vma. I
therefore propose the following changes:
diff --git a/arch/arm/mm/fault-armv.c b/arch/arm/mm/fault-armv.c
index 2bec87c3327d2..dddbca9a2597e 100644
--- a/arch/arm/mm/fault-armv.c
+++ b/arch/arm/mm/fault-armv.c
@@ -61,8 +61,41 @@ static int do_adjust_pte(struct vm_area_struct *vma,
unsigned long address,
return ret;
}
+#if defined(CONFIG_SPLIT_PTE_PTLOCKS)
+/*
+ * If we are using split PTE locks, then we need to take the pte
+ * lock here. Otherwise we are using shared mm->page_table_lock
+ * which is already locked, thus cannot take it.
+ */
+static inline bool do_pte_lock(spinlock_t *ptl, pmd_t pmdval, pmd_t *pmd)
+{
+ /*
+ * 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)))) {
+ spin_unlock(ptl);
+ return false;
+ }
+
+ return true;
+}
+
+static inline void do_pte_unlock(spinlock_t *ptl)
+{
+ spin_unlock(ptl);
+}
+#else /* !defined(CONFIG_SPLIT_PTE_PTLOCKS) */
+static inline bool do_pte_lock(spinlock_t *ptl)
+{
+ return true;
+}
+static inline void do_pte_unlock(spinlock_t *ptl) {}
+#endif /* defined(CONFIG_SPLIT_PTE_PTLOCKS) */
+
static int adjust_pte(struct vm_area_struct *vma, unsigned long address,
- unsigned long pfn, struct vm_fault *vmf)
+ unsigned long pfn)
{
spinlock_t *ptl;
pgd_t *pgd;
@@ -99,23 +132,14 @@ 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) {
- spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
- if (unlikely(!pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
- pte_unmap_unlock(pte, ptl);
- goto again;
- }
+ if (!do_pte_lock(ptl, pmdval, pmd)) {
+ pte_unmap(pte);
+ goto again;
}
ret = do_adjust_pte(vma, address, pfn, pte);
- if (ptl != vmf->ptl)
- spin_unlock(ptl);
+ do_pte_unlock(ptl);
pte_unmap(pte);
return ret;
@@ -123,16 +147,17 @@ 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 start;
pgoff_t pgoff;
int aliases = 0;
pgoff = vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT);
+ start = ALIGN_DOWN(addr, PMD_SIZE);
/*
* If we have any shared mappings that are in the same mm
@@ -141,6 +166,8 @@ 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) {
+ 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 +178,14 @@ 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;
+ /*
+ * If mpnt_addr and addr are mapped to the same PTE page,
+ * also skip this vma.
+ */
+ if (mpnt_addr >= start && mpnt_addr - start < PMD_SIZE)
+ continue;
+ aliases += adjust_pte(mpnt, mpnt_addr, pfn);
}
flush_dcache_mmap_unlock(mapping);
if (aliases)
@@ -194,7 +228,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();
}
Make sense?
