[PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[Jiaxun Yang]

Previously only a pointer to fdt string pool is saved to struct
reserved_mem as region name.

As on some architectures booting FDT will be wiped at later initialisation
stages, this is breaking reserved_mem users.

Copy and save the whole string into struct reserved_mem to avoid
FDT lifecycle problem.

Reported-by: Kevin Wheatfox <enkerewpo@hotmail.com>
Closes: https://lore.kernel.org/loongarch/ME4P282MB1397447C3C094554C7AF2E37B58E2@ME4P282MB1397.AUSP282.PROD.OUTLOOK.COM/
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 drivers/of/of_reserved_mem.c    | 2 +-
 include/linux/of_reserved_mem.h | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 46e1c3fbc769..22841599cd83 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -70,7 +70,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
 	}
 
 	rmem->fdt_node = node;
-	rmem->name = uname;
+	strscpy(rmem->name, uname, RESERVED_MEM_NAME_LEN);
 	rmem->base = base;
 	rmem->size = size;
 
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
index e338282da652..ed9de36c9cc9 100644
--- a/include/linux/of_reserved_mem.h
+++ b/include/linux/of_reserved_mem.h
@@ -8,8 +8,10 @@
 struct of_phandle_args;
 struct reserved_mem_ops;
 
+#define RESERVED_MEM_NAME_LEN	128
+
 struct reserved_mem {
-	const char			*name;
+	char				name[RESERVED_MEM_NAME_LEN];
 	unsigned long			fdt_node;
 	const struct reserved_mem_ops	*ops;
 	phys_addr_t			base;

---
base-commit: bb1b0acdcd66e0d8eedee3570d249e076b89ab32
change-id: 20240821-save_resv_name-4f2e2cb8883b

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

Re: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[kernel test robot]

Hi Jiaxun,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bb1b0acdcd66e0d8eedee3570d249e076b89ab32]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiaxun-Yang/of_reserved_mem-Save-region-name-string-into-struct-reserved_mem/20240821-215235
base:   bb1b0acdcd66e0d8eedee3570d249e076b89ab32
patch link:    https://lore.kernel.org/r/20240821-save_resv_name-v1-1-b9c17f103ffb%40flygoat.com
patch subject: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem
config: i386-buildonly-randconfig-002-20240824 (https://download.01.org/0day-ci/archive/20240824/202408241510.Rk14q8Rg-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240824/202408241510.Rk14q8Rg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408241510.Rk14q8Rg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/of/of_reserved_mem.c:467:12: warning: address of array 'rmem->name' will always evaluate to 'true' [-Wpointer-bool-conversion]
     467 |                                         rmem->name ? rmem->name : "unknown");
         |                                         ~~~~~~^~~~ ~
   include/linux/printk.h:538:34: note: expanded from macro 'pr_info'
     538 |         printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
         |                                         ^~~~~~~~~~~
   include/linux/printk.h:465:60: note: expanded from macro 'printk'
     465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
         |                                                            ^~~~~~~~~~~
   include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
     437 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                                 ^~~~~~~~~~~
   1 warning generated.


vim +467 drivers/of/of_reserved_mem.c

