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

[David Hildenbrand <david@redhat.com>]

On 13.12.21 08:27, Jianyong Wu wrote:
> 
> 
>> -----Original Message-----
>> From: Anshuman Khandual <anshuman.khandual@arm.com>
>> Sent: Monday, December 13, 2021 2:56 PM
>> To: Jianyong Wu <Jianyong.Wu@arm.com>; Catalin Marinas
>> <Catalin.Marinas@arm.com>; will@kernel.org; akpm@linux-foundation.org
>> Cc: ardb@kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; david@redhat.com; gshan@redhat.com; Justin
>> He <Justin.He@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create
>> pud mapping
>>
>>
>>
>> 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.
>>> +	 */
>>> +	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,
>>>
>>
>> As the race could only happen with memory hotplug being enabled, could
>> not we wrap this around with CONFIG_MEMORY_HOTPLUG, just to narrow
>> its scope possibly speed up other non-hotplug cases ?
> 
> I think it's better.

We better avoid using ifdef if not really necessary, it just uglifies
the code. We could add

if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
	...

But should we really try to micto-optimize this code and make it harder
to read for the purpose of an unproven performance gain? (if there is no
contention, we'll most probably not even recognize that we're taking a lock)

-- 
Thanks,

David / dhildenb


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