The unflatten_device_tree() function contains a call to
memblock_alloc(). This is a problem because this allocation is done
before any of the reserved memory is set aside in paging_init().
This means that there is a possibility for memblock to allocate from
any of the memory regions that are supposed to be set aside as reserved.
Hence, move the call to paging_init() to be earlier in the init
sequence so that the reserved memory regions are set aside before any
allocations are done using memblock.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Oreoluwa Babatunde <[email protected]>
---
v2:
- Added Rob Herrings Reviewed-by.
- cc Andrew Morton to assist with merging this for sh architecture.
Similar change made for loongarch and openrisc in v1 have already
been merged.
v1:
https://lore.kernel.org/all/[email protected]/
arch/sh/kernel/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c
index 620e5cf8ae1e..98c8473e130d 100644
--- a/arch/sh/kernel/setup.c
+++ b/arch/sh/kernel/setup.c
@@ -322,6 +322,8 @@ void __init setup_arch(char **cmdline_p)
/* Let earlyprintk output early console messages */
sh_early_platform_driver_probe("earlyprintk", 1, 1);
+ paging_init();
+
#ifdef CONFIG_OF_EARLY_FLATTREE
#ifdef CONFIG_USE_BUILTIN_DTB
unflatten_and_copy_device_tree();
@@ -330,8 +332,6 @@ void __init setup_arch(char **cmdline_p)
#endif
#endif
- paging_init();
-
/* Perform the machine specific initialisation */
if (likely(sh_mv.mv_setup))
sh_mv.mv_setup(cmdline_p);
--
2.34.1
On Tue, 2024-04-23 at 16:31 -0700, Oreoluwa Babatunde wrote:
> Reviewed-by: Rob Herring <[email protected]>
> Signed-off-by: Oreoluwa Babatunde <[email protected]>
> ---
> v2:
> - Added Rob Herrings Reviewed-by.
> - cc Andrew Morton to assist with merging this for sh architecture.
> Similar change made for loongarch and openrisc in v1 have already
> been merged.
I'll do it no later than this weekend. Please don't put me under pressure.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
…
> Hence, move the call to paging_init() to be earlier in the init
> sequence so that the reserved memory regions are set aside before any
> allocations are done using memblock.
…
Will the tag “Fixes” become relevant here?
Regards,
Markus
On Wed, 2024-04-24 at 10:45 +0200, Markus Elfring wrote:
> …
> > Hence, move the call to paging_init() to be earlier in the init
> > sequence so that the reserved memory regions are set aside before any
> > allocations are done using memblock.
> …
>
> Will the tag “Fixes” become relevant here?
I'm not aware of any bugs that have been reported in this context.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
>> …
>>> Hence, move the call to paging_init() to be earlier in the init
>>> sequence so that the reserved memory regions are set aside before any
>>> allocations are done using memblock.
>> …
>>
>> Will the tag “Fixes” become relevant here?
>
> I'm not aware of any bugs that have been reported in this context.
Can the mentioned questionable function call ordering be interpreted
as a programming mistake?
Regards,
Markus
Hi Oreoluwa,
On Tue, 2024-04-23 at 16:31 -0700, Oreoluwa Babatunde wrote:
> The unflatten_device_tree() function contains a call to
> memblock_alloc(). This is a problem because this allocation is done
> before any of the reserved memory is set aside in paging_init().
> This means that there is a possibility for memblock to allocate from
> any of the memory regions that are supposed to be set aside as reserved.
>
> Hence, move the call to paging_init() to be earlier in the init
> sequence so that the reserved memory regions are set aside before any
> allocations are done using memblock.
>
> Reviewed-by: Rob Herring <[email protected]>
> Signed-off-by: Oreoluwa Babatunde <[email protected]>
> ---
> v2:
> - Added Rob Herrings Reviewed-by.
> - cc Andrew Morton to assist with merging this for sh architecture.
> Similar change made for loongarch and openrisc in v1 have already
> been merged.
Could you link the changes for references so I can have a look?
> v1:
> https://lore.kernel.org/all/[email protected]/
>
> arch/sh/kernel/setup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c
> index 620e5cf8ae1e..98c8473e130d 100644
> --- a/arch/sh/kernel/setup.c
> +++ b/arch/sh/kernel/setup.c
> @@ -322,6 +322,8 @@ void __init setup_arch(char **cmdline_p)
> /* Let earlyprintk output early console messages */
> sh_early_platform_driver_probe("earlyprintk", 1, 1);
>
> + paging_init();
> +
> #ifdef CONFIG_OF_EARLY_FLATTREE
> #ifdef CONFIG_USE_BUILTIN_DTB
> unflatten_and_copy_device_tree();
> @@ -330,8 +332,6 @@ void __init setup_arch(char **cmdline_p)
> #endif
> #endif
>
> - paging_init();
> -
> /* Perform the machine specific initialisation */
> if (likely(sh_mv.mv_setup))
> sh_mv.mv_setup(cmdline_p);
Reviewed-by: John Paul Adrian Glaubitz <[email protected]>
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On 4/29/2024 2:03 AM, John Paul Adrian Glaubitz wrote:
> Hi Oreoluwa,
>
> On Tue, 2024-04-23 at 16:31 -0700, Oreoluwa Babatunde wrote:
>> The unflatten_device_tree() function contains a call to
>> memblock_alloc(). This is a problem because this allocation is done
>> before any of the reserved memory is set aside in paging_init().
>> This means that there is a possibility for memblock to allocate from
>> any of the memory regions that are supposed to be set aside as reserved.
>>
>> Hence, move the call to paging_init() to be earlier in the init
>> sequence so that the reserved memory regions are set aside before any
>> allocations are done using memblock.
>>
>> Reviewed-by: Rob Herring <[email protected]>
>> Signed-off-by: Oreoluwa Babatunde <[email protected]>
>> ---
>> v2:
>> - Added Rob Herrings Reviewed-by.
>> - cc Andrew Morton to assist with merging this for sh architecture.
>> Similar change made for loongarch and openrisc in v1 have already
>> been merged.
> Could you link the changes for references so I can have a look?
Hi John,
Here are links to the corresponding changes on Loongarch and Openrisc.
- Loongarch:
https://lore.kernel.org/all/[email protected]/
- Openrisc:
https://lore.kernel.org/all/[email protected]/
Thank you!
Oreoluwa
>
>> v1:
>> https://lore.kernel.org/all/[email protected]/
>>
>> arch/sh/kernel/setup.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c
>> index 620e5cf8ae1e..98c8473e130d 100644
>> --- a/arch/sh/kernel/setup.c
>> +++ b/arch/sh/kernel/setup.c
>> @@ -322,6 +322,8 @@ void __init setup_arch(char **cmdline_p)
>> /* Let earlyprintk output early console messages */
>> sh_early_platform_driver_probe("earlyprintk", 1, 1);
>>
>> + paging_init();
>> +
>> #ifdef CONFIG_OF_EARLY_FLATTREE
>> #ifdef CONFIG_USE_BUILTIN_DTB
>> unflatten_and_copy_device_tree();
>> @@ -330,8 +332,6 @@ void __init setup_arch(char **cmdline_p)
>> #endif
>> #endif
>>
>> - paging_init();
>> -
>> /* Perform the machine specific initialisation */
>> if (likely(sh_mv.mv_setup))
>> sh_mv.mv_setup(cmdline_p);
> Reviewed-by: John Paul Adrian Glaubitz <[email protected]>
>
> Adrian
>
Hi Oreoluwa,
On Mon, 2024-04-29 at 09:28 -0700, Oreoluwa Babatunde wrote:
> Here are links to the corresponding changes on Loongarch and Openrisc.
>
> - Loongarch:
> https://lore.kernel.org/all/[email protected]/
>
> - Openrisc:
> https://lore.kernel.org/all/[email protected]/
Great, thanks a lot! I will apply all reviewed patches to my sh-linux tree tomorrow.
Thanks,
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On 4/29/2024 10:26 AM, John Paul Adrian Glaubitz wrote:
> Hi Oreoluwa,
>
> On Mon, 2024-04-29 at 09:28 -0700, Oreoluwa Babatunde wrote:
>> Here are links to the corresponding changes on Loongarch and Openrisc.
>>
>> - Loongarch:
>> https://lore.kernel.org/all/[email protected]/
>>
>> - Openrisc:
>> https://lore.kernel.org/all/[email protected]/
> Great, thanks a lot! I will apply all reviewed patches to my sh-linux tree tomorrow.
>
> Thanks,
> Adrian
Thank you!
Hi Oreoluwa,
On Tue, 2024-04-23 at 16:31 -0700, Oreoluwa Babatunde wrote:
> The unflatten_device_tree() function contains a call to
> memblock_alloc(). This is a problem because this allocation is done
> before any of the reserved memory is set aside in paging_init().
> This means that there is a possibility for memblock to allocate from
> any of the memory regions that are supposed to be set aside as reserved.
>
> Hence, move the call to paging_init() to be earlier in the init
> sequence so that the reserved memory regions are set aside before any
> allocations are done using memblock.
I was just about to merge your patch when I ran a git blame on the code in
arch/sh/kernel/setup.c and noticed the following commit by Rich Felker:
commit eb6b6930a70faefe04479a71088cc10366782d9a
Author: Rich Felker <[email protected]>
Date: Mon Jul 31 01:27:50 2017 -0400
sh: fix memory corruption of unflattened device tree
unflatten_device_tree() makes use of memblock allocation, and
therefore must be called before paging_init() migrates the memblock
allocation data to the bootmem framework. Otherwise the record of the
allocation for the expanded device tree will be lost, and will
eventually be clobbered when allocated for another use.
Signed-off-by: Rich Felker <[email protected]>
It looks like that the call to unflatten_device_tree() before paging_init()
is intentional and needed for the device tree to be preserved in memory
after running paging_init().
@Geert: Do you have any comments on this patch?
@Rob: Could you test this patch on your J2 board and report back?
Thanks,
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Hi Oreoluwa,
On Wed, 2024-05-01 at 19:18 +0200, John Paul Adrian Glaubitz wrote:
> Hi Oreoluwa,
>
> On Tue, 2024-04-23 at 16:31 -0700, Oreoluwa Babatunde wrote:
> > The unflatten_device_tree() function contains a call to
> > memblock_alloc(). This is a problem because this allocation is done
> > before any of the reserved memory is set aside in paging_init().
> > This means that there is a possibility for memblock to allocate from
> > any of the memory regions that are supposed to be set aside as reserved.
> >
> > Hence, move the call to paging_init() to be earlier in the init
> > sequence so that the reserved memory regions are set aside before any
> > allocations are done using memblock.
>
> I was just about to merge your patch when I ran a git blame on the code in
> arch/sh/kernel/setup.c and noticed the following commit by Rich Felker:
>
> commit eb6b6930a70faefe04479a71088cc10366782d9a
> Author: Rich Felker <[email protected]>
> Date: Mon Jul 31 01:27:50 2017 -0400
>
> sh: fix memory corruption of unflattened device tree
>
> unflatten_device_tree() makes use of memblock allocation, and
> therefore must be called before paging_init() migrates the memblock
> allocation data to the bootmem framework. Otherwise the record of the
> allocation for the expanded device tree will be lost, and will
> eventually be clobbered when allocated for another use.
>
> Signed-off-by: Rich Felker <[email protected]>
>
> It looks like that the call to unflatten_device_tree() before paging_init()
> is intentional and needed for the device tree to be preserved in memory
> after running paging_init().
>
> @Geert: Do you have any comments on this patch?
> @Rob: Could you test this patch on your J2 board and report back?
I'm skipping this patch for v6.10 now for the aforementioned reasons.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On 5/2/2024 3:29 AM, John Paul Adrian Glaubitz wrote:
> Hi Oreoluwa,
>
> On Wed, 2024-05-01 at 19:18 +0200, John Paul Adrian Glaubitz wrote:
>> Hi Oreoluwa,
>>
>> On Tue, 2024-04-23 at 16:31 -0700, Oreoluwa Babatunde wrote:
>>> The unflatten_device_tree() function contains a call to
>>> memblock_alloc(). This is a problem because this allocation is done
>>> before any of the reserved memory is set aside in paging_init().
>>> This means that there is a possibility for memblock to allocate from
>>> any of the memory regions that are supposed to be set aside as reserved.
>>>
>>> Hence, move the call to paging_init() to be earlier in the init
>>> sequence so that the reserved memory regions are set aside before any
>>> allocations are done using memblock.
>> I was just about to merge your patch when I ran a git blame on the code in
>> arch/sh/kernel/setup.c and noticed the following commit by Rich Felker:
>>
>> commit eb6b6930a70faefe04479a71088cc10366782d9a
>> Author: Rich Felker <[email protected]>
>> Date: Mon Jul 31 01:27:50 2017 -0400
>>
>> sh: fix memory corruption of unflattened device tree
>>
>> unflatten_device_tree() makes use of memblock allocation, and
>> therefore must be called before paging_init() migrates the memblock
>> allocation data to the bootmem framework. Otherwise the record of the
>> allocation for the expanded device tree will be lost, and will
>> eventually be clobbered when allocated for another use.
>>
>> Signed-off-by: Rich Felker <[email protected]>
>>
>> It looks like that the call to unflatten_device_tree() before paging_init()
>> is intentional and needed for the device tree to be preserved in memory
>> after running paging_init().
Hi John,
Thank you for pointing this out.
memblock_alloc() marks all its allocations as reserved by calling
memblock_reserve().
https://elixir.bootlin.com/linux/latest/source/mm/memblock.c#L1463
This should normally stop other users from allocating from within that
region of memory.
But in this case, since all the free memory regions have already been
transferred over to the bootmem framework by paging_init(), I am not
sure if that logic will still hold for the unflatten_deivcetree allocated memory.
The main goal of this patch is to make sure that the reserved memory
regions defined in the DT are set aside before any memblock allocations
are done (which includes the allocation done by unflatten_devicetree).
Hence, I can restructure the patch to only remove the portion of code that is
is responsible for setting aside the DT defined reserved memory regions from
within paging_init(), and move it above the unflatten_devicetree() call.
https://elixir.bootlin.com/linux/latest/source/arch/sh/mm/init.c#L292
I will explore further and possibly restructure this patch based on my findings.
Thank you!
Oreoluwa
>>
>> @Geert: Do you have any comments on this patch?
>> @Rob: Could you test this patch on your J2 board and report back?
Hello Oreoluwa,
On Tue, 2024-05-07 at 14:42 -0700, Oreoluwa Babatunde wrote:
> memblock_alloc() marks all its allocations as reserved by calling
> memblock_reserve().
> https://elixir.bootlin.com/linux/latest/source/mm/memblock.c#L1463
>
> This should normally stop other users from allocating from within that
> region of memory.
>
> But in this case, since all the free memory regions have already been
> transferred over to the bootmem framework by paging_init(), I am not
> sure if that logic will still hold for the unflatten_deivcetree allocated memory.
>
> The main goal of this patch is to make sure that the reserved memory
> regions defined in the DT are set aside before any memblock allocations
> are done (which includes the allocation done by unflatten_devicetree).
>
> Hence, I can restructure the patch to only remove the portion of code that is
> is responsible for setting aside the DT defined reserved memory regions from
> within paging_init(), and move it above the unflatten_devicetree() call.
> https://elixir.bootlin.com/linux/latest/source/arch/sh/mm/init.c#L292
>
> I will explore further and possibly restructure this patch based on my findings.
OK, sounds like a plan. In the meantime, I have set up my J2 Turtle Board and
I am actually now able to test patches for this target, so that I would be
able to verify that your patch didn't break anything.
However, I think for v6.10 I think the ship has sailed.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On 5/7/2024 3:41 PM, John Paul Adrian Glaubitz wrote:
> Hello Oreoluwa,
>
> On Tue, 2024-05-07 at 14:42 -0700, Oreoluwa Babatunde wrote:
>> memblock_alloc() marks all its allocations as reserved by calling
>> memblock_reserve().
>> https://elixir.bootlin.com/linux/latest/source/mm/memblock.c#L1463
>>
>> This should normally stop other users from allocating from within that
>> region of memory.
>>
>> But in this case, since all the free memory regions have already been
>> transferred over to the bootmem framework by paging_init(), I am not
>> sure if that logic will still hold for the unflatten_deivcetree allocated memory.
>>
>> The main goal of this patch is to make sure that the reserved memory
>> regions defined in the DT are set aside before any memblock allocations
>> are done (which includes the allocation done by unflatten_devicetree).
>>
>> Hence, I can restructure the patch to only remove the portion of code that is
>> is responsible for setting aside the DT defined reserved memory regions from
>> within paging_init(), and move it above the unflatten_devicetree() call.
>> https://elixir.bootlin.com/linux/latest/source/arch/sh/mm/init.c#L292
>>
>> I will explore further and possibly restructure this patch based on my findings.
> OK, sounds like a plan. In the meantime, I have set up my J2 Turtle Board and
> I am actually now able to test patches for this target, so that I would be
> able to verify that your patch didn't break anything.
>
> However, I think for v6.10 I think the ship has sailed.
>
> Adrian
Hi Adrian,
I have uploaded v3 of this patch now.
https://lore.kernel.org/all/[email protected]/
In this new version, paging_init() is left in its original position and only the portion
of code that is responsible for setting aside the reserved memory regions is moved.
Thank you!
Oreoluwa
Hi Oreoluwa,
On Mon, 2024-05-20 at 11:03 -0700, Oreoluwa Babatunde wrote:
> I have uploaded v3 of this patch now.
> https://lore.kernel.org/all/[email protected]/
>
> In this new version, paging_init() is left in its original position and only the portion
> of code that is responsible for setting aside the reserved memory regions is moved.
Thanks a lot for the updated version.
I will give it a try later this week as we had a public holiday in Germany
today and therefore enjoyed an extended weekend.
If it doesn't break anything and Geert is happy with the change as well,
I'll pick it up for v6.11.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On 5/20/2024 11:24 AM, John Paul Adrian Glaubitz wrote:
> Hi Oreoluwa,
>
> On Mon, 2024-05-20 at 11:03 -0700, Oreoluwa Babatunde wrote:
>> I have uploaded v3 of this patch now.
>> https://lore.kernel.org/all/[email protected]/
>>
>> In this new version, paging_init() is left in its original position and only the portion
>> of code that is responsible for setting aside the reserved memory regions is moved.
> Thanks a lot for the updated version.
>
> I will give it a try later this week as we had a public holiday in Germany
> today and therefore enjoyed an extended weekend.
>
> If it doesn't break anything and Geert is happy with the change as well,
> I'll pick it up for v6.11.
>
> Adrian
Hi Adrian,
That sounds good, thank you!
Enjoy the extended weekend!
Regards,
Oreoluwa