Re: [PATCH 2/4] arm64: mte: Add asynchronous mode support

From: Andrey Konovalov <andreyknvl@google.com>
To: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Branislav Rankov <Branislav.Rankov@arm.com>,
	Marco Elver <elver@google.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Will Deacon <will.deacon@arm.com>,
	LKML <linux-kernel@vger.kernel.org>,
	kasan-dev <kasan-dev@googlegroups.com>,
	Alexander Potapenko <glider@google.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH 2/4] arm64: mte: Add asynchronous mode support
Date: Fri, 8 Jan 2021 14:36:47 +0100	[thread overview]
Message-ID: <CAAeHK+zjwr0M92zqUjseJmRmhHb=4GjevEft-mahfx5DOkq==w@mail.gmail.com> (raw)
In-Reply-To: <c3efaa8d-cb3a-0c2a-457e-bfba60551d80@arm.com>

On Fri, Jan 8, 2021 at 11:44 AM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> Hi Andrey,
>
> On 1/7/21 7:18 PM, Andrey Konovalov wrote:
> >> Boolean arguments are generally bad for legibility, hence I tend to avoid them.
> >> In this case exposing the constants does not seem a big issue especially because
> >> the only user of this code is "KASAN_HW_TAGS" and definitely improves its
> >> legibility hence I would prefer to keep it as is.
> >
> > I don't like that this spills KASAN internals to the arm64 code.
>
> Could you please elaborate a bit more on this?
>
> If I understand it correctly these enumerations I exposed are the direct
> representation of a kernel command line parameter which, according to me, should
> not be considered an internal interface.
> Seems that in general the kernel subsystems expose the interface for the
> architectures to consume which is the same design pattern I followed in this case.

It's fine from the point of view of kernel interfaces and such, but
not from a higher-level design perspective.

I think the best way to approach the KASAN-MTE architecture is: 1.
arm64 code provides API to enable, disable and otherwise work with
MTE, and 2. KASAN builds on top of this API to implement the logic of
the bug detector, including which APIs to use. Part #2 includes making
the decisions about which mode - sync or async - to use and when. And
that mode is chosen by KASAN code based on the command line configs.

With your current approach, the active decision about enabling
sync/async is made by the arm64 code, and that doesn't fit within this
architecture. But having a decisionless arm64 API to choose the MTE
mode and using it from KASAN code would fit.

> > Let's add another enum with two values and pass it as an argument then.
> > Something like:
> >
> > enum mte_mode {
> >   ARM_MTE_SYNC,
> >   ARM_MTE_ASYNC
> > }
>
> I had something similar at the beginning of the development but I ended up in a
> situation in which the generic kasan code had to know about "enum mte_mode",
> hence I preferred to keep kasan agnostic to the hw implementation details.
>
> What do you think?

Perhaps we could add a generic arch-agnostic enum to
include/linux/kasan.h and use it in both arm64 and KASAN code?

enum kasan_hw_tags_mode {
  KASAN_HW_TAGS_SYNC,
  KASAN_HW_TAGS_ASYNC
}

Assuming other architectures that support memory tagging will end up
with sync/async mode separation as well, this should work. But even if
that doesn't happen, this interface can be adjusted later.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel