Messages in this thread Patch in this message | | | From | Jianyong Wu <> | Subject | RE: [PATCH v3] arm64/mm: avoid fixmap race condition when create pud mapping | Date | Fri, 17 Dec 2021 10:09:04 +0000 |
| |
Hi Mark,
> -----Original Message----- > From: Mark Rutland <mark.rutland@arm.com> > Sent: Friday, December 17, 2021 5:31 PM > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: Catalin Marinas <Catalin.Marinas@arm.com>; will@kernel.org; Anshuman > Khandual <Anshuman.Khandual@arm.com>; akpm@linux-foundation.org; > david@redhat.com; quic_qiancai@quicinc.com; ardb@kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > gshan@redhat.com; Justin He <Justin.He@arm.com>; nd <nd@arm.com> > 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@arm.com> > > 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 > >
| |