Re: [PATCH v2 00/11] kasan: unify kasan_arch_is_ready with kasan_enabled

[Christophe Leroy]

Le 01/07/2025 à 12:15, Heiko Carstens a écrit :
> > > > Another thing that needs careful consideration is whether it's
> > > > possible to combine kasan_arch_is_ready() and kasan_enabled() into the
> > > > same check logically at all. There's one issue mentioned in [1]:
> > >
> > > Hello,
> > > I've removed kasan_arch_is_ready() at all in this series:
> > > [PATCH v2 11/11] kasan: replace kasan_arch_is_ready with kasan_enabled
> > >
> > > Is it not what's expected by unification?
> >
> > I guess the issue description diverged a bit from what needs to be
> > done, sorry about that.
> >
> > The core 2 things I wanted to address with the unification are:
> >
> > 1. Avoid spraying kasan_arch_is_ready() throughout the KASAN
> > implementation and move these checks into include/linux/kasan.h (and
> > add __wrappers when required).
> >
> > 2. Avoid architectures redefining the same kasan_enabled global
> > variable/static key.
> >
> > Initially, I thought that s/kasan_arch_is_ready/kasan_enabled + simply
> > moving the calls into affected include/linux/kasan.h functions would
> > be enough. But then, based on [1], turns out it's not that simple.
> >
> > So now, I think we likely still need two separate checks/flags:
> > kasan_enabled() that controls whether KASAN is enabled at all and
> > kasan_arch_is_ready() that gets turned on by kasan_init() when shadow
> > is initialized (should we rename it to kasan_shadow_initialized()?).
> > But then we can still move kasan_arch_is_ready() into
> > include/linux/kasan.h and use the proper combination of checks for
> > each affected function before calling __wrappers. And we can still
> > remove the duplicated flags/keys code from the arch code.
>
> FWIW, as Alexander Gordeev already mentioned: this series breaks s390,
> since the static_branch_enable() call in kasan_init_generic() is now
> called way too early, and it isn't necessary at all. Which, as far as
> I understand, may be the case for other architectures as well. s390
> sets up the required KASAN mappings in the decompressor and can start
> with KASAN enabled nearly from the beginning.
>
> So something like below on top of this series would address
> that. Given that this series is about to be reworked this is just for
> illustration :)

I had the same kind of comment on powerpc/32. Allthough this series work 
on powerpc32 as is, it is overkill because it adds code and data for 
static branches for no real benefit.

Your patch below is simpler than what I proposed, but it keeps the 
static branches so the overhead remains.

I also proposed a change, it goes further by removing the static branch 
for architectures that don't need it, see 
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/20250626153147.145312-1-snovitoll@xxxxxxxxx/#3537388 
. Feedback welcome.

Christophe

> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 0c16dc443e2f..c2f51ac39a91 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -172,6 +172,7 @@ config S390
>   	select HAVE_ARCH_JUMP_LABEL
>   	select HAVE_ARCH_JUMP_LABEL_RELATIVE
>   	select HAVE_ARCH_KASAN
> +	select HAVE_ARCH_KASAN_EARLY
>   	select HAVE_ARCH_KASAN_VMALLOC
>   	select HAVE_ARCH_KCSAN
>   	select HAVE_ARCH_KMSAN
> diff --git a/include/linux/kasan-enabled.h b/include/linux/kasan-enabled.h
> index 2436eb45cfee..049270a2269f 100644
> --- a/include/linux/kasan-enabled.h
> +++ b/include/linux/kasan-enabled.h
> @@ -10,7 +10,11 @@
>    * Global runtime flag. Starts ‘false’; switched to ‘true’ by
>    * the appropriate kasan_init_*() once KASAN is fully initialized.
>    */
> +#ifdef CONFIG_HAVE_ARCH_KASAN_EARLY
> +DECLARE_STATIC_KEY_TRUE(kasan_flag_enabled);
> +#else
>   DECLARE_STATIC_KEY_FALSE(kasan_flag_enabled);
> +#endif
>   
>   static __always_inline bool kasan_enabled(void)
>   {
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index f82889a830fa..1407374e83b9 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -4,6 +4,13 @@
>   config HAVE_ARCH_KASAN
>   	bool
>   
> +config HAVE_ARCH_KASAN_EARLY
> +	bool
> +	help
> +	  Architectures should select this if KASAN mappings are setup in
> +	  the decompressor and when the kernel can run very early with
> +	  KASAN enabled.
> +
>   config HAVE_ARCH_KASAN_SW_TAGS
>   	bool
>   
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 0f3648335a6b..2aae0ce659b4 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -36,7 +36,11 @@
>    * Definition of the unified static key declared in kasan-enabled.h.
>    * This provides consistent runtime enable/disable across all KASAN modes.
>    */
> +#ifdef CONFIG_HAVE_ARCH_KASAN_EARLY
> +DEFINE_STATIC_KEY_TRUE(kasan_flag_enabled);
> +#else
>   DEFINE_STATIC_KEY_FALSE(kasan_flag_enabled);
> +#endif
>   EXPORT_SYMBOL(kasan_flag_enabled);
>   
>   struct slab *kasan_addr_to_slab(const void *addr)
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index a3b112868be7..455376d5f1c3 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -42,7 +42,8 @@
>    */
>   void __init kasan_init_generic(void)
>   {
> -	static_branch_enable(&kasan_flag_enabled);
> +	if (!IS_ENABLED(CONFIG_HAVE_ARCH_KASAN_EARLY))
> +		static_branch_enable(&kasan_flag_enabled);
>   
>   	pr_info("KernelAddressSanitizer initialized (generic)\n");
>   }