ae1add247bf8c2 Mitchel Humpherys  2015-09-15  426  
3f0c8206644836 Marek Szyprowski   2014-02-28  427  /**
c8813f7ec01c67 chenqiwu           2020-05-11  428   * fdt_init_reserved_mem() - allocate and init all saved reserved memory regions
3f0c8206644836 Marek Szyprowski   2014-02-28  429   */
3f0c8206644836 Marek Szyprowski   2014-02-28  430  void __init fdt_init_reserved_mem(void)
3f0c8206644836 Marek Szyprowski   2014-02-28  431  {
3f0c8206644836 Marek Szyprowski   2014-02-28  432  	int i;
ae1add247bf8c2 Mitchel Humpherys  2015-09-15  433  
ae1add247bf8c2 Mitchel Humpherys  2015-09-15  434  	/* check for overlapping reserved regions */
ae1add247bf8c2 Mitchel Humpherys  2015-09-15  435  	__rmem_check_for_overlap();
ae1add247bf8c2 Mitchel Humpherys  2015-09-15  436  
3f0c8206644836 Marek Szyprowski   2014-02-28  437  	for (i = 0; i < reserved_mem_count; i++) {
3f0c8206644836 Marek Szyprowski   2014-02-28  438  		struct reserved_mem *rmem = &reserved_mem[i];
3f0c8206644836 Marek Szyprowski   2014-02-28  439  		unsigned long node = rmem->fdt_node;
3f0c8206644836 Marek Szyprowski   2014-02-28  440  		int err = 0;
6f1188b4ac7577 Yue Hu             2020-07-30  441  		bool nomap;
3f0c8206644836 Marek Szyprowski   2014-02-28  442  
d0b8ed47e83a22 pierre Kuo         2019-02-19  443  		nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
9dcfee01930e6c Marek Szyprowski   2014-07-14  444  
3f0c8206644836 Marek Szyprowski   2014-02-28  445  		if (rmem->size == 0)
3f0c8206644836 Marek Szyprowski   2014-02-28  446  			err = __reserved_mem_alloc_size(node, rmem->name,
3f0c8206644836 Marek Szyprowski   2014-02-28  447  						 &rmem->base, &rmem->size);
d0b8ed47e83a22 pierre Kuo         2019-02-19  448  		if (err == 0) {
d0b8ed47e83a22 pierre Kuo         2019-02-19  449  			err = __reserved_mem_init_node(rmem);
d0b8ed47e83a22 pierre Kuo         2019-02-19  450  			if (err != 0 && err != -ENOENT) {
d0b8ed47e83a22 pierre Kuo         2019-02-19  451  				pr_info("node %s compatible matching fail\n",
d0b8ed47e83a22 pierre Kuo         2019-02-19  452  					rmem->name);
d0b8ed47e83a22 pierre Kuo         2019-02-19  453  				if (nomap)
7b25995f5319ad Dong Aisheng       2021-06-11  454  					memblock_clear_nomap(rmem->base, rmem->size);
3c6867a12a224d Dong Aisheng       2021-06-11  455  				else
3ecc68349bbab6 Mike Rapoport      2021-11-05  456  					memblock_phys_free(rmem->base,
3ecc68349bbab6 Mike Rapoport      2021-11-05  457  							   rmem->size);
aeb9267eb6b1df Martin Liu         2023-02-10  458  			} else {
aeb9267eb6b1df Martin Liu         2023-02-10  459  				phys_addr_t end = rmem->base + rmem->size - 1;
aeb9267eb6b1df Martin Liu         2023-02-10  460  				bool reusable =
aeb9267eb6b1df Martin Liu         2023-02-10  461  					(of_get_flat_dt_prop(node, "reusable", NULL)) != NULL;
aeb9267eb6b1df Martin Liu         2023-02-10  462  
6ee7afbabcee4d Geert Uytterhoeven 2023-02-16  463  				pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
aeb9267eb6b1df Martin Liu         2023-02-10  464  					&rmem->base, &end, (unsigned long)(rmem->size / SZ_1K),
aeb9267eb6b1df Martin Liu         2023-02-10  465  					nomap ? "nomap" : "map",
aeb9267eb6b1df Martin Liu         2023-02-10  466  					reusable ? "reusable" : "non-reusable",
aeb9267eb6b1df Martin Liu         2023-02-10 @467  					rmem->name ? rmem->name : "unknown");
d0b8ed47e83a22 pierre Kuo         2019-02-19  468  			}
d0b8ed47e83a22 pierre Kuo         2019-02-19  469  		}
3f0c8206644836 Marek Szyprowski   2014-02-28  470  	}
3f0c8206644836 Marek Szyprowski   2014-02-28  471  }
9dcfee01930e6c Marek Szyprowski   2014-07-14  472  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[Krzysztof Kozlowski]

On Wed, Aug 21, 2024 at 02:51:02PM +0100, Jiaxun Yang wrote:
> Previously only a pointer to fdt string pool is saved to struct
> reserved_mem as region name.
> 
> As on some architectures booting FDT will be wiped at later initialisation
> stages, this is breaking reserved_mem users.
> 
> Copy and save the whole string into struct reserved_mem to avoid
> FDT lifecycle problem.
> 
> Reported-by: Kevin Wheatfox <enkerewpo@hotmail.com>
> Closes: https://lore.kernel.org/loongarch/ME4P282MB1397447C3C094554C7AF2E37B58E2@ME4P282MB1397.AUSP282.PROD.OUTLOOK.COM/

