| Message ID | TYCP286MB089598ABD1E2F66003D71EB8BCD52@TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] mips: kernel: fix detect_memory_region() function | expand |
在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
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.
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.
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
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
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
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
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(-)