[PATCH V2] mips: kernel: fix detect_memory_region() function

[PATCH V2] mips: kernel: fix detect_memory_region() function

[Shiji Yang]

The detect_memory_region() has been broken on 6.6 kernel[1]. This
patch fixes it by:
1. Do not use memcmp() on unallocated memory, as the new introduced
   fortify dynamic object size check[2] will return unexpected result.
2. Use a fixed pattern instead of a random function pointer as the
   magic value.
3. Flip magic value and double check it.
4. Enable this feature only for 32-bit CPUs. Currently, only ath79 and
   ralink CPUs are using it. And 64-bit CPU doesn't have the KSEG1ADDR
   definition.

[1] https://github.com/openwrt/openwrt/pull/14774#issuecomment-1989356746
[2] commit 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available")
Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
Tested-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
---
 arch/mips/include/asm/bootinfo.h |  2 ++
 arch/mips/kernel/setup.c         | 17 ++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/mips/include/asm/bootinfo.h b/arch/mips/include/asm/bootinfo.h
index 2128ba903391..8516c11916a4 100644
--- a/arch/mips/include/asm/bootinfo.h
+++ b/arch/mips/include/asm/bootinfo.h
@@ -93,7 +93,9 @@ const char *get_system_type(void);
 
 extern unsigned long mips_machtype;
 
+#ifndef CONFIG_64BIT
 extern void detect_memory_region(phys_addr_t start, phys_addr_t sz_min,  phys_addr_t sz_max);
+#endif
 
 extern void prom_init(void);
 extern void prom_free_prom_memory(void);
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 12a1a4ffb602..3a3bc1b7e62e 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -86,21 +86,27 @@ static struct resource bss_resource = { .name = "Kernel bss", };
 unsigned long __kaslr_offset __ro_after_init;
 EXPORT_SYMBOL(__kaslr_offset);
 
-static void *detect_magic __initdata = detect_memory_region;
-
 #ifdef CONFIG_MIPS_AUTO_PFN_OFFSET
 unsigned long ARCH_PFN_OFFSET;
 EXPORT_SYMBOL(ARCH_PFN_OFFSET);
 #endif
 
