mm: pass nid to reserve_bootmem_region()

PATCH Applied Gutierrez Cantu, Bernardo 5 participants 8 messages Apr 25, 2025 – Apr 28, 2025

Subsystems (3)

Memory management MIPS memblock and memory management initialization

lore

Share & tools

lore

Gutierrez Cantu, Bernardo

author

2025-04-25 10:02 UTC #0 lore raw

Review

Quoted 4 lines

(quoted diff) +4 -0

+		if (memblock_is_nomap(region))
+			reserve_bootmem_region(start, end, nid);
+
+		memblock_set_node(start, end, &memblock.reserved, nid);

memblock_set_node() receives a base and a size argument. Passing end would cause us to set the node id on an incorrect range. Will send a fixup patch shortly...

Best regards
Bernardo

Amazon Web Services Development Center Germany GmbH Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

Bernardo C. Gutierrez Cantu

author

2025-04-25 10:20 UTC #1 lore raw

PATCH 1/1 mm: memblock: Fix arguments passed to memblock_set_node()

memblock_set_node() receives a base and a size arguments, but we are passing the start and end of the memory regions when iterating over them in memmap_init_reserved_pages() to set their node ids.

This results in the function setting the node ids for the reserved memory regions in [base, base + base + size) instead of [base, base + size).

Pass start and size, so that we iterate over the correct range.

Fixes 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")

Signed-off-by Bernardo C. Gutierrez Cantu <[email protected]>

---
 mm/memblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memblock.c +1 -1

index 0a53db4d9f7b..9639f04b4fdf 100644
@@ -2196,7 +2196,7 @@ static void __init memmap_init_reserved_pages(void)
 		if (memblock_is_nomap(region))
 			reserve_bootmem_region(start, end, nid);
 
-		memblock_set_node(start, end, &memblock.reserved, nid);
+		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
 
 	/*

Signature · 12 lines

2.47.1




Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

David Woodhouse

Acked-by on v1 1/1

2025-04-25 14:18 UTC #2 lore raw

Re: [PATCH] mm: memblock: Fix arguments passed to memblock_set_node()

Quoted 4 lines · Bernardo C. Gutierrez Cantu

memblock_set_node() receives a base and a size arguments, but we are passing the start and end of the memory regions when iterating over them in memmap_init_reserved_pages() to set their node ids.

Quoted 8 lines

This results in the function setting the node ids for the reserved memory regions in [base, base + base + size) instead of [base, base + size).
Pass start and size, so that we iterate over the correct range.
Fixes 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
Signed-off-by Bernardo C. Gutierrez Cantu <[email protected]>

Thanks, Bernardo. Good catch!

Acked-by David Woodhouse <[email protected]>

That function taking (start, size) arguments seems a little surprising to me; I instinctively expect physical memory management functions to use (start, end). Clearly the author of that commit did too :)

As you pointed out in our private chat though, the rest of the memblock_* functions are all (start, size) too, so they are at least consistent. I don't think it makes a lot of sense to talk about changing them *all* at this point?

I did a quick grep for memblock_set_node() callers, and the one in szmem() in arch/mips/loongson64/init.c looks odd.

       /* set nid for reserved memory */
       memblock_set_node((u64)node << 44, (u64)(node + 1) << 44,
                       &memblock.reserved, node);

At first glance I suspect the 'size' should just be (1<<44) or maybe it should be inside the loop over the memmap, and called with mem_start, mem_size each time?

And why are we calling memblock_reserve() for what appears to be a single system-wide vgabios_addr, repeatedly each time szmem() is called for a different NUMA node?

Jiaxun Yang

2025-04-25 14:38 UTC #3 lore raw

Re: [PATCH] mm: memblock: Fix arguments passed to memblock_set_node()

在2025年4月25日周五下午3:18，David Woodhouse写道：
[...]

Hi David & co,

Quoted 5 lines

I did a quick grep for memblock_set_node() callers, and the one in szmem() in arch/mips/loongson64/init.c looks odd.

Quoted 7 lines

       /* set nid for reserved memory */
       memblock_set_node((u64)node << 44, (u64)(node + 1) << 44,
                       &memblock.reserved, node);
At first glance I suspect the 'size' should just be (1<<44) or maybe it should be inside the loop over the memmap, and called with mem_start, mem_size each time?

You are right, it should be (1 << 44), it is an oversight when I was converting MIPS private boot allocator to memblock.

Quoted 4 lines

And why are we calling memblock_reserve() for what appears to be a single system-wide vgabios_addr, repeatedly each time szmem() is called for a different NUMA node?

Alas this should be done only for node 0.

Thanks

Quoted 4 lines

附件:
* smime.p7s

Signature · 1 line

- Jiaxun

Andrew Morton

maintainer

2025-04-26 01:05 UTC #4 lore raw

Re: [PATCH] mm: memblock: Fix arguments passed to memblock_set_node()

In reply to Bernardo C. Gutierrez Cantu

Quoted 16 lines · On Fri, 25 Apr 2025 10:20:03 +0000 "Bernardo C. Gutierrez Cantu"…

memblock_set_node() receives a base and a size arguments, but we are passing the start and end of the memory regions when iterating over them in memmap_init_reserved_pages() to set their node ids.
This results in the function setting the node ids for the reserved memory regions in [base, base + base + size) instead of [base, base + size).
Pass start and size, so that we iterate over the correct range.
Fixes 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
...

mm/memblock.c static void __init memmap_init_reserved_pages(void) · line 2202

Quoted 8 lines

mm/memblock.c +1 -1

