MIME-Version: 1.0
References: <20211210095432.51798-1-jianyong.wu@arm.com> <1e63e2f7-0563-2866-4665-84fe220b615f@arm.com>
In-Reply-To: <1e63e2f7-0563-2866-4665-84fe220b615f@arm.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Mon, 13 Dec 2021 11:35:16 +0100
Message-ID: <CAMj1kXF3Kf_OP-EL6oRfstw6VdEaqowVPioABxfMv+1FnPvfew@mail.gmail.com>
Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create pud mapping
To:     Anshuman Khandual <anshuman.khandual@arm.com>
Cc:     Jianyong Wu <jianyong.wu@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        David Hildenbrand <david@redhat.com>,
        Gavin Shan <gshan@redhat.com>, Jia He <justin.he@arm.com>,
        nd <nd@arm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 13 Dec 2021 at 11:16, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
>
> 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.
> > +      */
>
> Small nit, format and align this comment block. I guess
> could also be done while merging this patch as well.
>
> > +     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,
> >
>
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

We have different fixmap slots for different page table levels, so
'fixmap_lock' is not the right name.

But we already have swapper_pgdir_lock as well, which serializes the
use of the pgdir level fixmap slot. And we have no spinlocks
protecting the other levels.

So should we perhaps clean this up more comprehensively? Wouldn't it
be better to add a mutex to __create_pgd_mapping(), for instance?