powerpc/Kconfig: Force THREAD_SHIFT to at least 14 with KASAN

[PATCH] powerpc/Kconfig: Force THREAD_SHIFT to at least 14 with KASAN

Posted by Christophe Leroy 3 years, 11 months ago

Allthough 14 is the default THREAD_SHIFT when KASAN is selected,
taking an old config may keep 13 when CONFIG_EXPERT is selected.

Force it to 14 as a minimum when KASAN is selected.

Also default to 15 when KASAN on PPC64.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 3eaddb8997a9..a4427977fdf8 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -788,8 +788,10 @@ config PPC_PAGE_SHIFT
 
 config THREAD_SHIFT
 	int "Thread shift" if EXPERT
-	range 13 15
+	range 13 15 if !KASAN
+	range 14 15 if KASAN
 	default "15" if PPC_256K_PAGES
+	default "15" if PPC64 && KASAN
 	default "14" if PPC64
 	default "14" if KASAN
 	default "13"
-- 
2.35.3

Re: [PATCH] powerpc/Kconfig: Force THREAD_SHIFT to at least 14 with KASAN

Posted by Michael Ellerman 3 years, 11 months ago

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Allthough 14 is the default THREAD_SHIFT when KASAN is selected,
> taking an old config may keep 13 when CONFIG_EXPERT is selected.
>
> Force it to 14 as a minimum when KASAN is selected.
>
> Also default to 15 when KASAN on PPC64.
>
> Reported-by: Erhard Furtner <erhard_f@mailbox.org>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/Kconfig | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I was thinking of doing it in C, similar to the way arm64 handles it.

Something like below. It means we always double the stack size when
KASAN is enabled. I think it's preferable, as it will always work
regardless of whether the user has an old .config (or is bisectting)?

cheers

diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 125328d1b980..c9735f93f8e6 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -14,12 +14,17 @@
 
 #ifdef __KERNEL__
 
-#if defined(CONFIG_VMAP_STACK) && CONFIG_THREAD_SHIFT < PAGE_SHIFT
-#define THREAD_SHIFT		PAGE_SHIFT
+#ifdef CONFIG_KASAN
+#define THREAD_SHIFT		(CONFIG_THREAD_SHIFT + 1)
 #else
 #define THREAD_SHIFT		CONFIG_THREAD_SHIFT
 #endif
 
