RE: [PATCH v2] arm64/mm: avoid fixmap race condition when create pud mapping

-----Original Message-----
From: Anshuman Khandual <redacted>
Sent: Monday, December 13, 2021 2:56 PM
To: Jianyong Wu <redacted>; Catalin Marinas
[off-list ref]; will@kernel.org; akpm@linux-foundation.org
Cc: ardb@kernel.org; linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; david@redhat.com; gshan@redhat.com; Justin
He [off-list ref]; nd [off-list ref]
Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create
pud mapping



On 12/10/21 3:24 PM, Jianyong Wu wrote:

quoted

fixmap is a global resource and is used recursively in create pud mapping.
It may lead to race condition when alloc_init_pud is called concurrently.

Fox example:
alloc_init_pud is called when kernel_init. If memory hotplug thread,
which will also call alloc_init_pud, happens during kernel_init, the
race for fixmap occurs.

The race condition flow can be:

*************** begin **************

kerenl_init thread                          virtio-mem workqueue thread
==================                          ======== ==================
alloc_init_pud(...)
  pudp = pud_set_fixmap_offset(..)          alloc_init_pud(...)
...                                         ...
    READ_ONCE(*pudp) //OK!                    pudp = pud_set_fixmap_offset(
...                                         ...
  pud_clear_fixmap() //fixmap break
                                              READ_ONCE(*pudp) //CRASH!

**************** end ***************

Hence, a spin lock is introduced to protect the fixmap during create
pdg mapping.

Signed-off-by: Jianyong Wu <redacted>
---
 arch/arm64/mm/mmu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index

acfae9b41cc8..98ac09ae9588 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

quoted

__maybe_unused;  static pud_t bm_pud[PTRS_PER_PUD]

__page_aligned_bss

quoted

__maybe_unused;

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

 void set_swapper_pgd(pgd_t *pgdp, pgd_t pgd)  { @@ -329,6 +330,11 @@
static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long

end,

quoted

 	}
 	BUG_ON(p4d_bad(p4d));

+	/*
+	 * fixmap is global resource, thus it needs to be protected by a lock
+	 * in case of race condition.
+	 */
+	spin_lock(&fixmap_lock);
 	pudp = pud_set_fixmap_offset(p4dp, addr);
 	do {
 		pud_t old_pud = READ_ONCE(*pudp);

@@ -359,6 +365,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned

long addr, unsigned long end,

quoted

 	} while (pudp++, addr = next, addr != end);

 	pud_clear_fixmap();
+	spin_unlock(&fixmap_lock);
 }

 static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,

As the race could only happen with memory hotplug being enabled, could
not we wrap this around with CONFIG_MEMORY_HOTPLUG, just to narrow
its scope possibly speed up other non-hotplug cases ?

I think it's better.

Thanks
Jianyong
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel