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

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

[Jianyong Wu]

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,
-- 
2.17.1

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

[Catalin Marinas]

On Fri, Dec 10, 2021 at 05:54:32PM +0800, 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>

It looks fine to me:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Do we need a cc stable? Fixmap was introduced in 4.6, so usually:

Fixes: f4710445458c ("arm64: mm: use fixmap when creating page tables")
Cc: <stable@vger.kernel.org> # 4.6.x

but I haven't checked when memory hotplug was added to be able to
trigger the race. It may not need to go back that far.

-- 
Catalin

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

[Jianyong Wu]

Hi Catalin,

> -----Original Message-----
> From: Catalin Marinas <catalin.marinas@arm.com>
> Sent: Friday, December 10, 2021 7:22 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: will@kernel.org; Anshuman Khandual <Anshuman.Khandual@arm.com>;
> akpm@linux-foundation.org; 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>; Mark Rutland <Mark.Rutland@arm.com>
> Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create
> pud mapping
> 
> On Fri, Dec 10, 2021 at 05:54:32PM +0800, 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>
> 
> It looks fine to me:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Do we need a cc stable? Fixmap was introduced in 4.6, so usually:
> 
> Fixes: f4710445458c ("arm64: mm: use fixmap when creating page tables")
> Cc: <stable@vger.kernel.org> # 4.6.x
> 

> but I haven't checked when memory hotplug was added to be able to trigger
> the race. It may not need to go back that far.
>
For now, I only find the memory hotplug can conflict with it. I see that memory hotplug is enabled on arm64 in 4.20, thus what about changing "4.6" to "4.20".
 
Thanks
Jianyong
> --
> Catalin

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

[Anshuman Khandual]

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 ?

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

[Jianyong Wu]

> -----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.

Thanks
Jianyong

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

[David Hildenbrand]

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

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

[Catalin Marinas]

On Mon, Dec 13, 2021 at 08:37:04AM +0100, David Hildenbrand wrote:
> On 13.12.21 08:27, Jianyong Wu wrote:
> >>> @@ -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 ?

Assuming you can quantify the speed-up...

> > 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)

I agree, I don't see the point of this optimisation.

-- 
Catalin

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

[Anshuman Khandual]

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>

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

[Ard Biesheuvel]

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?

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

[Will Deacon]

On Mon, Dec 13, 2021 at 11:35:16AM +0100, Ard Biesheuvel wrote:
> 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:
> > > 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?

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?

Will

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

[Ard Biesheuvel]

On Mon, 13 Dec 2021 at 14:45, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 13, 2021 at 11:35:16AM +0100, Ard Biesheuvel wrote:
> > 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:
> > > > 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?
>
> 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.

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

[Will Deacon]

On Fri, 10 Dec 2021 17:54:32 +0800, 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.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64/mm: avoid fixmap race condition when create pud mapping
      https://git.kernel.org/arm64/c/a6a6b9aa34e5

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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

[Qian Cai]

On Fri, Dec 10, 2021 at 05:54:32PM +0800, 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>

I am afraid there is a problem to take a spinlock there.

node 0 deferred pages initialised in 2740ms
 pgdatinit0 (176) used greatest stack depth: 59184 bytes left
 devtmpfs: initialized
 KASLR disabled due to lack of seed
 BUG: sleeping function called from invalid context at mm/page_alloc.c:5151
 in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
 preempt_count: 1, expected: 0
 1 lock held by swapper/0/1:
  #0: ffff800009ea3278 (fixmap_lock){+.+.}-{2:2}, at: __create_pgd_mapping
  alloc_init_pud at /usr/src/linux-next/arch/arm64/mm/mmu.c:340 (discriminator 4)
  (inlined by) __create_pgd_mapping at /usr/src/linux-next/arch/arm64/mm/mmu.c:393 (discriminator 4)
 CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W         5.16.0-rc5-next-20211214
 Call trace:
  dump_backtrace
  show_stack
  dump_stack_lvl
  dump_stack
  __might_resched
  __might_sleep
  __alloc_pages
  alloc_page_interleave
  alloc_pages
  __get_free_pages
  __pgd_pgtable_alloc
  __create_pgd_mapping
  __phys_to_pte_val at /usr/src/linux-next/./arch/arm64/include/asm/pgtable.h:77
  (inlined by) __pud_populate at /usr/src/linux-next/./arch/arm64/include/asm/pgalloc.h:25
  (inlined by) alloc_init_cont_pmd at /usr/src/linux-next/arch/arm64/mm/mmu.c:277
  (inlined by) alloc_init_pud at /usr/src/linux-next/arch/arm64/mm/mmu.c:358
  (inlined by) __create_pgd_mapping at /usr/src/linux-next/arch/arm64/mm/mmu.c:393
  map_entry_trampoline
  map_entry_trampoline at /usr/src/linux-next/arch/arm64/mm/mmu.c:639
  do_one_initcall
  kernel_init_freeable
  kernel_init
  ret_from_fork

