Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <4c76a95aff79723de76df146a10888a5a9196faf.1654011120.git.andreyknvl@google.com>
 <bbc30451228f670abeaf1b8aad678b9f6dda4ad3.1654011120.git.andreyknvl@google.com>
 <YpdbgGjjz954Us/y@elver.google.com>
In-Reply-To: <YpdbgGjjz954Us/y@elver.google.com>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Thu, 9 Jun 2022 20:12:35 +0200
Message-ID: <CA+fCnZf7eyksP7cAVXVPdS9X=qnDTCTBMVJaqmLGUbwnbD6cdA@mail.gmail.com>
Subject: Re: [PATCH 3/3] kasan: fix zeroing vmalloc memory with HW_TAGS
To:     Marco Elver <elver@google.com>
Cc:     andrey.konovalov@linux.dev,
        Alexander Potapenko <glider@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 1, 2022 at 2:28 PM Marco Elver <elver@google.com> wrote:
>
> On Tue, May 31, 2022 at 05:43PM +0200, andrey.konovalov@linux.dev wrote:
> > From: Andrey Konovalov <andreyknvl@google.com>
> >
> > 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@google.com>
> > ---
> >  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!