I doubt this uses mainline tree...

> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

This is a bigger problem and should be solved unified way in multiple
places. You cannot just wipe out FDT from the memory, because it is used
in all other places.

Your report earlier probably used some custom patches allowing this
wipe out, but that's the mistake. Fix your wiping out mechanism...

Best regards,
Krzysztof

Re: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[Krzysztof Kozlowski]

On 25/08/2024 09:18, Krzysztof Kozlowski wrote:
> On Wed, Aug 21, 2024 at 02:51:02PM +0100, Jiaxun Yang wrote:
>> Previously only a pointer to fdt string pool is saved to struct
>> reserved_mem as region name.
>>
>> As on some architectures booting FDT will be wiped at later initialisation
>> stages, this is breaking reserved_mem users.
>>
>> Copy and save the whole string into struct reserved_mem to avoid
>> FDT lifecycle problem.
>>
>> Reported-by: Kevin Wheatfox <enkerewpo@hotmail.com>
>> Closes: https://lore.kernel.org/loongarch/ME4P282MB1397447C3C094554C7AF2E37B58E2@ME4P282MB1397.AUSP282.PROD.OUTLOOK.COM/
> 
> I doubt this uses mainline tree...
> 
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> 
> This is a bigger problem and should be solved unified way in multiple
> places. You cannot just wipe out FDT from the memory, because it is used
> in all other places.
> 
> Your report earlier probably used some custom patches allowing this
> wipe out, but that's the mistake. Fix your wiping out mechanism...

The commit msg is quite vague on real problem, so one has to go to
original report to find that you use built-in dtb, so only the
unflatten_and_copy_device_tree() path, while I have impression you just
want to drop the copied (in-kernel) FDT.

Fix the commit msg to describe the real problem being fixed here.
Provide also proper fixes tag.

Best regards,
Krzysztof

Re: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[Rob Herring]

On Wed, Aug 21, 2024 at 8:51 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
> Previously only a pointer to fdt string pool is saved to struct
> reserved_mem as region name.
>
> As on some architectures booting FDT will be wiped at later initialisation
> stages, this is breaking reserved_mem users.

What architectures? Be specific.

Why is the FDT wiped? It should be preserved and you need it later to
implement kexec.