> ---
>  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,
> -- 
> 2.17.1
> 

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

[Catalin Marinas]

On Wed, Dec 15, 2021 at 09:13:37AM -0500, Qian Cai wrote:
> On Fri, Dec 10, 2021 at 05:54:32PM +0800, 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>
> 
> I am afraid there is a problem to take a spinlock there.
> 
> node 0 deferred pages initialised in 2740ms
>  pgdatinit0 (176) used greatest stack depth: 59184 bytes left
>  devtmpfs: initialized
>  KASLR disabled due to lack of seed
>  BUG: sleeping function called from invalid context at mm/page_alloc.c:5151

Thanks for the report. Definitely a bug, GFP_KERNEL_ACCOUNT allows for
the page allocator to sleep. I'll drop the patch from the arm64
for-next/fixes branch for now and sort it out later.

-- 
Catalin

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

[David Hildenbrand]

On 15.12.21 17:02, Catalin Marinas wrote:
> On Wed, Dec 15, 2021 at 09:13:37AM -0500, Qian Cai wrote:
>> On Fri, Dec 10, 2021 at 05:54:32PM +0800, 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>
>>
>> I am afraid there is a problem to take a spinlock there.
>>
>> node 0 deferred pages initialised in 2740ms
>>  pgdatinit0 (176) used greatest stack depth: 59184 bytes left
>>  devtmpfs: initialized
>>  KASLR disabled due to lack of seed
>>  BUG: sleeping function called from invalid context at mm/page_alloc.c:5151
> 
> Thanks for the report. Definitely a bug, GFP_KERNEL_ACCOUNT allows for
> the page allocator to sleep. I'll drop the patch from the arm64
> for-next/fixes branch for now and sort it out later.
> 

Sounds like we need a mutex then.

-- 
Thanks,

David / dhildenb

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

[Jianyong Wu]

Thanks Qian to report this.
I will change to mutex lock and resend it later.

Thanks
Jianyong

> -----Original Message-----
> From: David Hildenbrand <david@redhat.com>
> Sent: Thursday, December 16, 2021 12:04 AM
> To: Catalin Marinas <Catalin.Marinas@arm.com>; Qian Cai
> <quic_qiancai@quicinc.com>
> Cc: Jianyong Wu <Jianyong.Wu@arm.com>; will@kernel.org; Anshuman
> Khandual <Anshuman.Khandual@arm.com>; akpm@linux-foundation.org;
> 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 v2] arm64/mm: avoid fixmap race condition when create
> pud mapping
> 
> On 15.12.21 17:02, Catalin Marinas wrote:
> > On Wed, Dec 15, 2021 at 09:13:37AM -0500, Qian Cai wrote:
> >> On Fri, Dec 10, 2021 at 05:54:32PM +0800, 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>
> >>
> >> I am afraid there is a problem to take a spinlock there.
> >>
> >> node 0 deferred pages initialised in 2740ms
> >>  pgdatinit0 (176) used greatest stack depth: 59184 bytes left
> >>  devtmpfs: initialized
> >>  KASLR disabled due to lack of seed
> >>  BUG: sleeping function called from invalid context at
> >> mm/page_alloc.c:5151
> >
> > Thanks for the report. Definitely a bug, GFP_KERNEL_ACCOUNT allows for
> > the page allocator to sleep. I'll drop the patch from the arm64
> > for-next/fixes branch for now and sort it out later.
> >
> 
> Sounds like we need a mutex then.
> 
> --
> Thanks,
> 
> David / dhildenb