2022-09-07 12:00:16

by Vincenzo Frascino

[permalink] [raw]
Subject: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

When the kernel is entered on aarch64, the MTE allocation tags are in an
UNKNOWN state.

With MTE enabled, the tags are initialized:
- When a page is allocated and the user maps it with PROT_MTE.
- On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).

If the tag pool is zeroed by the hardware at reset, it makes it
difficult to track potential places where the initialization of the
tags was missed.

This can be observed under QEMU for aarch64, which initializes the MTE
allocation tags to zero.

Initialize to tag storage to KASAN_TAG_INVALID to catch potential
places where the initialization of the tags was missed.

This is done introducing a new kernel command line parameter
"mte.tags_init" that enables the debug option.

Note: The proposed solution should be considered a debug option because
it might have performance impact on large machines at boot.

Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Signed-off-by: Vincenzo Frascino <[email protected]>
---
arch/arm64/kernel/mte.c | 47 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index b2b730233274..af9a8eba9be4 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -6,6 +6,7 @@
#include <linux/bitops.h>
#include <linux/cpu.h>
#include <linux/kernel.h>
+#include <linux/memblock.h>
#include <linux/mm.h>
#include <linux/prctl.h>
#include <linux/sched.h>
@@ -35,6 +36,8 @@ DEFINE_STATIC_KEY_FALSE(mte_async_or_asymm_mode);
EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode);
#endif

+static bool mte_tags_init __ro_after_init;
+
static void mte_sync_page_tags(struct page *page, pte_t old_pte,
bool check_swap, bool pte_is_tagged)
{
@@ -98,6 +101,48 @@ int memcmp_pages(struct page *page1, struct page *page2)
return ret;
}

+/* mte.tags_init=off/on */
+static int __init early_mte_tags_init(char *arg)
+{
+ if (!arg)
+ return -EINVAL;
+
+ if (!strcmp(arg, "off"))
+ mte_tags_init = false;
+ else if (!strcmp(arg, "on"))
+ mte_tags_init = true;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+early_param("mte.tags_init", early_mte_tags_init);
+
+static inline void __mte_tag_storage_init(void)
+{
+ static bool mte_tags_uninitialized = true;
+ phys_addr_t pa_start, pa_end;
+ u64 index;
+
+ if (mte_tags_init && !mte_tags_uninitialized)
+ return;
+
+ for_each_mem_range(index, &pa_start, &pa_end) {
+ void *va_start = (void *)__phys_to_virt(pa_start);
+ void *va_end = (void *)__phys_to_virt(pa_end);
+ size_t va_size = (u64)va_end - (u64)va_start;
+
+ if (va_start >= va_end)
+ break;
+
+ mte_set_mem_tag_range(va_start, va_size, KASAN_TAG_INVALID, false);
+ }
+
+ /* Tags are now initialized to KASAN_TAG_INVALID */
+ mte_tags_uninitialized = false;
+ pr_info("MTE: Tag Storage Initialized\n");
+}
+
static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
{
/* Enable MTE Sync Mode for EL1. */
@@ -105,6 +150,8 @@ static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
SYS_FIELD_PREP(SCTLR_EL1, TCF, tcf));
isb();

+ __mte_tag_storage_init();
+
pr_info_once("MTE: enabled in %s mode at EL1\n", mode);
}

--
2.37.3


2022-09-08 10:40:45

by Vladimir Murzin

[permalink] [raw]
Subject: Re: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

Hi Vincenzo,

On 9/7/22 12:00, Vincenzo Frascino wrote:
> When the kernel is entered on aarch64, the MTE allocation tags are in an
> UNKNOWN state.
>
> With MTE enabled, the tags are initialized:
> - When a page is allocated and the user maps it with PROT_MTE.
> - On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).
>
> If the tag pool is zeroed by the hardware at reset, it makes it
> difficult to track potential places where the initialization of the
> tags was missed.
>
> This can be observed under QEMU for aarch64, which initializes the MTE
> allocation tags to zero.
>
> Initialize to tag storage to KASAN_TAG_INVALID to catch potential
> places where the initialization of the tags was missed.
>
> This is done introducing a new kernel command line parameter
> "mte.tags_init" that enables the debug option.
>
> Note: The proposed solution should be considered a debug option because
> it might have performance impact on large machines at boot.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Vincenzo Frascino <[email protected]>
> ---
> arch/arm64/kernel/mte.c | 47 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)

Nothing in Documentation/ ?

