Hi Will,
> On Wed, Sep 03, 2025 at 04:00:19PM +0100, Yeoreum Yun wrote:
> > Since Armv8.9, FEATURE_MTE_STORE_ONLY feature is introduced to restrict
> > raise of tag check fault on store operation only.
> > Introcude KASAN write only mode based on this feature.
>
> Typo ^^
Thanks.
>
> >
> > KASAN write only mode restricts KASAN checks operation for write only and
> > omits the checks for fetch/read operations when accessing memory.
> > So it might be used not only debugging enviroment but also normal
> > enviroment to check memory safty.
> >
> > This features can be controlled with "kasan.write_only" arguments.
> > When "kasan.write_only=on", KASAN checks write operation only otherwise
> > KASAN checks all operations.
> >
> > This changes the MTE_STORE_ONLY feature as BOOT_CPU_FEATURE like
> > ARM64_MTE_ASYMM so that makes it initialise in kasan_init_hw_tags()
> > with other function together.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum....@arm.com>
> > Reviewed-by: Andrey Konovalov <andreyk...@gmail.com>
> > Reviewed-by: Catalin Marinas <catalin.mari...@arm.com>
> > ---
> > Documentation/dev-tools/kasan.rst | 3 ++
> > arch/arm64/include/asm/memory.h | 1 +
> > arch/arm64/include/asm/mte-kasan.h | 6 +++
> > arch/arm64/kernel/cpufeature.c | 2 +-
> > arch/arm64/kernel/mte.c | 18 ++++++++
> > mm/kasan/hw_tags.c | 70 +++++++++++++++++++++++++++++-
> > mm/kasan/kasan.h | 7 +++
> > 7 files changed, 104 insertions(+), 3 deletions(-)
>
> [...]
>
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index 9a6927394b54..d5b5fb47d52b 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -41,9 +41,16 @@ enum kasan_arg_vmalloc {
> > KASAN_ARG_VMALLOC_ON,
> > };
> >
> > +enum kasan_arg_write_only {
> > + KASAN_ARG_WRITE_ONLY_DEFAULT,
> > + KASAN_ARG_WRITE_ONLY_OFF,
> > + KASAN_ARG_WRITE_ONLY_ON,
> > +};
> > +
> > static enum kasan_arg kasan_arg __ro_after_init;
> > static enum kasan_arg_mode kasan_arg_mode __ro_after_init;
> > static enum kasan_arg_vmalloc kasan_arg_vmalloc __initdata;
> > +static enum kasan_arg_write_only kasan_arg_write_only __ro_after_init;
> >
> > /*
> > * Whether KASAN is enabled at all.
> > @@ -67,6 +74,9 @@ DEFINE_STATIC_KEY_FALSE(kasan_flag_vmalloc);
> > #endif
> > EXPORT_SYMBOL_GPL(kasan_flag_vmalloc);
> >
> > +/* Whether to check write accesses only. */
> > +static bool kasan_flag_write_only = false;
> > +
> > #define PAGE_ALLOC_SAMPLE_DEFAULT 1
> > #define PAGE_ALLOC_SAMPLE_ORDER_DEFAULT 3
> >
> > @@ -141,6 +151,23 @@ static int __init early_kasan_flag_vmalloc(char *arg)
> > }
> > early_param("kasan.vmalloc", early_kasan_flag_vmalloc);
> >
> > +/* kasan.write_only=off/on */
> > +static int __init early_kasan_flag_write_only(char *arg)
> > +{
> > + if (!arg)
> > + return -EINVAL;
> > +
> > + if (!strcmp(arg, "off"))
> > + kasan_arg_write_only = KASAN_ARG_WRITE_ONLY_OFF;
> > + else if (!strcmp(arg, "on"))
> > + kasan_arg_write_only = KASAN_ARG_WRITE_ONLY_ON;
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +early_param("kasan.write_only", early_kasan_flag_write_only);
> > +
> > static inline const char *kasan_mode_info(void)
> > {
> > if (kasan_mode == KASAN_MODE_ASYNC)
> > @@ -257,15 +284,28 @@ void __init kasan_init_hw_tags(void)
> > break;
> > }
> >
> > + switch (kasan_arg_write_only) {
> > + case KASAN_ARG_WRITE_ONLY_DEFAULT:
> > + /* Default is specified by kasan_flag_write_only definition. */
> > + break;
> > + case KASAN_ARG_WRITE_ONLY_OFF:
> > + kasan_flag_write_only = false;
> > + break;
> > + case KASAN_ARG_WRITE_ONLY_ON:
> > + kasan_flag_write_only = true;
> > + break;
> > + }
> > +
> > kasan_init_tags();
>
> I'm probably missing something here, but why have 'enum
> kasan_arg_write_only' at all? What stops you from setting
> 'kasan_flag_write_only' directly from early_kasan_flag_write_only()?
>
> This all looks weirdly over-engineered, as though 'kasan_flag_write_only'
> is expected to be statically initialised to something other than 'false'.
For the conherent pattern for other options.
Since other options manage arg value and internal state separately,
I just followed former ancestor.
>
> > /* KASAN is now initialized, enable it. */
> > static_branch_enable(&kasan_flag_enabled);
> >
> > - pr_info("KernelAddressSanitizer initialized (hw-tags, mode=%s,
> > vmalloc=%s, stacktrace=%s)\n",
> > + pr_info("KernelAddressSanitizer initialized (hw-tags, mode=%s,
> > vmalloc=%s, stacktrace=%s, write_only=%s)\n",
> > kasan_mode_info(),
> > str_on_off(kasan_vmalloc_enabled()),
> > - str_on_off(kasan_stack_collection_enabled()));
> > + str_on_off(kasan_stack_collection_enabled()),
> > + str_on_off(kasan_arg_write_only));
>
> It's also confusing, because now you appear to be passing the funny new
> 'enum' type to str_on_off(), which expects a bool.
Oops. This is my mistake from v3 :(
Thanks to point out ///
--
Sincerely,
Yeoreum Yun
