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

Jianyong Wu
2021-12-13T07:28:09Z

> -----Original Message-----
> From: Anshuman Khandual <anshuman.khandual@arm.com>
> Sent: Monday, December 13, 2021 2:56 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; Catalin Marinas
> <Catalin.Marinas@arm.com>; 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 <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create
> pud mapping
>
>
>
> On 12/10/21 3:24 PM, Jianyong Wu wrote:
> > 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 <jianyong.wu@arm.com>
> > ---
> > 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
> > __maybe_unused; static pud_t bm_pud[PTRS_PER_PUD]
> __page_aligned_bss
> > __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,
> > }
> > 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,
> > } 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