>
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index b2b730233274..af9a8eba9be4 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -6,6 +6,7 @@
> #include <linux/bitops.h>
> #include <linux/cpu.h>
> #include <linux/kernel.h>
> +#include <linux/memblock.h>
> #include <linux/mm.h>
> #include <linux/prctl.h>
> #include <linux/sched.h>
> @@ -35,6 +36,8 @@ DEFINE_STATIC_KEY_FALSE(mte_async_or_asymm_mode);
> EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode);
> #endif
>
> +static bool mte_tags_init __ro_after_init;
> +
> static void mte_sync_page_tags(struct page *page, pte_t old_pte,
> bool check_swap, bool pte_is_tagged)
> {
> @@ -98,6 +101,48 @@ int memcmp_pages(struct page *page1, struct page *page2)
> return ret;
> }
>
> +/* mte.tags_init=off/on */
> +static int __init early_mte_tags_init(char *arg)
> +{
> + if (!arg)
> + return -EINVAL;
> +
> + if (!strcmp(arg, "off"))
> + mte_tags_init = false;
> + else if (!strcmp(arg, "on"))
> + mte_tags_init = true;
> + else
> + return -EINVAL;
> +

You might want to offload parsing to kstrtobool()

> + return 0;
> +}
> +early_param("mte.tags_init", early_mte_tags_init);
> +
> +static inline void __mte_tag_storage_init(void)
> +{
> + static bool mte_tags_uninitialized = true;
> + phys_addr_t pa_start, pa_end;
> + u64 index;
> +
> + if (mte_tags_init && !mte_tags_uninitialized)
> + return;
> +
> + for_each_mem_range(index, &pa_start, &pa_end) {
> + void *va_start = (void *)__phys_to_virt(pa_start);
> + void *va_end = (void *)__phys_to_virt(pa_end);
> + size_t va_size = (u64)va_end - (u64)va_start;
> +
> + if (va_start >= va_end)
> + break;
> +
> + mte_set_mem_tag_range(va_start, va_size, KASAN_TAG_INVALID, false);
> + }
> +
> + /* Tags are now initialized to KASAN_TAG_INVALID */
> + mte_tags_uninitialized = false;
> + pr_info("MTE: Tag Storage Initialized\n");

Why All Words Start With Capital Letter? :D

Anyway, you might want to advertise tag value used for initialization.

> +}
> +
> static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
> {
> /* Enable MTE Sync Mode for EL1. */
> @@ -105,6 +150,8 @@ static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
> SYS_FIELD_PREP(SCTLR_EL1, TCF, tcf));
> isb();
>
> + __mte_tag_storage_init();
> +
> pr_info_once("MTE: enabled in %s mode at EL1\n", mode);
> }
>

Cheers
Vladimir

2022-09-08 14:13:08

by Vincenzo Frascino

[permalink] [raw]
Subject: Re: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

Hi Vladimir,

On 9/8/22 11:36, Vladimir Murzin wrote:
> Hi Vincenzo,
>
> On 9/7/22 12:00, Vincenzo Frascino wrote:
>> When the kernel is entered on aarch64, the MTE allocation tags are in an
>> UNKNOWN state.
>>
>> With MTE enabled, the tags are initialized:
>> - When a page is allocated and the user maps it with PROT_MTE.
>> - On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).
>>
>> If the tag pool is zeroed by the hardware at reset, it makes it
>> difficult to track potential places where the initialization of the
>> tags was missed.
>>
>> This can be observed under QEMU for aarch64, which initializes the MTE
>> allocation tags to zero.
>>
>> Initialize to tag storage to KASAN_TAG_INVALID to catch potential
>> places where the initialization of the tags was missed.
>>
>> This is done introducing a new kernel command line parameter
>> "mte.tags_init" that enables the debug option.
>>
>> Note: The proposed solution should be considered a debug option because
>> it might have performance impact on large machines at boot.
>>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Signed-off-by: Vincenzo Frascino <[email protected]>
>> ---
>> arch/arm64/kernel/mte.c | 47 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>
> Nothing in Documentation/ ?
>

I can have a separate patch that adds documentation of the kernel parameter.

