Re: [PATCH v4 1/2] kasan/hw-tags: introduce kasan.write_only option

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Andrey,

[...]

> >  static inline const char *kasan_mode_info(void)
> >  {
> >         if (kasan_mode == KASAN_MODE_ASYNC)
> > @@ -257,15 +283,26 @@ void __init kasan_init_hw_tags(void)
> >                 break;
> >         }
> >
> > +       switch (kasan_arg_write_only) {
> > +       case KASAN_ARG_WRITE_ONLY_DEFAULT:
>
> Let's keep this part similar to the other parameters for consistency:
>
> /* Default is specified by kasan_flag_write_only definition. */
> break;

Okay.

> > +       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();
> >
> >         /* 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));
> >  }
> >
> >  #ifdef CONFIG_KASAN_VMALLOC
> > @@ -392,6 +429,13 @@ void kasan_enable_hw_tags(void)
> >                 hw_enable_tag_checks_asymm();
> >         else
> >                 hw_enable_tag_checks_sync();
> > +
> > +       if (kasan_arg_write_only == KASAN_ARG_WRITE_ONLY_ON &&
>
> We already set kasan_flag_write_only by this point, right? Let's check
> that one then.

Yes. if user specifies the write_only == on.
>
> > +           hw_enable_tag_checks_write_only()) {
> > +               kasan_arg_write_only == KASAN_ARG_WRITE_ONLY_OFF;
>
> Typo in == in the line above. But also I think we can just drop the
> line: kasan_arg_write_only is KASAN_ARG_WRITE_ONLY_ON after all, it's
> just not supported and thus kasan_flag_write_only is set to false to
> reflect that.

Sorry :\ I've missed this fix from patch 3... this should be == to =.

However, we couldn't remove kasan_arg_write_only check in condition.
If one of cpu get failed to hw_enable_tag_checks_write_only() then
By changing this with KASAN_ARG_WRITE_ONLY_OFF, It prevent to call
hw_eanble_tag_checks_write_only() in other cpu.

As you said, kasan_flag_write_only reflects the state.
But like other option, I keep the condition to call the hw_enable_xxx()
by checking the "argments" and keep the "hw enable state" with
kasan_flag_write_only.

so let me change == to = only in here.

>
> > +               kasan_flag_write_only = false;
> > +               pr_warn_once("System doesn't support write-only option. Disable it\n");
>
> Let's do pr_err like the rest of KASAN code. And:
>
> pr_err_once("write-only mode is not supported and thus not enabled\n");

Okay. Thanks for suggestion.

>
>
>
> > +       }
> >  }
> >
> >  #if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST)
> > @@ -404,4 +448,10 @@ VISIBLE_IF_KUNIT void kasan_force_async_fault(void)
> >  }
> >  EXPORT_SYMBOL_IF_KUNIT(kasan_force_async_fault);
> >
> > +VISIBLE_IF_KUNIT bool kasan_write_only_enabled(void)
> > +{
> > +       return kasan_flag_write_only;
> > +}
> > +EXPORT_SYMBOL_IF_KUNIT(kasan_write_only_enabled);
> > +
> >  #endif
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index 129178be5e64..c1490136c96b 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -428,6 +428,7 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
> >  #define hw_enable_tag_checks_sync()            arch_enable_tag_checks_sync()
> >  #define hw_enable_tag_checks_async()           arch_enable_tag_checks_async()
> >  #define hw_enable_tag_checks_asymm()           arch_enable_tag_checks_asymm()
> > +#define hw_enable_tag_checks_write_only()      arch_enable_tag_checks_write_only()
> >  #define hw_suppress_tag_checks_start()         arch_suppress_tag_checks_start()
> >  #define hw_suppress_tag_checks_stop()          arch_suppress_tag_checks_stop()
> >  #define hw_force_async_tag_fault()             arch_force_async_tag_fault()
> > @@ -437,11 +438,17 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
> >                         arch_set_mem_tag_range((addr), (size), (tag), (init))
> >
> >  void kasan_enable_hw_tags(void);
> > +bool kasan_write_only_enabled(void);
>
> This should go next to kasan_force_async_fault().
>
> >
> >  #else /* CONFIG_KASAN_HW_TAGS */
> >
> >  static inline void kasan_enable_hw_tags(void) { }
> >
> > +static inline bool kasan_write_only_enabled(void)
> > +{
> > +       return false;
> > +}
>
> And this too.

Right. I'll move them. Thanks!

Thanks!

--
Sincerely,
Yeoreum Yun