| Message ID | 20210106115519.32222-3-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ARMv8.5-A: MTE: Add async mode support | expand |
On Wed, Jan 6, 2021 at 12:56 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > MTE provides an asynchronous mode for detecting tag exceptions. In > particular instead of triggering a fault the arm64 core updates a > register which is checked by the kernel at the first entry after the tag > exception has occurred. > > Add support for MTE asynchronous mode. > > The exception handling mechanism will be added with a future patch. > > Note: KASAN HW activates async mode via kasan.mode kernel parameter. > The default mode is set to synchronous. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > arch/arm64/kernel/mte.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index 24a273d47df1..5d992e16b420 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -153,8 +153,35 @@ void mte_init_tags(u64 max_tag) > > void mte_enable_kernel(enum kasan_arg_mode mode) > { > - /* Enable MTE Sync Mode for EL1. */ > - sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > + const char *m; > + > + /* Preset parameter values based on the mode. */ > + switch (mode) { > + case KASAN_ARG_MODE_OFF: > + return; > + case KASAN_ARG_MODE_LIGHT: > + /* Enable MTE Async Mode for EL1. */ > + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC); > + m = "asynchronous"; > + break; > + case KASAN_ARG_MODE_DEFAULT: > + case KASAN_ARG_MODE_PROD: > + case KASAN_ARG_MODE_FULL: > + /* Enable MTE Sync Mode for EL1. */ > + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > + m = "synchronous"; > + break; > + default: > + /* > + * kasan mode should be always set hence we should > + * not reach this condition. > + */ > + WARN_ON_ONCE(1); > + return; > + } > + > + pr_info_once("MTE: enabled in %s mode at EL1\n", m); > + > isb(); > } > > -- > 2.29.2 > Hi Vincenzo, It would be cleaner to pass a bool to mte_enable_kernel() and have it indicate sync/async mode. This way you don't have to pull all these KASAN constants into the arm64 code. Thanks!
Hi Andrey, On 1/7/21 4:29 PM, Andrey Konovalov wrote: > On Wed, Jan 6, 2021 at 12:56 PM Vincenzo Frascino > <vincenzo.frascino@arm.com> wrote: >> >> MTE provides an asynchronous mode for detecting tag exceptions. In >> particular instead of triggering a fault the arm64 core updates a >> register which is checked by the kernel at the first entry after the tag >> exception has occurred. >> >> Add support for MTE asynchronous mode. >> >> The exception handling mechanism will be added with a future patch. >> >> Note: KASAN HW activates async mode via kasan.mode kernel parameter. >> The default mode is set to synchronous. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> >> --- >> arch/arm64/kernel/mte.c | 31 +++++++++++++++++++++++++++++-- >> 1 file changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c >> index 24a273d47df1..5d992e16b420 100644 >> --- a/arch/arm64/kernel/mte.c >> +++ b/arch/arm64/kernel/mte.c >> @@ -153,8 +153,35 @@ void mte_init_tags(u64 max_tag) >> >> void mte_enable_kernel(enum kasan_arg_mode mode) >> { >> - /* Enable MTE Sync Mode for EL1. */ >> - sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); >> + const char *m; >> + >> + /* Preset parameter values based on the mode. */ >> + switch (mode) { >> + case KASAN_ARG_MODE_OFF: >> + return; >> + case KASAN_ARG_MODE_LIGHT: >> + /* Enable MTE Async Mode for EL1. */ >> + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC); >> + m = "asynchronous"; >> + break; >> + case KASAN_ARG_MODE_DEFAULT: >> + case KASAN_ARG_MODE_PROD: >> + case KASAN_ARG_MODE_FULL: >> + /* Enable MTE Sync Mode for EL1. */ >> + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); >> + m = "synchronous"; >> + break; >> + default: >> + /* >> + * kasan mode should be always set hence we should >> + * not reach this condition. >> + */ >> + WARN_ON_ONCE(1); >> + return; >> + } >> + >> + pr_info_once("MTE: enabled in %s mode at EL1\n", m); >> + >> isb(); >> } >> >> -- >> 2.29.2 >> > > Hi Vincenzo, > > It would be cleaner to pass a bool to mte_enable_kernel() and have it > indicate sync/async mode. This way you don't have to pull all these > KASAN constants into the arm64 code. > 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. > Thanks! >
On Thu, Jan 7, 2021 at 6:25 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > Hi Andrey, > > On 1/7/21 4:29 PM, Andrey Konovalov wrote: > > On Wed, Jan 6, 2021 at 12:56 PM Vincenzo Frascino > > <vincenzo.frascino@arm.com> wrote: > >> > >> MTE provides an asynchronous mode for detecting tag exceptions. In > >> particular instead of triggering a fault the arm64 core updates a > >> register which is checked by the kernel at the first entry after the tag > >> exception has occurred. > >> > >> Add support for MTE asynchronous mode. > >> > >> The exception handling mechanism will be added with a future patch. > >> > >> Note: KASAN HW activates async mode via kasan.mode kernel parameter. > >> The default mode is set to synchronous. > >> > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > >> Cc: Will Deacon <will.deacon@arm.com> > >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > >> --- > >> arch/arm64/kernel/mte.c | 31 +++++++++++++++++++++++++++++-- > >> 1 file changed, 29 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > >> index 24a273d47df1..5d992e16b420 100644 > >> --- a/arch/arm64/kernel/mte.c > >> +++ b/arch/arm64/kernel/mte.c > >> @@ -153,8 +153,35 @@ void mte_init_tags(u64 max_tag) > >> > >> void mte_enable_kernel(enum kasan_arg_mode mode) > >> { > >> - /* Enable MTE Sync Mode for EL1. */ > >> - sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > >> + const char *m; > >> + > >> + /* Preset parameter values based on the mode. */ > >> + switch (mode) { > >> + case KASAN_ARG_MODE_OFF: > >> + return; > >> + case KASAN_ARG_MODE_LIGHT: > >> + /* Enable MTE Async Mode for EL1. */ > >> + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC); > >> + m = "asynchronous"; > >> + break; > >> + case KASAN_ARG_MODE_DEFAULT: > >> + case KASAN_ARG_MODE_PROD: > >> + case KASAN_ARG_MODE_FULL: > >> + /* Enable MTE Sync Mode for EL1. */ > >> + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > >> + m = "synchronous"; > >> + break; > >> + default: > >> + /* > >> + * kasan mode should be always set hence we should > >> + * not reach this condition. > >> + */ > >> + WARN_ON_ONCE(1); > >> + return; > >> + } > >> + > >> + pr_info_once("MTE: enabled in %s mode at EL1\n", m); > >> + > >> isb(); > >> } > >> > >> -- > >> 2.29.2 > >> > > > > Hi Vincenzo, > > > > It would be cleaner to pass a bool to mte_enable_kernel() and have it > > indicate sync/async mode. This way you don't have to pull all these > > KASAN constants into the arm64 code. > > > > 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. 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 }
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. > 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?
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.
Hi Andrey, On 1/8/21 1:36 PM, Andrey Konovalov wrote: > 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. I am fine with this solution, I will add it in my v3. As part of the enumeration I will add READ_SYNC mode as well, so we have all the possible combinations.
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index 24a273d47df1..5d992e16b420 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -153,8 +153,35 @@ void mte_init_tags(u64 max_tag) void mte_enable_kernel(enum kasan_arg_mode mode) { - /* Enable MTE Sync Mode for EL1. */ - sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); + const char *m; + + /* Preset parameter values based on the mode. */ + switch (mode) { + case KASAN_ARG_MODE_OFF: + return; + case KASAN_ARG_MODE_LIGHT: + /* Enable MTE Async Mode for EL1. */ + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC); + m = "asynchronous"; + break; + case KASAN_ARG_MODE_DEFAULT: + case KASAN_ARG_MODE_PROD: + case KASAN_ARG_MODE_FULL: + /* Enable MTE Sync Mode for EL1. */ + sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); + m = "synchronous"; + break; + default: + /* + * kasan mode should be always set hence we should + * not reach this condition. + */ + WARN_ON_ONCE(1); + return; + } + + pr_info_once("MTE: enabled in %s mode at EL1\n", m); + isb(); }
MTE provides an asynchronous mode for detecting tag exceptions. In particular instead of triggering a fault the arm64 core updates a register which is checked by the kernel at the first entry after the tag exception has occurred. Add support for MTE asynchronous mode. The exception handling mechanism will be added with a future patch. Note: KASAN HW activates async mode via kasan.mode kernel parameter. The default mode is set to synchronous. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> --- arch/arm64/kernel/mte.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)