>>
>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index b2b730233274..af9a8eba9be4 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -6,6 +6,7 @@
>> #include <linux/bitops.h>
>> #include <linux/cpu.h>
>> #include <linux/kernel.h>
>> +#include <linux/memblock.h>
>> #include <linux/mm.h>
>> #include <linux/prctl.h>
>> #include <linux/sched.h>
>> @@ -35,6 +36,8 @@ DEFINE_STATIC_KEY_FALSE(mte_async_or_asymm_mode);
>> EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode);
>> #endif
>>
>> +static bool mte_tags_init __ro_after_init;
>> +
>> static void mte_sync_page_tags(struct page *page, pte_t old_pte,
>> bool check_swap, bool pte_is_tagged)
>> {
>> @@ -98,6 +101,48 @@ int memcmp_pages(struct page *page1, struct page *page2)
>> return ret;
>> }
>>
>> +/* mte.tags_init=off/on */
>> +static int __init early_mte_tags_init(char *arg)
>> +{
>> + if (!arg)
>> + return -EINVAL;
>> +
>> + if (!strcmp(arg, "off"))
>> + mte_tags_init = false;
>> + else if (!strcmp(arg, "on"))
>> + mte_tags_init = true;
>> + else
>> + return -EINVAL;
>> +
>
> You might want to offload parsing to kstrtobool()
>

Good point, I was not aware of this API. Thanks!

>> + return 0;
>> +}
>> +early_param("mte.tags_init", early_mte_tags_init);
>> +
>> +static inline void __mte_tag_storage_init(void)
>> +{
>> + static bool mte_tags_uninitialized = true;
>> + phys_addr_t pa_start, pa_end;
>> + u64 index;
>> +
>> + if (mte_tags_init && !mte_tags_uninitialized)
>> + return;
>> +
>> + for_each_mem_range(index, &pa_start, &pa_end) {
>> + void *va_start = (void *)__phys_to_virt(pa_start);
>> + void *va_end = (void *)__phys_to_virt(pa_end);
>> + size_t va_size = (u64)va_end - (u64)va_start;
>> +
>> + if (va_start >= va_end)
>> + break;
>> +
>> + mte_set_mem_tag_range(va_start, va_size, KASAN_TAG_INVALID, false);
>> + }
>> +
>> + /* Tags are now initialized to KASAN_TAG_INVALID */
>> + mte_tags_uninitialized = false;
>> + pr_info("MTE: Tag Storage Initialized\n");
>
> Why All Words Start With Capital Letter? :D
>

Do you have any preference? :D

> Anyway, you might want to advertise tag value used for initialization.
>

Yes I agree, I can print "Tag Storage Initialized to 0x.."

>> +}
>> +
>> static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
>> {
>> /* Enable MTE Sync Mode for EL1. */
>> @@ -105,6 +150,8 @@ static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
>> SYS_FIELD_PREP(SCTLR_EL1, TCF, tcf));
>> isb();
>>
>> + __mte_tag_storage_init();
>> +
>> pr_info_once("MTE: enabled in %s mode at EL1\n", mode);
>> }
>>
>
> Cheers
> Vladimir

--
Regards,
Vincenzo

2022-09-10 23:53:44

by Andrey Konovalov

[permalink] [raw]
Subject: Re: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

On Wed, Sep 7, 2022 at 1:00 PM Vincenzo Frascino
<[email protected]> wrote:
>
> When the kernel is entered on aarch64, the MTE allocation tags are in an
> UNKNOWN state.
>
> With MTE enabled, the tags are initialized:
> - When a page is allocated and the user maps it with PROT_MTE.
> - On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).
>
> If the tag pool is zeroed by the hardware at reset, it makes it
> difficult to track potential places where the initialization of the
> tags was missed.
>
> This can be observed under QEMU for aarch64, which initializes the MTE
> allocation tags to zero.
>
> Initialize to tag storage to KASAN_TAG_INVALID to catch potential
> places where the initialization of the tags was missed.

Hi Vincenzo,

Cold you clarify what kind of places this refers to? Like the kernel
allocating memory and not setting the tags? Or is this related to
userspace applications? I'm not sure what's the user story for this
new flag is.

> This is done introducing a new kernel command line parameter
> "mte.tags_init" that enables the debug option.

Depending on the intended use, this can be extended to "mte.tags_init=<tag>".

> Note: The proposed solution should be considered a debug option because
> it might have performance impact on large machines at boot.

Thanks!

2022-11-07 15:38:53

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

On Wed, Sep 07, 2022 at 12:00:15PM +0100, Vincenzo Frascino wrote:
> When the kernel is entered on aarch64, the MTE allocation tags are in an
> UNKNOWN state.
>
> With MTE enabled, the tags are initialized:
> - When a page is allocated and the user maps it with PROT_MTE.
> - On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).
>
> If the tag pool is zeroed by the hardware at reset, it makes it
> difficult to track potential places where the initialization of the
> tags was missed.
>
> This can be observed under QEMU for aarch64, which initializes the MTE
> allocation tags to zero.
>
> Initialize to tag storage to KASAN_TAG_INVALID to catch potential
> places where the initialization of the tags was missed.
>
> This is done introducing a new kernel command line parameter
> "mte.tags_init" that enables the debug option.
>
> Note: The proposed solution should be considered a debug option because
> it might have performance impact on large machines at boot.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Vincenzo Frascino <[email protected]>
> ---
> arch/arm64/kernel/mte.c | 47 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)