>
> Copy and save the whole string into struct reserved_mem to avoid
> FDT lifecycle problem.
>
> Reported-by: Kevin Wheatfox <enkerewpo@hotmail.com>
> Closes: https://lore.kernel.org/loongarch/ME4P282MB1397447C3C094554C7AF2E37B58E2@ME4P282MB1397.AUSP282.PROD.OUTLOOK.COM/
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  drivers/of/of_reserved_mem.c    | 2 +-
>  include/linux/of_reserved_mem.h | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 46e1c3fbc769..22841599cd83 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -70,7 +70,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
>         }
>
>         rmem->fdt_node = node;
> -       rmem->name = uname;
> +       strscpy(rmem->name, uname, RESERVED_MEM_NAME_LEN);
>         rmem->base = base;
>         rmem->size = size;
>
> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
> index e338282da652..ed9de36c9cc9 100644
> --- a/include/linux/of_reserved_mem.h
> +++ b/include/linux/of_reserved_mem.h
> @@ -8,8 +8,10 @@
>  struct of_phandle_args;
>  struct reserved_mem_ops;
>
> +#define RESERVED_MEM_NAME_LEN  128
> +
>  struct reserved_mem {
> -       const char                      *name;
> +       char                            name[RESERVED_MEM_NAME_LEN];
>         unsigned long                   fdt_node;
>         const struct reserved_mem_ops   *ops;
>         phys_addr_t                     base;
>
> ---
> base-commit: bb1b0acdcd66e0d8eedee3570d249e076b89ab32
> change-id: 20240821-save_resv_name-4f2e2cb8883b
>
> Best regards,
> --
> Jiaxun Yang <jiaxun.yang@flygoat.com>
>

Re: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[Jiaxun Yang]

在2024年8月26日八月 下午3:09，Rob Herring写道：
> On Wed, Aug 21, 2024 at 8:51 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>>
>> Previously only a pointer to fdt string pool is saved to struct
>> reserved_mem as region name.
>>
>> As on some architectures booting FDT will be wiped at later initialisation
>> stages, this is breaking reserved_mem users.
>
> What architectures? Be specific.

It's LoongArch and MIPS, I'll expand commit message.

FDT might be placed in .init sections or memory not managed by kernel, thus
it may be wiped out.

>
> Why is the FDT wiped? It should be preserved and you need it later to
> implement kexec.

So KEXEC is using kernel's self copy of FDT created by unflatten_and_copy_device_tree(),
while reserved-mem scan is performed before copy to ensure that reserved memory
are being tracked by memblock before possible memblock_alloc in unflatten_and_copy_device_tree().

Thanks
- Jiaxun

>
>>
>> Copy and save the whole string into struct reserved_mem to avoid
>> FDT lifecycle problem.
>>
>> Reported-by: Kevin Wheatfox <enkerewpo@hotmail.com>
>> Closes: https://lore.kernel.org/loongarch/ME4P282MB1397447C3C094554C7AF2E37B58E2@ME4P282MB1397.AUSP282.PROD.OUTLOOK.COM/
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>>  drivers/of/of_reserved_mem.c    | 2 +-
>>  include/linux/of_reserved_mem.h | 4 +++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 46e1c3fbc769..22841599cd83 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -70,7 +70,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
>>         }
>>
>>         rmem->fdt_node = node;
>> -       rmem->name = uname;
>> +       strscpy(rmem->name, uname, RESERVED_MEM_NAME_LEN);
>>         rmem->base = base;
>>         rmem->size = size;
>>
>> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
>> index e338282da652..ed9de36c9cc9 100644
>> --- a/include/linux/of_reserved_mem.h
>> +++ b/include/linux/of_reserved_mem.h
>> @@ -8,8 +8,10 @@
>>  struct of_phandle_args;
>>  struct reserved_mem_ops;
>>
>> +#define RESERVED_MEM_NAME_LEN  128
>> +
>>  struct reserved_mem {
>> -       const char                      *name;
>> +       char                            name[RESERVED_MEM_NAME_LEN];
>>         unsigned long                   fdt_node;
>>         const struct reserved_mem_ops   *ops;
>>         phys_addr_t                     base;
>>
>> ---
>> base-commit: bb1b0acdcd66e0d8eedee3570d249e076b89ab32
>> change-id: 20240821-save_resv_name-4f2e2cb8883b
>>
>> Best regards,
>> --
>> Jiaxun Yang <jiaxun.yang@flygoat.com>
>>

-- 
- Jiaxun

Re: [PATCH] of_reserved_mem: Save region name string into struct reserved_mem

[Rob Herring]

On Mon, Aug 26, 2024 at 9:22 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
>
>
> 在2024年8月26日八月 下午3:09，Rob Herring写道：
> > On Wed, Aug 21, 2024 at 8:51 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> >>
> >> Previously only a pointer to fdt string pool is saved to struct
> >> reserved_mem as region name.
> >>
> >> As on some architectures booting FDT will be wiped at later initialisation
> >> stages, this is breaking reserved_mem users.
> >
> > What architectures? Be specific.
>
> It's LoongArch and MIPS, I'll expand commit message.
>
> FDT might be placed in .init sections or memory not managed by kernel, thus
> it may be wiped out.
>
> >
> > Why is the FDT wiped? It should be preserved and you need it later to
> > implement kexec.
>
> So KEXEC is using kernel's self copy of FDT created by unflatten_and_copy_device_tree(),
> while reserved-mem scan is performed before copy to ensure that reserved memory
> are being tracked by memblock before possible memblock_alloc in unflatten_and_copy_device_tree().

The FDT being copied is not the same as 'wiped out'.

I'd rather update the pointers when copied or eliminate the need to
store them than introduce an arbitrary max length. Though if name
comes from the node name, then that has a max of 63 already.

Rob