+#if defined(CONFIG_VMAP_STACK) && THREAD_SHIFT < PAGE_SHIFT
+#undef THREAD_SHIFT
+#define THREAD_SHIFT		PAGE_SHIFT
+#endif
+
 #define THREAD_SIZE		(1 << THREAD_SHIFT)
 
 /*

Re: [PATCH] powerpc/Kconfig: Force THREAD_SHIFT to at least 14 with KASAN

Posted by Christoph Hellwig 3 years, 11 months ago

On Tue, May 31, 2022 at 04:16:19PM +1000, Michael Ellerman wrote:
> I was thinking of doing it in C, similar to the way arm64 handles it.
> 
> Something like below. It means we always double the stack size when
> KASAN is enabled. I think it's preferable, as it will always work
> regardless of whether the user has an old .config (or is bisectting)?

Is there any reason to even offer the Kconfig?  It is super cryptic and
just picking the right value directly in the header would seem much
more sensible:

#if defined(CONFIG_PPC_256K_PAGES)
#define MIN_THREAD_SHIFT		15
#elif defined(CONFIG_PPC64)
#define MIN_THREAD_SHIFT		14
#else
#define MIN_THREAD_SHIFT		13
#endif

#ifdef CONFIG_KASAN
#define THREAD_SHIFT		(MIN_THREAD_SHIFT + 1)
#else
#define THREAD_SHIFT		MIN_THREAD_SHIFT
#endif

#if defined(CONFIG_VMAP_STACK) && THREAD_SHIFT < PAGE_SHIFT
#undef THREAD_SHIFT
#define THREAD_SHIFT		PAGE_SHIFT
#endif

Re: [PATCH] powerpc/Kconfig: Force THREAD_SHIFT to at least 14 with KASAN

Posted by Christophe Leroy 3 years, 11 months ago


Le 31/05/2022 à 09:41, Christoph Hellwig a écrit :
> On Tue, May 31, 2022 at 04:16:19PM +1000, Michael Ellerman wrote:
>> I was thinking of doing it in C, similar to the way arm64 handles it.
>>
>> Something like below. It means we always double the stack size when
>> KASAN is enabled. I think it's preferable, as it will always work
>> regardless of whether the user has an old .config (or is bisectting)?
> 
> Is there any reason to even offer the Kconfig?  It is super cryptic and
> just picking the right value directly in the header would seem much
> more sensible:

Looks like it  was added by commit 476134070c03 ("powerpc: Move 
THREAD_SHIFT config to Kconfig") in 2017.
https://github.com/torvalds/linux/commit/476134070c037820bd909ff6e43e0d3eae33f376

Seems like powerpc is the only architecture with it configurable via 
Kconfig.

I'd be inclined to reverting that commit, and then I'd do something 
similar to arm64:

#ifdef CONFIG_KSAN
#define KASAN_THREAD_SHIFT	1
#else
#define KASAN_THREAD_SHIFT	0
#endif

#if defined(CONFIG_PPC_256K_PAGES)
#define MIN_THREAD_SHIFT		(15 + KASAN_THREAD_SHIFT)
#elif defined(CONFIG_PPC64)
#define MIN_THREAD_SHIFT		(14 + KASAN_THREAD_SHIFT)
#else
#define MIN_THREAD_SHIFT		(13 + KASAN_THREAD_SHIFT)
#endif

#if defined(CONFIG_VMAP_STACK) && (MIN_THREAD_SHIFT < PAGE_SHIFT)
#define THREAD_SHIFT		PAGE_SHIFT
#else
#define THREAD_SHIFT		MIN_THREAD_SHIFT
#endif


And maybe the CONFIG_PPC_256K_PAGES case is already big enough and 
doesn't require the + 1 for KASAN.

> 
> #if defined(CONFIG_PPC_256K_PAGES)
> #define MIN_THREAD_SHIFT		15
> #elif defined(CONFIG_PPC64)
> #define MIN_THREAD_SHIFT		14
> #else
> #define MIN_THREAD_SHIFT		13
> #endif
> 
> #ifdef CONFIG_KASAN
> #define THREAD_SHIFT		(MIN_THREAD_SHIFT + 1)
> #else
> #define THREAD_SHIFT		MIN_THREAD_SHIFT
> #endif
> 
> #if defined(CONFIG_VMAP_STACK) && THREAD_SHIFT < PAGE_SHIFT
> #undef THREAD_SHIFT
> #define THREAD_SHIFT		PAGE_SHIFT
> #endif

Re: [PATCH] powerpc/Kconfig: Force THREAD_SHIFT to at least 14 with KASAN

Posted by Christophe Leroy 3 years, 11 months ago


Le 31/05/2022 à 08:16, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Allthough 14 is the default THREAD_SHIFT when KASAN is selected,
>> taking an old config may keep 13 when CONFIG_EXPERT is selected.
>>
>> Force it to 14 as a minimum when KASAN is selected.
>>
>> Also default to 15 when KASAN on PPC64.
>>
>> Reported-by: Erhard Furtner <erhard_f@mailbox.org>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/Kconfig | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I was thinking of doing it in C, similar to the way arm64 handles it.
> 
> Something like below. It means we always double the stack size when
> KASAN is enabled. I think it's preferable, as it will always work
> regardless of whether the user has an old .config (or is bisectting)?

Yes good idea.

I leave it to you then.

You can therefore also remove the default value for KASAN in Kconfig.

Christophe

> 
> cheers
> 
> diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
> index 125328d1b980..c9735f93f8e6 100644
> --- a/arch/powerpc/include/asm/thread_info.h
> +++ b/arch/powerpc/include/asm/thread_info.h
> @@ -14,12 +14,17 @@
>   
>   #ifdef __KERNEL__
>   
> -#if defined(CONFIG_VMAP_STACK) && CONFIG_THREAD_SHIFT < PAGE_SHIFT
> -#define THREAD_SHIFT		PAGE_SHIFT
> +#ifdef CONFIG_KASAN
> +#define THREAD_SHIFT		(CONFIG_THREAD_SHIFT + 1)
>   #else
>   #define THREAD_SHIFT		CONFIG_THREAD_SHIFT
>   #endif
>   
> +#if defined(CONFIG_VMAP_STACK) && THREAD_SHIFT < PAGE_SHIFT
> +#undef THREAD_SHIFT
> +#define THREAD_SHIFT		PAGE_SHIFT
> +#endif
> +
>   #define THREAD_SIZE		(1 << THREAD_SHIFT)
>   
>   /*
> 
>