MIME-Version: 1.0
References: <cover.1638308023.git.andreyknvl@google.com> <984104c118a451fc4afa2eadb7206065f13b7af2.1638308023.git.andreyknvl@google.com>
 <CAG_fn=U71Yn-qCGMBR=_uOt0QCEu9skGzhgRBJjpkQCjZ=dKiA@mail.gmail.com>
In-Reply-To: <CAG_fn=U71Yn-qCGMBR=_uOt0QCEu9skGzhgRBJjpkQCjZ=dKiA@mail.gmail.com>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Mon, 6 Dec 2021 22:09:38 +0100
Message-ID: <CA+fCnZfto82vg3vGkZGNxJKOOqsOp_bpmHEd0Z350PfPJ7Y=1w@mail.gmail.com>
Subject: Re: [PATCH 08/31] kasan, page_alloc: refactor init checks in post_alloc_hook
To:     Alexander Potapenko <glider@google.com>
Cc:     andrey.konovalov@linux.dev, Marco Elver <elver@google.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Peter Collingbourne <pcc@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        Will Deacon <will@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Evgenii Stepanov <eugenis@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 2, 2021 at 5:14 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Nov 30, 2021 at 10:41 PM <andrey.konovalov@linux.dev> wrote:
> >
> > From: Andrey Konovalov <andreyknvl@google.com>
> >
> > This patch separates code for zeroing memory from the code clearing tags
> > in post_alloc_hook().
> >
> > This patch is not useful by itself but makes the simplifications in
> > the following patches easier to follow.
> >
> > This patch does no functional changes.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > ---
> >  mm/page_alloc.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 2ada09a58e4b..0561cdafce36 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2406,19 +2406,21 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> >                 kasan_alloc_pages(page, order, gfp_flags);
> >         } else {
> >                 bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags);
> > +               bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS);
> >
> >                 kasan_unpoison_pages(page, order, init);
> >
> > -               if (init) {
> > -                       if (gfp_flags & __GFP_ZEROTAGS) {
> > -                               int i;
> > +               if (init_tags) {
> > +                       int i;
> >
> > -                               for (i = 0; i < 1 << order; i++)
> > -                                       tag_clear_highpage(page + i);
> > -                       } else {
> > -                               kernel_init_free_pages(page, 1 << order);
> > -                       }
> > +                       for (i = 0; i < 1 << order; i++)
> > +                               tag_clear_highpage(page + i);
> > +
> > +                       init = false;
>
> I find this a bit twisted and prone to breakages.
> Maybe just check for (init && !init_tags) below?

I did it this way deliberately. Check out the code after all the changes:

https://github.com/xairy/linux/blob/up-kasan-vmalloc-tags-v1/mm/page_alloc.c#L2447

It's possible to remove resetting the init variable by expanding the
if (init) check listing all conditions under which init is currently
reset, but that would essentially be duplicating the checks. I think
resetting init is more clear.

Please let me know what you think.

Thanks!