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

On Mon, 13 Dec 2021 at 14:45, Will Deacon [off-list ref] wrote:

On Mon, Dec 13, 2021 at 11:35:16AM +0100, Ard Biesheuvel wrote:

quoted

On Mon, 13 Dec 2021 at 11:16, Anshuman Khandual
[off-list ref] wrote:

quoted

On 12/10/21 3:24 PM, Jianyong Wu wrote:

quoted

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.

quoted

+     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 <redacted>

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?

That does sound like a better way to do things, but the simplicity of this
patch is quite attractive for backporting. Would you object to me queuing
it as-is, on the premise that I'm more than happy to take consolidation
changes on top?

No objections from me.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel