Re: [PATCH v2 10/20] kasan: inline and rename kasan_unpoison_memory

From: Marco Elver <elver@google.com>
To: Andrey Konovalov <andreyknvl@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>,
	Alexander Potapenko <glider@google.com>,
	 Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	 Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	 Andrey Ryabinin <aryabinin@virtuozzo.com>,
	Branislav Rankov <Branislav.Rankov@arm.com>,
	 Kevin Brodsky <kevin.brodsky@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	 kasan-dev <kasan-dev@googlegroups.com>,
	 Linux ARM <linux-arm-kernel@lists.infradead.org>,
	 Linux Memory Management List <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 10/20] kasan: inline and rename kasan_unpoison_memory
Date: Thu, 12 Nov 2020 20:52:12 +0100	[thread overview]
Message-ID: <CANpmjNOboPh97HdMGAESSEYdeyd9+9MVy6E3QsvVAYuWVReRew@mail.gmail.com> (raw)
In-Reply-To: <CAAeHK+wvvkYko=tM=NHODkKas13h5Jvsswvg05jhv9LqE0jSjQ@mail.gmail.com>

On Thu, 12 Nov 2020 at 20:45, Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Wed, Nov 11, 2020 at 6:49 PM Marco Elver <elver@google.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 11:20PM +0100, Andrey Konovalov wrote:
> > > Currently kasan_unpoison_memory() is used as both an external annotation
> > > and as an internal memory poisoning helper. Rename external annotation to
> > > kasan_unpoison_data() and inline the internal helper for hardware
> > > tag-based mode to avoid undeeded function calls.
> >
> > I don't understand why this needs to be renamed again. The users of
> > kasan_unpoison_memory() outweigh those of kasan_unpoison_slab(), of
> > which there seems to be only 1!
>
> The idea is to make kasan_(un)poison_memory() functions inlinable for
> internal use. It doesn't have anything to do with the number of times
> they are used.
>
> Perhaps we can drop the kasan_ prefix for the internal implementations
> though, and keep using kasan_unpoison_memory() externally.

Whatever avoids changing the external interface, because it seems
really pointless. I can see why it's done, but it's a side-effect of
the various wrappers being added.

I'd much rather prefer we do it right from the beginning, and cleaning
up things very much is related to this series vs. just making things
uglier and hoping somebody will clean it up later.

> > So can't we just get rid of kasan_unpoison_slab() and just open-code it
> > in mm/mempool.c:kasan_unpoison_element()? That function is already
> > kasan-prefixed, so we can even place a small comment there (which would
> > also be an improvement over current interface, since
> > kasan_unpoison_slab() is not documented and its existence not quite
> > justified).
>
> We can, but this is a change unrelated to this patch.

Not quite, we're trying to optimize KASAN which is related -- this
patch as-is would obviously change, but replaced by a patch
simplifying things. This change as-is makes 2 changes outside of
KASAN, whereas if we removed it it would only be 1 and we end up with
less cruft.