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

[Jianyong Wu]

Hi Mark,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@xxxxxxx>
> Sent: Friday, December 17, 2021 5:31 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> Cc: Catalin Marinas <Catalin.Marinas@xxxxxxx>; will@xxxxxxxxxx; Anshuman
> Khandual <Anshuman.Khandual@xxxxxxx>; akpm@xxxxxxxxxxxxxxxxxxxx;
> david@xxxxxxxxxx; quic_qiancai@xxxxxxxxxxx; ardb@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> gshan@xxxxxxxxxx; Justin He <Justin.He@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH v3] arm64/mm: avoid fixmap race condition when create
> pud mapping
> 
> On Thu, Dec 16, 2021 at 04:28:12PM +0800, Jianyong Wu wrote:
> > The 'fixmap' is a global resource and is used recursively by create
> > pud mapping(), leading to a potential race condition in the presence
> > of a concurrent call to alloc_init_pud():
> >
> > kernel_init thread                          virtio-mem workqueue thread
> > ==================                          ===========================
> >
> >   alloc_init_pud(...)                       alloc_init_pud(...)
> >   pudp = pud_set_fixmap_offset(...)         pudp = pud_set_fixmap_offset(...)
> >   READ_ONCE(*pudp)
> >   pud_clear_fixmap(...)
> >                                             READ_ONCE(*pudp) // CRASH!
> >
> > As kernel may sleep during creating pud mapping, introduce a mutex
> > lock to serialise use of the fixmap entries by alloc_init_pud().
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> 
> Since there were deadlock issues with the last version, it would be very nice
> if we could check this with at least:
> 
> * CONFIG_DEBUG_ATOMIC_SLEEP
> * CONFIG_PROVE_LOCKING
> 
> ... so that we can be reasonably certain that we're not introducing some
> livelock/deadlock scenario.
> 

I enable these 2 configs and test for the current patch. No warning related with this change found. 

> Are you able to reproduce the problem for testing, or was this found by
> inspection? Do you have any instructions for reproducing the problem? e.g.
> can this easily be tested with QEMU?
> 

I test it using Cloud Hypervisor not QEMU. I find the bug when I tested the incoming feature of virtio-mem using Cloud Hypervisor.
I think we can reproduce this bug using QEMU, but as there is no virtio-mem support for the current QEMU, we can only test the ACPI-based memory hotplug. However, I think it's not easy to do and I have not tried that.

In my test: firstly, start a VM and hotplug a large size of memory using virtio-mem and reboot or kexec a new kernel. When the kernel booting, memory hotplugged by virtio-mem will be added within kernel_init. As both of kernel init and memory add thread will update page table, "alloc_pud_init" may be executed concurrently. 

I think it's not easy to reproduce the bug using ACPI based memory hotplug, as we must hotplug memory at the same time of kernel_init to crash with it. 

> If you're able to reproduce the issue, it would be nice to have an example
> backtrace of when this goes wrong.
> 
Yes, this bug occurs when kernel init, the function execute flow is:
-------------------------
kernel_init
  kernel_init_freeable
    ...
      do_initcall
        ...
          module_init [A]

  ...
  mark_readonly
    mark_rodata_ro [B]
-------------------------
[A] can contains memory hotplug init therefore both [A] and [B] can
update page table at the same time and may lead to crash.

Thanks
Jianyong Wu

> Thanks,
> Mark.
> 
> > ---
> >
> > Change log:
> >
> > from v2 to v3:
> >      change spin lock to mutex lock as kernel may sleep when create
> > pud map.
> >
> >  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..e680a6a8ca40 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)  { @@ -329,6 +330,11 @@
> > static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long
> end,
> >  	}
> >  	BUG_ON(p4d_bad(p4d));
> >
> > +	/*
> > +	 * We only have one fixmap entry per page-table level, so take
> > +	 * the fixmap lock until we're done.
> > +	 */
> > +	mutex_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();
> > +	mutex_unlock(&fixmap_lock);
> >  }
> >
> >  static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
> > --
> > 2.17.1
> >