Re: [PATCH v3 1/3] kasan: use separate (un)poison implementation for integrated init

[Peter Collingbourne <pcc@google.com>]

On Wed, May 26, 2021 at 3:12 AM Marco Elver <elver@google.com> wrote:
>
> On Wed, May 12, 2021 at 01:09PM -0700, Peter Collingbourne wrote:
> [...]
> > +void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags);
> > +void kasan_free_pages(struct page *page, unsigned int order);
> > +
> >  #else /* CONFIG_KASAN_HW_TAGS */
> >
> >  static inline bool kasan_enabled(void)
> >  {
> > +#ifdef CONFIG_KASAN
> >       return true;
> > +#else
> > +     return false;
> > +#endif
> >  }
>
> Just
>
>         return IS_ENABLED(CONFIG_KASAN);

Will do.

> >  static inline bool kasan_has_integrated_init(void)
> > @@ -113,8 +113,30 @@ static inline bool kasan_has_integrated_init(void)
> >       return false;
> >  }
> >
> > +static __always_inline void kasan_alloc_pages(struct page *page,
> > +                                           unsigned int order, gfp_t flags)
> > +{
> > +     /* Only available for integrated init. */
> > +     BUILD_BUG();
> > +}
> > +
> > +static __always_inline void kasan_free_pages(struct page *page,
> > +                                          unsigned int order)
> > +{
> > +     /* Only available for integrated init. */
> > +     BUILD_BUG();
> > +}
>
> This *should* always work, as long as the compiler optimizes everything
> like we expect.

Yeah, as I mentioned to Catalin on an earlier revision I'm not a fan
of relying on the compiler optimizing this away, but it looks like
we're already relying on this elsewhere in the kernel.

> But: In this case, I think this is sign that the interface design can be
> improved. Can we just make kasan_{alloc,free}_pages() return a 'bool
> __must_check' to indicate if kasan takes care of init?

I considered a number of different approaches including something like
that before settling on the one in this patch. One consideration was
that we should avoid involving KASAN in normal execution as much as
possible, in order to make the normal code path as comprehensible as
possible. With an approach where alloc/free return a bool the reader
needs to understand what the KASAN alloc/free functions do in the
normal case. Whereas with an approach where an "accessor" function on
the KASAN side returns a bool, it's more obvious that the code has a
"normal path" and a "KASAN path", and readers who only care about the
normal path can ignore the KASAN path.

Does that make sense? I don't feel too strongly so I can change
alloc/free to return a bool if you don't agree.

> The variants here would simply return kasan_has_integrated_init().
>
> That way, there'd be no need for the BUILD_BUG()s and the interface
> becomes harder to misuse by design.
>
> Also, given that kasan_{alloc,free}_pages() initializes memory, this is
> an opportunity to just give them a better name. Perhaps
>
>         /* Returns true if KASAN took care of initialization, false otherwise. */
>         bool __must_check kasan_alloc_pages_try_init(struct page *page, unsigned int order, gfp_t flags);
>         bool __must_check kasan_free_pages_try_init(struct page *page, unsigned int order);

I considered changing the name but concluded that we probably
shouldn't try to pack too much information into the name. With a code
flow like:

if (kasan_has_integrated_init()) {
  kasan_alloc_pages();
} else {
  kernel_init_free_pages();
}

I think it's probably clear enough that kasan_alloc_pages() is doing
the stuff in kernel_init_free_pages() as well.

> [...]
> > -     init = want_init_on_free();
> > -     if (init && !kasan_has_integrated_init())
> > -             kernel_init_free_pages(page, 1 << order);
> > -     kasan_free_nondeferred_pages(page, order, init, fpi_flags);
> > +     if (kasan_has_integrated_init()) {
> > +             if (!skip_kasan_poison)
> > +                     kasan_free_pages(page, order);
>
> I think kasan_free_pages() could return a bool, and this would become
>
>         if (skip_kasan_poison || !kasan_free_pages(...)) {
>                 ...
>
> > +     } else {
> > +             bool init = want_init_on_free();
> > +
> > +             if (init)
> > +                     kernel_init_free_pages(page, 1 << order);
> > +             if (!skip_kasan_poison)
> > +                     kasan_poison_pages(page, order, init);
> > +     }
> >
> >       /*
> >        * arch_free_page() can make the page's contents inaccessible.  s390
> > @@ -2324,8 +2324,6 @@ static bool check_new_pages(struct page *page, unsigned int order)
> >  inline void post_alloc_hook(struct page *page, unsigned int order,
> >                               gfp_t gfp_flags)
> >  {
> > -     bool init;
> > -
> >       set_page_private(page, 0);
> >       set_page_refcounted(page);
> >
> > @@ -2344,10 +2342,16 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> >        * kasan_alloc_pages and kernel_init_free_pages must be
> >        * kept together to avoid discrepancies in behavior.
> >        */
> > -     init = !want_init_on_free() && want_init_on_alloc(gfp_flags);
> > -     kasan_alloc_pages(page, order, init);
> > -     if (init && !kasan_has_integrated_init())
> > -             kernel_init_free_pages(page, 1 << order);
> > +     if (kasan_has_integrated_init()) {
> > +             kasan_alloc_pages(page, order, gfp_flags);
>
> It looks to me that kasan_alloc_pages() could return a bool, and this
> would become
>
>         if (!kasan_alloc_pages(...)) {
>                 ...
>
> > +     } else {
> > +             bool init =
> > +                     !want_init_on_free() && want_init_on_alloc(gfp_flags);
> > +
>
> [ No need for line-break (for cases like this the kernel is fine with up
> to 100 cols if it improves readability). ]

Okay, I'll make that change.

Peter