Re: [PATCH 3/3] kasan: fix zeroing vmalloc memory with HW_TAGS

[Andrey Konovalov]

On Wed, Jun 1, 2022 at 2:28 PM Marco Elver <elver@xxxxxxxxxx> wrote:
>
> On Tue, May 31, 2022 at 05:43PM +0200, andrey.konovalov@xxxxxxxxx wrote:
> > From: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> >
> > HW_TAGS KASAN skips zeroing page_alloc allocations backing vmalloc
> > mappings via __GFP_SKIP_ZERO. Instead, these pages are zeroed via
> > kasan_unpoison_vmalloc() by passing the KASAN_VMALLOC_INIT flag.
> >
> > The problem is that __kasan_unpoison_vmalloc() does not zero pages
> > when either kasan_vmalloc_enabled() or is_vmalloc_or_module_addr() fail.
> >
> > Thus:
> >
> > 1. Change __vmalloc_node_range() to only set KASAN_VMALLOC_INIT when
> >    __GFP_SKIP_ZERO is set.
> >
> > 2. Change __kasan_unpoison_vmalloc() to always zero pages when the
> >    KASAN_VMALLOC_INIT flag is set.
> >
> > 3. Add WARN_ON() asserts to check that KASAN_VMALLOC_INIT cannot be set
> >    in other early return paths of __kasan_unpoison_vmalloc().
> >
> > Also clean up the comment in __kasan_unpoison_vmalloc.
> >
> > Fixes: 23689e91fb22 ("kasan, vmalloc: add vmalloc tagging for HW_TAGS")
> > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> > ---
> >  mm/kasan/hw_tags.c | 30 ++++++++++++++++++++++--------
> >  mm/vmalloc.c       | 10 +++++-----
> >  2 files changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index 9e1b6544bfa8..c0ec01eadf20 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -263,21 +263,31 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
> >       u8 tag;
> >       unsigned long redzone_start, redzone_size;
> >
> > -     if (!kasan_vmalloc_enabled())
> > -             return (void *)start;
> > +     if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
> > +             struct page *page;
> > +             const void *addr;
> > +
> > +             /* Initialize memory if required. */
> > +
>
> This whole block of code looks out-of-place in this function, since it's
> not at all related to unpoisoning but a fallback if KASAN-vmalloc is off
> but we still want to initialize the memory.
>
> Maybe to ease readability here I'd change it to look like:

Sounds good, will do in v2! Thanks!