| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Dec 2021 10:09:04 +0000
From: Jianyong Wu <Jianyong.Wu@....com>
To: Mark Rutland <Mark.Rutland@....com>
CC: Catalin Marinas <Catalin.Marinas@....com>,
"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 Mark,
> -----Original Message-----
> From: Mark Rutland <mark.rutland@....com>
> Sent: Friday, December 17, 2021 5:31 PM
> To: Jianyong Wu <Jianyong.Wu@....com>
> Cc: Catalin Marinas <Catalin.Marinas@....com>; 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, 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@....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
> >
Powered by blists - more mailing lists