I don't really see the point in this change -- who is going to use this
option?

Will

2022-11-07 17:10:07

by Vincenzo Frascino

[permalink] [raw]
Subject: Re: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

Hi Will,

On 11/7/22 15:19, Will Deacon wrote:
> On Wed, Sep 07, 2022 at 12:00:15PM +0100, Vincenzo Frascino wrote:
>> When the kernel is entered on aarch64, the MTE allocation tags are in an
>> UNKNOWN state.
>>
>> With MTE enabled, the tags are initialized:
>> - When a page is allocated and the user maps it with PROT_MTE.
>> - On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).
>>
>> If the tag pool is zeroed by the hardware at reset, it makes it
>> difficult to track potential places where the initialization of the
>> tags was missed.
>>
>> This can be observed under QEMU for aarch64, which initializes the MTE
>> allocation tags to zero.
>>
>> Initialize to tag storage to KASAN_TAG_INVALID to catch potential
>> places where the initialization of the tags was missed.
>>
>> This is done introducing a new kernel command line parameter
>> "mte.tags_init" that enables the debug option.
>>
>> Note: The proposed solution should be considered a debug option because
>> it might have performance impact on large machines at boot.
>>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Signed-off-by: Vincenzo Frascino <[email protected]>
>> ---
>> arch/arm64/kernel/mte.c | 47 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>
> I don't really see the point in this change -- who is going to use this
> option?
>

I think this option can be useful to someone who is trying to debug a problem
that is related to a missed tag initialization and it is doing it on QEMU.

QEMU by default would mask this class of problems because it initializes to zero
the tags at "reset" (which is a valid UNKNOWN STATE according to the architecture).

I noticed this behavior because I was trying to debug a similar issue which I
was able to reproduce only on FVP.

Said that, I originally posted this patch as RFC back in April this year to find
out if someone else would find it useful, in fact my idea was to keep it locally.

Please let me know what do you want to do.

> Will

--
Regards,
Vincenzo

2022-11-08 14:25:27

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v2] mte: Initialize tag storage to KASAN_TAG_INVALID

On Mon, Nov 07, 2022 at 04:47:14PM +0000, Vincenzo Frascino wrote:
> Hi Will,
>
> On 11/7/22 15:19, Will Deacon wrote:
> > On Wed, Sep 07, 2022 at 12:00:15PM +0100, Vincenzo Frascino wrote:
> >> When the kernel is entered on aarch64, the MTE allocation tags are in an
> >> UNKNOWN state.
> >>
> >> With MTE enabled, the tags are initialized:
> >> - When a page is allocated and the user maps it with PROT_MTE.
> >> - On allocation, with in-kernel MTE enabled (HW_TAGS KASAN).
> >>
> >> If the tag pool is zeroed by the hardware at reset, it makes it
> >> difficult to track potential places where the initialization of the
> >> tags was missed.
> >>
> >> This can be observed under QEMU for aarch64, which initializes the MTE
> >> allocation tags to zero.
> >>
> >> Initialize to tag storage to KASAN_TAG_INVALID to catch potential
> >> places where the initialization of the tags was missed.
> >>
> >> This is done introducing a new kernel command line parameter
> >> "mte.tags_init" that enables the debug option.
> >>
> >> Note: The proposed solution should be considered a debug option because
> >> it might have performance impact on large machines at boot.
> >>
> >> Cc: Catalin Marinas <[email protected]>
> >> Cc: Will Deacon <[email protected]>
> >> Signed-off-by: Vincenzo Frascino <[email protected]>
> >> ---
> >> arch/arm64/kernel/mte.c | 47 +++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 47 insertions(+)
> >
> > I don't really see the point in this change -- who is going to use this
> > option?
> >
>
> I think this option can be useful to someone who is trying to debug a problem
> that is related to a missed tag initialization and it is doing it on QEMU.
>
> QEMU by default would mask this class of problems because it initializes to zero
> the tags at "reset" (which is a valid UNKNOWN STATE according to the architecture).
>
> I noticed this behavior because I was trying to debug a similar issue which I
> was able to reproduce only on FVP.
>
> Said that, I originally posted this patch as RFC back in April this year to find
> out if someone else would find it useful, in fact my idea was to keep it locally.
>
> Please let me know what do you want to do.

I'd prefer to leave the code as-is until we have a concrete ask for this
feature.

Will