@@ -2196,7 +2196,7 @@ static void __init memmap_init_reserved_pages(void)
 		if (memblock_is_nomap(region))
 			reserve_bootmem_region(start, end, nid);
 
-		memblock_set_node(start, end, &memblock.reserved, nid);
+		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
 
 	/*

What were the runtime effects of this bug?

Bernardo C. Gutierrez Cantu

author

2025-04-28 09:14 UTC #5 lore raw

Re: [PATCH] mm: memblock: Fix arguments passed to memblock_set_node()

Quoted 21 lines · On Sat, 26 Apr 2025 03:05:39 +0200 Andrew Morton <[email protected]>…

Quoted 19 lines · On Fri, 25 Apr 2025 10:20:03 +0000 "Bernardo C. Gutierrez Cantu"…
memblock_set_node() receives a base and a size arguments, but we are passing the start and end of the memory regions when iterating over them in memmap_init_reserved_pages() to set their node ids.
This results in the function setting the node ids for the reserved memory regions in [base, base + base + size) instead of [base, base + size).
Pass start and size, so that we iterate over the correct range.
Fixes 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
...
mm/memblock.c +0 -0
@@ -2196,7 +2196,7 @@ static void __init memmap_init_reserved_pages(void)
 		if (memblock_is_nomap(region))
 			reserve_bootmem_region(start, end, nid);
 

Quoted 7 lines

mm/memblock.c static void __init memmap_init_reserved_pages(void) · line 2202
Quoted 5 lines
mm/memblock.c +1 -1
-		memblock_set_node(start, end, &memblock.reserved, nid);
+		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
 
 	/*
What were the runtime effects of this bug?

I found the bug via code introspection while trying to learn how the boot time memory allocation of Linux worked. I was not actively pursuing to fix a runtime issue that I could then prove was fixed by this.

But I see that in most cases this bug should be mostly benign (which could explain why it was not caught before). In kernels compiled with CONFIG_NUMA, the memblock_set_node() function would iterate over more memblock regions and potentially set incorrect node ids, which would then be overwritten with the correct node ids once the caller function memmap_init_reserved_pages() iterates over the next regions. So the resultant node ids after initialization should still be correct.

It could still impact the length of the mm init process on kernel boot, but I did not measure the improvement, as it is highly dependent on the memory configuration system, and in my specific system under test, it was neglible.

Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

Mike Rapoport

maintainer

2025-04-26 08:22 UTC #6 lore raw

Re: [PATCH] mm: memblock: Fix arguments passed to memblock_set_node()

In reply to Bernardo C. Gutierrez Cantu

Hi Bernardo,

Quoted 4 lines · Bernardo C. Gutierrez Cantu

memblock_set_node() receives a base and a size arguments, but we are passing the start and end of the memory regions when iterating over them in memmap_init_reserved_pages() to set their node ids.

Quoted 8 lines

This results in the function setting the node ids for the reserved memory regions in [base, base + base + size) instead of [base, base + size).
Pass start and size, so that we iterate over the correct range.
Fixes 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
Signed-off-by Bernardo C. Gutierrez Cantu <[email protected]>

There's already a fix in memblock tree:
https://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock.git/commit/?h=for-next&id=06eaa824fd239edd1eab2754f29b2d03da313003

Will send PR to Linus soon.

Quoted 19 lines

---
 mm/memblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memblock.c +1 -1

index 0a53db4d9f7b..9639f04b4fdf 100644
@@ -2196,7 +2196,7 @@ static void __init memmap_init_reserved_pages(void)
 		if (memblock_is_nomap(region))
 			reserve_bootmem_region(start, end, nid);
 
-		memblock_set_node(start, end, &memblock.reserved, nid);
+		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
 
 	/*

Signature · 1 line

2.47.1

Signature · 2 lines

Sincerely yours,
Mike.

Bernardo C. Gutierrez Cantu

author

2025-04-28 09:53 UTC #7 lore raw

Re: [PATCH] mm: memblock: Fix arguments passed to memblock_set_node()

Quoted 12 lines · Mike Rapoport

Hi Bernardo,
Quoted 9 lines · Bernardo C. Gutierrez Cantu
memblock_set_node() receives a base and a size arguments, but we are passing the start and end of the memory regions when iterating over them in memmap_init_reserved_pages() to set their node ids.
This results in the function setting the node ids for the reserved memory regions in [base, base + base + size) instead of [base, base + size).
Pass start and size, so that we iterate over the correct range.

Quoted 9 lines

Quoted 3 lines
Fixes 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
Signed-off-by Bernardo C. Gutierrez Cantu <[email protected]>
There's already a fix in memblock tree:
https://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock.git/commit/?h=for-next&id=06eaa824fd239edd1eab2754f29b2d03da313003
Will send PR to Linus soon.

Hi Mike

Oh ok, I see. Thank you, that patch should fix the issue.

Will it also be backported to LTS kernels affected by the bug, which was introduced in v6.5?

Quoted 12 lines

---
 mm/memblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memblock.c +0 -0

index 0a53db4d9f7b..9639f04b4fdf 100644
@@ -2196,7 +2196,7 @@ static void __init memmap_init_reserved_pages(void)
 		if (memblock_is_nomap(region))
 			reserve_bootmem_region(start, end, nid);

Quoted 11 lines

Quoted 7 lines

mm/memblock.c +1 -1

-		memblock_set_node(start, end, &memblock.reserved, nid);
+		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
 
 	/*

Signature · 1 line

2.47.1

Signature · 2 lines

Sincerely yours,
Mike.

Best regards
Bernardo