+#ifndef CONFIG_64BIT
+static u32 detect_magic __initdata;
+#define MIPS_MEM_TEST_PATTERN		0xaa5555aa
+
 void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_addr_t sz_max)
 {
-	void *dm = &detect_magic;
+	void *dm = (void *)KSEG1ADDR(&detect_magic);
 	phys_addr_t size;
 
 	for (size = sz_min; size < sz_max; size <<= 1) {
-		if (!memcmp(dm, dm + size, sizeof(detect_magic)))
-			break;
+		__raw_writel(MIPS_MEM_TEST_PATTERN, dm);
+		if (__raw_readl(dm) == __raw_readl(dm + size)) {
+			__raw_writel(~MIPS_MEM_TEST_PATTERN, dm);
+			if (__raw_readl(dm) == __raw_readl(dm + size))
+				break;
+		}
 	}
 
 	pr_debug("Memory: %lluMB of RAM detected at 0x%llx (min: %lluMB, max: %lluMB)\n",
@@ -111,6 +117,7 @@ void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_add
 
 	memblock_add(start, size);
 }
+#endif /* CONFIG_64BIT */
 
 /*
  * Manage initrd
-- 
2.45.1

Re: [PATCH V2] mips: kernel: fix detect_memory_region() function

[Jiaxun Yang]

在2024年6月25日六月 上午2:44，Shiji Yang写道：
> The detect_memory_region() has been broken on 6.6 kernel[1]. This
> patch fixes it by:
> 1. Do not use memcmp() on unallocated memory, as the new introduced
>    fortify dynamic object size check[2] will return unexpected result.
> 2. Use a fixed pattern instead of a random function pointer as the
>    magic value.
> 3. Flip magic value and double check it.
> 4. Enable this feature only for 32-bit CPUs. Currently, only ath79 and
>    ralink CPUs are using it. And 64-bit CPU doesn't have the KSEG1ADDR
>    definition.

Hi Shiji,

Thanks for your patch.

Please don't break 64bit system.
Use CKSEG1ADDR_OR_64BIT instead.

Thanks
- Jiaxun

>
> [1] 
> https://github.com/openwrt/openwrt/pull/14774#issuecomment-1989356746
> [2] commit 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() 
> when available")
> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> Tested-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
> ---
>  arch/mips/include/asm/bootinfo.h |  2 ++
>  arch/mips/kernel/setup.c         | 17 ++++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/mips/include/asm/bootinfo.h b/arch/mips/include/asm/bootinfo.h
> index 2128ba903391..8516c11916a4 100644
> --- a/arch/mips/include/asm/bootinfo.h
> +++ b/arch/mips/include/asm/bootinfo.h
> @@ -93,7 +93,9 @@ const char *get_system_type(void);
> 
>  extern unsigned long mips_machtype;
> 
> +#ifndef CONFIG_64BIT
>  extern void detect_memory_region(phys_addr_t start, phys_addr_t 
> sz_min,  phys_addr_t sz_max);
> +#endif
> 
>  extern void prom_init(void);
>  extern void prom_free_prom_memory(void);
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 12a1a4ffb602..3a3bc1b7e62e 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -86,21 +86,27 @@ static struct resource bss_resource = { .name = 
> "Kernel bss", };
>  unsigned long __kaslr_offset __ro_after_init;
>  EXPORT_SYMBOL(__kaslr_offset);
> 
> -static void *detect_magic __initdata = detect_memory_region;
> -
>  #ifdef CONFIG_MIPS_AUTO_PFN_OFFSET
>  unsigned long ARCH_PFN_OFFSET;
>  EXPORT_SYMBOL(ARCH_PFN_OFFSET);
>  #endif
> 
> +#ifndef CONFIG_64BIT
> +static u32 detect_magic __initdata;
> +#define MIPS_MEM_TEST_PATTERN		0xaa5555aa
> +
>  void __init detect_memory_region(phys_addr_t start, phys_addr_t 
> sz_min, phys_addr_t sz_max)
>  {
> -	void *dm = &detect_magic;
> +	void *dm = (void *)KSEG1ADDR(&detect_magic);
>  	phys_addr_t size;
> 
>  	for (size = sz_min; size < sz_max; size <<= 1) {
> -		if (!memcmp(dm, dm + size, sizeof(detect_magic)))
> -			break;
> +		__raw_writel(MIPS_MEM_TEST_PATTERN, dm);
> +		if (__raw_readl(dm) == __raw_readl(dm + size)) {
> +			__raw_writel(~MIPS_MEM_TEST_PATTERN, dm);
> +			if (__raw_readl(dm) == __raw_readl(dm + size))
> +				break;
> +		}
>  	}
> 
>  	pr_debug("Memory: %lluMB of RAM detected at 0x%llx (min: %lluMB, max: 
> %lluMB)\n",
> @@ -111,6 +117,7 @@ void __init detect_memory_region(phys_addr_t start, 
> phys_addr_t sz_min, phys_add
> 
>  	memblock_add(start, size);
>  }
> +#endif /* CONFIG_64BIT */
> 
>  /*
>   * Manage initrd
> -- 
> 2.45.1

-- 
- Jiaxun

Re: [PATCH V2] mips: kernel: fix detect_memory_region() function

[Shiji Yang]

On Tue, 25 Jun 2024 02:58:54 +0100, Jiaxun Yang wrote:
>> The detect_memory_region() has been broken on 6.6 kernel[1]. This
>> patch fixes it by:
>> 1. Do not use memcmp() on unallocated memory, as the new introduced
>>    fortify dynamic object size check[2] will return unexpected result.
>> 2. Use a fixed pattern instead of a random function pointer as the
>>    magic value.
>> 3. Flip magic value and double check it.
>> 4. Enable this feature only for 32-bit CPUs. Currently, only ath79 and
>>    ralink CPUs are using it. And 64-bit CPU doesn't have the KSEG1ADDR
>>    definition.
>
>Hi Shiji,
>
>Thanks for your patch.
>
>Please don't break 64bit system.
>Use CKSEG1ADDR_OR_64BIT instead.
>
>Thanks
>- Jiaxun

Thanks. I've updated and tested it in v2 patch.
https://lore.kernel.org/linux-mips/TYCP286MB0895F65439037ED134FEA7DDBCD12@TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM/

Regards,
Shiji Yang

Re: [PATCH V2] mips: kernel: fix detect_memory_region() function

[Thomas Bogendoerfer]

On Tue, Jun 25, 2024 at 09:44:44AM +0800, Shiji Yang wrote:
>  	for (size = sz_min; size < sz_max; size <<= 1) {
> -		if (!memcmp(dm, dm + size, sizeof(detect_magic)))
> -			break;
> +		__raw_writel(MIPS_MEM_TEST_PATTERN, dm);
> +		if (__raw_readl(dm) == __raw_readl(dm + size)) {
> +			__raw_writel(~MIPS_MEM_TEST_PATTERN, dm);
> +			if (__raw_readl(dm) == __raw_readl(dm + size))
> +				break;
> +		}

you are testing memory, so just use pointers. __raw_readl and __raw_writel
are for io regions (which should be ioremppaed before usage).

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH V2] mips: kernel: fix detect_memory_region() function

[Thomas Bogendoerfer]

On Tue, Jun 25, 2024 at 09:44:44AM +0800, Shiji Yang wrote:
> The detect_memory_region() has been broken on 6.6 kernel[1]. This
> patch fixes it by:
> 1. Do not use memcmp() on unallocated memory, as the new introduced
>    fortify dynamic object size check[2] will return unexpected result.

hmm, so there should a new way for doing memory probing without
triggering this fortify check. How do other platforms deal with this ?

> 2. Use a fixed pattern instead of a random function pointer as the
>    magic value.

why is this better ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH V2] mips: kernel: fix detect_memory_region() function

[Shiji Yang]

On Tue, 25 Jun 2024 09:46:34 +0200, Thomas Bogendoerfer wrote:
>>  	for (size = sz_min; size < sz_max; size <<= 1) {
>> -		if (!memcmp(dm, dm + size, sizeof(detect_magic)))
>> -			break;
>> +		__raw_writel(MIPS_MEM_TEST_PATTERN, dm);
>> +		if (__raw_readl(dm) == __raw_readl(dm + size)) {
>> +			__raw_writel(~MIPS_MEM_TEST_PATTERN, dm);
>> +			if (__raw_readl(dm) == __raw_readl(dm + size))
>> +				break;
>> +		}
>
>you are testing memory, so just use pointers. __raw_readl and __raw_writel
>are for io regions (which should be ioremppaed before usage).

Fixed in v3:
https://lore.kernel.org/linux-mips/TYCP286MB0895F65439037ED134FEA7DDBCD12@TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM/


>
>> The detect_memory_region() has been broken on 6.6 kernel[1]. This
>> patch fixes it by:
>> 1. Do not use memcmp() on unallocated memory, as the new introduced
>>    fortify dynamic object size check[2] will return unexpected result.
>
>hmm, so there should a new way for doing memory probing without
>triggering this fortify check. How do other platforms deal with this ?

I guess __builtin_memcmp() should work. But this patch also fixes a
potential wrong memory size issue[1] by fliping magic number and
double checking it. And there is no need to ues memcmp() to check
an u32 variable.

It seems that other ARCHs directly read some registers to judge
the memory size. However, the theory in mips ARCH is checking the
overlapping memory address.

>
>> 2. Use a fixed pattern instead of a random function pointer as the
>>    magic value.
>
>why is this better ?

Just engineering experience. Byte 0xaa/0x55 has the largest information
entropy and is widely used in memory testing. Most codes in v2 patch
are copied from 'arch/mips/ralink/mt7621.c'.

>
>Thomas.
>

[1] https://github.com/openwrt/openwrt/pull/14282


Regards,
Shiji Yang

Re: [PATCH V2] mips: kernel: fix detect_memory_region() function

[kernel test robot]

Hi Shiji,

kernel test robot noticed the following build warnings:

[auto build test WARNING on soc/for-next]
[also build test WARNING on linus/master v6.10-rc5 next-20240627]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shiji-Yang/mips-kernel-fix-detect_memory_region-function/20240626-070033
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/TYCP286MB089598ABD1E2F66003D71EB8BCD52%40TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM
patch subject: [PATCH V2] mips: kernel: fix detect_memory_region() function
config: mips-randconfig-r113-20240628 (https://download.01.org/0day-ci/archive/20240628/202406282354.0hGuOKo0-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20240628/202406282354.0hGuOKo0-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/202406282354.0hGuOKo0-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> arch/mips/kernel/setup.c:104:53: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *mem @@     got void *dm @@
   arch/mips/kernel/setup.c:104:53: sparse:     expected void volatile [noderef] __iomem *mem
   arch/mips/kernel/setup.c:104:53: sparse:     got void *dm
>> arch/mips/kernel/setup.c:105:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got void *dm @@
   arch/mips/kernel/setup.c:105:33: sparse:     expected void const volatile [noderef] __iomem *mem
   arch/mips/kernel/setup.c:105:33: sparse:     got void *dm
>> arch/mips/kernel/setup.c:105:55: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got void * @@
   arch/mips/kernel/setup.c:105:55: sparse:     expected void const volatile [noderef] __iomem *mem
   arch/mips/kernel/setup.c:105:55: sparse:     got void *
   arch/mips/kernel/setup.c:106:62: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *mem @@     got void *dm @@
   arch/mips/kernel/setup.c:106:62: sparse:     expected void volatile [noderef] __iomem *mem
   arch/mips/kernel/setup.c:106:62: sparse:     got void *dm
   arch/mips/kernel/setup.c:107:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got void *dm @@
   arch/mips/kernel/setup.c:107:41: sparse:     expected void const volatile [noderef] __iomem *mem
   arch/mips/kernel/setup.c:107:41: sparse:     got void *dm
   arch/mips/kernel/setup.c:107:63: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got void * @@
   arch/mips/kernel/setup.c:107:63: sparse:     expected void const volatile [noderef] __iomem *mem
   arch/mips/kernel/setup.c:107:63: sparse:     got void *
   arch/mips/kernel/setup.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, ...):
   include/linux/page-flags.h:240:46: sparse: sparse: self-comparison always evaluates to false
   include/linux/page-flags.h:240:46: sparse: sparse: self-comparison always evaluates to false

vim +104 arch/mips/kernel/setup.c

    97	
    98	void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_addr_t sz_max)
    99	{
   100		void *dm = (void *)KSEG1ADDR(&detect_magic);
   101		phys_addr_t size;
   102	
   103		for (size = sz_min; size < sz_max; size <<= 1) {
 > 104			__raw_writel(MIPS_MEM_TEST_PATTERN, dm);
 > 105			if (__raw_readl(dm) == __raw_readl(dm + size)) {
   106				__raw_writel(~MIPS_MEM_TEST_PATTERN, dm);
   107				if (__raw_readl(dm) == __raw_readl(dm + size))
   108					break;
   109			}
   110		}
   111	
   112		pr_debug("Memory: %lluMB of RAM detected at 0x%llx (min: %lluMB, max: %lluMB)\n",
   113			((unsigned long long) size) / SZ_1M,
   114			(unsigned long long) start,
   115			((unsigned long long) sz_min) / SZ_1M,
   116			((unsigned long long) sz_max) / SZ_1M);
   117	
   118		memblock_add(start, size);
   119	}
   120	#endif /* CONFIG_64BIT */
   121	

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