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 <redacted>
---
arch/arm64/mm/mmu.c | 7 +++++++
1 file changed, 7 insertions(+)
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 <redacted>
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: <redacted> # 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-----Original Message-----
From: Catalin Marinas <catalin.marinas@arm.com>
Sent: Friday, December 10, 2021 7:22 PM
To: Jianyong Wu <redacted>
Cc: will@kernel.org; Anshuman Khandual <redacted>;
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 [off-list ref];
nd [off-list ref]; Mark Rutland [off-list ref]
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:
quoted
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 <redacted>
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: <redacted> # 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
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 <redacted>
---
arch/arm64/mm/mmu.c | 7 +++++++
1 file changed, 7 insertions(+)
@@ -329,6 +330,11 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end,}BUG_ON(p4d_bad(p4d));+/*+*fixmapisglobalresource,thusitneedstobeprotectedbyalock+*incaseofracecondition.+*/+spin_lock(&fixmap_lock);pudp=pud_set_fixmap_offset(p4dp,addr);do{pud_told_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);}staticvoid__create_pgd_mapping(pgd_t*pgdir,phys_addr_tphys,
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 ?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
---
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
}
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);
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 ?
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 <redacted>
---
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
}
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);
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
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...
quoted
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)
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 <redacted>
---
arch/arm64/mm/mmu.c | 7 +++++++
1 file changed, 7 insertions(+)
On Mon, 13 Dec 2021 at 11:16, Anshuman Khandual
[off-list ref] wrote:
On 12/10/21 3:24 PM, Jianyong Wu wrote:
quoted
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 <redacted>
---
arch/arm64/mm/mmu.c | 7 +++++++
1 file changed, 7 insertions(+)
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?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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?
From: Will Deacon <will@kernel.org> Date: 2021-12-13 16:42:57
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.
[...]
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 <redacted>
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
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:
quoted
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 <redacted>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: David Hildenbrand <hidden> Date: 2021-12-15 16:04:31
On 15.12.21 17:02, Catalin Marinas wrote:
On Wed, Dec 15, 2021 at 09:13:37AM -0500, Qian Cai wrote:
quoted
On Fri, Dec 10, 2021 at 05:54:32PM +0800, Jianyong Wu wrote:
quoted
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 <redacted>
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.
Thanks Qian to report this.
I will change to mutex lock and resend it later.
Thanks
Jianyong
-----Original Message-----
From: David Hildenbrand <redacted>
Sent: Thursday, December 16, 2021 12:04 AM
To: Catalin Marinas <Catalin.Marinas@arm.com>; Qian Cai
[off-list ref]
Cc: Jianyong Wu <redacted>; will@kernel.org; Anshuman
Khandual [off-list ref]; akpm@linux-foundation.org;
ardb@kernel.org; linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; gshan@redhat.com; Justin He
[off-list ref]; nd [off-list ref]
Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create
pud mapping
On 15.12.21 17:02, Catalin Marinas wrote:
quoted
On Wed, Dec 15, 2021 at 09:13:37AM -0500, Qian Cai wrote:
quoted
On Fri, Dec 10, 2021 at 05:54:32PM +0800, Jianyong Wu wrote:
quoted
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 <redacted>
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