Message-ID: <AM9PR08MB7276B412F02CA0431E30E06CF44D9@AM9PR08MB7276.eurprd08.prod.outlook.com>
Date:   Fri, 7 Jan 2022 09:10:57 +0000
From:   Jianyong Wu <Jianyong.Wu@....com>
To:     Catalin Marinas <Catalin.Marinas@....com>
CC:     "will@...nel.org" <will@...nel.org>,
        Anshuman Khandual <Anshuman.Khandual@....com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "david@...hat.com" <david@...hat.com>,
        "quic_qiancai@...cinc.com" <quic_qiancai@...cinc.com>,
        "ardb@...nel.org" <ardb@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "gshan@...hat.com" <gshan@...hat.com>,
        Justin He <Justin.He@....com>, nd <nd@....com>
Subject: RE: [PATCH v3] arm64/mm: avoid fixmap race condition when create pud
 mapping

Hi Catalin,

I roughly find the root cause.
 alloc_init_pud will be called at the very beginning of kernel boot in create_mapping_noalloc where no memory allocator is initialized. But lockdep check may need allocate memory. So, kernel take exception when acquire lock.(I have not found the exact code that cause this issue) that's say we may not be able to use a lock so early.

I come up with 2 methods to address it. 
1) skip dead lock check at the very beginning of kernel boot in lockdep code.
2) provided 2 two versions of __create_pgd_mapping, one with lock in it and the other without. There may be no possible of race for memory mapping at the very beginning time of kernel boot, thus we can use the no lock version of __create_pgd_mapping safely.
In my test, this issue is gone if there is no lock held in create_mapping_noalloc. I think create_mapping_noalloc is called early enough to avoid the race conditions of memory mapping, however, I have not proved it.

For now, I prefer 2).
The rough change for method 2:
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index acfae9b41cc8..3d3c910f446b 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -63,6 +63,7 @@ static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused;
 static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused;

 static DEFINE_SPINLOCK(swapper_pgdir_lock);
+static DEFINE_MUTEX(fixmap_lock);

 void set_swapper_pgd(pgd_t *pgdp, pgd_t pgd)
 {
@@ -381,6 +382,41 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
        addr = virt & PAGE_MASK;
        end = PAGE_ALIGN(virt + size);

+       do {
+               next = pgd_addr_end(addr, end);
+               /*
+                * fixmap is used inside of alloc_init_pud, but we only have
+                * one fixmap entry per page-table level, so take the fixmap
+                * lock until we're done.
+                */
+               mutex_lock(&fixmap_lock);
+               alloc_init_pud(pgdp, addr, next, phys, prot, pgtable_alloc,
+                              flags);
+               mutex_unlock(&fixmap_lock);
+               phys += next - addr;
+       } while (pgdp++, addr = next, addr != end);
+}
+
+static void __create_pgd_mapping_nolock(pgd_t *pgdir, phys_addr_t phys,
+                                unsigned long virt, phys_addr_t size,
+                                pgprot_t prot,
+                                phys_addr_t (*pgtable_alloc)(int),
+                                int flags)
+{
+       unsigned long addr, end, next;
+       pgd_t *pgdp = pgd_offset_pgd(pgdir, virt);
+
+       /*
+        * If the virtual and physical address don't have the same offset
+        * within a page, we cannot map the region as the caller expects.
+        */
+       if (WARN_ON((phys ^ virt) & ~PAGE_MASK))
+               return;
+
+       phys &= PAGE_MASK;
+       addr = virt & PAGE_MASK;
+       end = PAGE_ALIGN(virt + size);
+
        do {
                next = pgd_addr_end(addr, end);
                alloc_init_pud(pgdp, addr, next, phys, prot, pgtable_alloc,
@@ -432,7 +468,10 @@ static void __init create_mapping_noalloc(phys_addr_t phys, unsigned long virt,
                        &phys, virt);
                return;
        }
-       __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot, NULL,
+       /*
+        * We have no need to hold a lock at this very beginning.
+        */
+       __create_pgd_mapping_nolock(init_mm.pgd, phys, virt, size, prot, NULL,
                             NO_CONT_MAPPINGS);
 }

WDYT?

Thanks
Jianyong

> -----Original Message-----
> From: Catalin Marinas <catalin.marinas@....com>
> Sent: Thursday, January 6, 2022 11:57 PM
> To: Jianyong Wu <Jianyong.Wu@....com>
> Cc: will@...nel.org; Anshuman Khandual <Anshuman.Khandual@....com>;
> akpm@...ux-foundation.org; david@...hat.com; quic_qiancai@...cinc.com;
> ardb@...nel.org; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; gshan@...hat.com; Justin He
> <Justin.He@....com>; nd <nd@....com>
> Subject: Re: [PATCH v3] arm64/mm: avoid fixmap race condition when create
> pud mapping
> 
> On Thu, Jan 06, 2022 at 10:13:06AM +0000, Jianyong Wu wrote:
> > I test this patch in your way using both EDK2 V2.6 and EDK2 v2.7. it's
> > peculiar that this issue shows up on v2.6 but not on v2.7.
> > For now, I only find that if "CONFIG_DEBUG_LOCK_ALLOC" is enabled, the
> > kernel boot will hang. However, I can't debug it by printk as this
> > issue happens before pl11 is ready.
> 
> I tried earlycon but that doesn't help either.
> 
> > I will go on debugging, but very appreciated if someone can give some
> > hints on it.
> 
> FWIW, passing "nokaslr" on the kernel command line makes it boot (and this
> makes debugging harder). That's as far as I've gone.
> 
> --
> Catalin

Powered by blists - more mailing lists