| Message ID | 20210517123239.8025-4-steven.price@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | MTE support for KVM guest | expand |
On Mon, 17 May 2021 13:32:34 +0100, Steven Price <steven.price@arm.com> wrote: > > A KVM guest could store tags in a page even if the VMM hasn't mapped > the page with PROT_MTE. So when restoring pages from swap we will > need to check to see if there are any saved tags even if !pte_tagged(). > > However don't check pages for which pte_access_permitted() returns false > as these will not have been swapped out. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > arch/arm64/include/asm/pgtable.h | 9 +++++++-- > arch/arm64/kernel/mte.c | 16 ++++++++++++++-- > 2 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index 0b10204e72fc..275178a810c1 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, > if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) > __sync_icache_dcache(pte); > > - if (system_supports_mte() && > - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) > + /* > + * If the PTE would provide user space access to the tags associated > + * with it then ensure that the MTE tags are synchronised. Exec-only > + * mappings don't expose tags (instruction fetches don't check tags). I'm not sure I understand this comment. Of course, execution doesn't match tags. But the memory could still have tags associated with it. Does this mean such a page would lose its tags is swapped out? Thanks, M. > + */ > + if (system_supports_mte() && pte_present(pte) && > + pte_access_permitted(pte, false) && !pte_special(pte)) > mte_sync_tags(ptep, pte); > > __check_racy_pte_update(mm, ptep, pte); > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index c88e778c2fa9..a604818c52c1 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -33,11 +33,15 @@ DEFINE_STATIC_KEY_FALSE(mte_async_mode); > EXPORT_SYMBOL_GPL(mte_async_mode); > #endif > > -static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) > +static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap, > + bool pte_is_tagged) > { > unsigned long flags; > pte_t old_pte = READ_ONCE(*ptep); > > + if (!is_swap_pte(old_pte) && !pte_is_tagged) > + return; > + > spin_lock_irqsave(&tag_sync_lock, flags); > > /* Recheck with the lock held */ > @@ -53,6 +57,9 @@ static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) > } > } > > + if (!pte_is_tagged) > + goto out; > + > page_kasan_tag_reset(page); > /* > * We need smp_wmb() in between setting the flags and clearing the > @@ -76,10 +83,15 @@ void mte_sync_tags(pte_t *ptep, pte_t pte) > bool check_swap = nr_pages == 1; > bool pte_is_tagged = pte_tagged(pte); > > + /* Early out if there's nothing to do */ > + if (!check_swap && !pte_is_tagged) > + return; > + > /* if PG_mte_tagged is set, tags have already been initialised */ > for (i = 0; i < nr_pages; i++, page++) { > if (!test_bit(PG_mte_tagged, &page->flags)) > - mte_sync_page_tags(page, ptep, check_swap); > + mte_sync_page_tags(page, ptep, check_swap, > + pte_is_tagged); > } > } > > -- > 2.20.1 > >
On 17/05/2021 17:14, Marc Zyngier wrote: > On Mon, 17 May 2021 13:32:34 +0100, > Steven Price <steven.price@arm.com> wrote: >> >> A KVM guest could store tags in a page even if the VMM hasn't mapped >> the page with PROT_MTE. So when restoring pages from swap we will >> need to check to see if there are any saved tags even if !pte_tagged(). >> >> However don't check pages for which pte_access_permitted() returns false >> as these will not have been swapped out. >> >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> arch/arm64/include/asm/pgtable.h | 9 +++++++-- >> arch/arm64/kernel/mte.c | 16 ++++++++++++++-- >> 2 files changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >> index 0b10204e72fc..275178a810c1 100644 >> --- a/arch/arm64/include/asm/pgtable.h >> +++ b/arch/arm64/include/asm/pgtable.h >> @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, >> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) >> __sync_icache_dcache(pte); >> >> - if (system_supports_mte() && >> - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) >> + /* >> + * If the PTE would provide user space access to the tags associated >> + * with it then ensure that the MTE tags are synchronised. Exec-only >> + * mappings don't expose tags (instruction fetches don't check tags). > > I'm not sure I understand this comment. Of course, execution doesn't > match tags. But the memory could still have tags associated with > it. Does this mean such a page would lose its tags is swapped out? Hmm, I probably should have reread that - the context of the comment is lost. I added the comment when changing to pte_access_permitted(), and the comment on pte_access_permitted() explains a potential gotcha: * p??_access_permitted() is true for valid user mappings (PTE_USER * bit set, subject to the write permission check). For execute-only * mappings, like PROT_EXEC with EPAN (both PTE_USER and PTE_UXN bits * not set) must return false. PROT_NONE mappings do not have the * PTE_VALID bit set. So execute-only mappings return false even though that is effectively a type of user access. However, because MTE checks are not performed by the PE for instruction fetches this doesn't matter. I'll update the comment, how about: /* * If the PTE would provide user space access to the tags associated * with it then ensure that the MTE tags are synchronised. Although * pte_access_permitted() returns false for exec only mappings, they * don't expose tags (instruction fetches don't check tags). */ Thanks, Steve > Thanks, > > M. > >> + */ >> + if (system_supports_mte() && pte_present(pte) && >> + pte_access_permitted(pte, false) && !pte_special(pte)) >> mte_sync_tags(ptep, pte); >> >> __check_racy_pte_update(mm, ptep, pte); >> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c >> index c88e778c2fa9..a604818c52c1 100644 >> --- a/arch/arm64/kernel/mte.c >> +++ b/arch/arm64/kernel/mte.c >> @@ -33,11 +33,15 @@ DEFINE_STATIC_KEY_FALSE(mte_async_mode); >> EXPORT_SYMBOL_GPL(mte_async_mode); >> #endif >> >> -static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) >> +static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap, >> + bool pte_is_tagged) >> { >> unsigned long flags; >> pte_t old_pte = READ_ONCE(*ptep); >> >> + if (!is_swap_pte(old_pte) && !pte_is_tagged) >> + return; >> + >> spin_lock_irqsave(&tag_sync_lock, flags); >> >> /* Recheck with the lock held */ >> @@ -53,6 +57,9 @@ static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) >> } >> } >> >> + if (!pte_is_tagged) >> + goto out; >> + >> page_kasan_tag_reset(page); >> /* >> * We need smp_wmb() in between setting the flags and clearing the >> @@ -76,10 +83,15 @@ void mte_sync_tags(pte_t *ptep, pte_t pte) >> bool check_swap = nr_pages == 1; >> bool pte_is_tagged = pte_tagged(pte); >> >> + /* Early out if there's nothing to do */ >> + if (!check_swap && !pte_is_tagged) >> + return; >> + >> /* if PG_mte_tagged is set, tags have already been initialised */ >> for (i = 0; i < nr_pages; i++, page++) { >> if (!test_bit(PG_mte_tagged, &page->flags)) >> - mte_sync_page_tags(page, ptep, check_swap); >> + mte_sync_page_tags(page, ptep, check_swap, >> + pte_is_tagged); >> } >> } >> >> -- >> 2.20.1 >> >> >
On Wed, May 19, 2021 at 10:32:01AM +0100, Steven Price wrote: > On 17/05/2021 17:14, Marc Zyngier wrote: > > On Mon, 17 May 2021 13:32:34 +0100, > > Steven Price <steven.price@arm.com> wrote: > >> > >> A KVM guest could store tags in a page even if the VMM hasn't mapped > >> the page with PROT_MTE. So when restoring pages from swap we will > >> need to check to see if there are any saved tags even if !pte_tagged(). > >> > >> However don't check pages for which pte_access_permitted() returns false > >> as these will not have been swapped out. > >> > >> Signed-off-by: Steven Price <steven.price@arm.com> > >> --- > >> arch/arm64/include/asm/pgtable.h | 9 +++++++-- > >> arch/arm64/kernel/mte.c | 16 ++++++++++++++-- > >> 2 files changed, 21 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > >> index 0b10204e72fc..275178a810c1 100644 > >> --- a/arch/arm64/include/asm/pgtable.h > >> +++ b/arch/arm64/include/asm/pgtable.h > >> @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, > >> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) > >> __sync_icache_dcache(pte); > >> > >> - if (system_supports_mte() && > >> - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) > >> + /* > >> + * If the PTE would provide user space access to the tags associated > >> + * with it then ensure that the MTE tags are synchronised. Exec-only > >> + * mappings don't expose tags (instruction fetches don't check tags). > > > > I'm not sure I understand this comment. Of course, execution doesn't > > match tags. But the memory could still have tags associated with > > it. Does this mean such a page would lose its tags is swapped out? > > Hmm, I probably should have reread that - the context of the comment is > lost. > > I added the comment when changing to pte_access_permitted(), and the > comment on pte_access_permitted() explains a potential gotcha: > > * p??_access_permitted() is true for valid user mappings (PTE_USER > * bit set, subject to the write permission check). For execute-only > * mappings, like PROT_EXEC with EPAN (both PTE_USER and PTE_UXN bits > * not set) must return false. PROT_NONE mappings do not have the > * PTE_VALID bit set. > > So execute-only mappings return false even though that is effectively a > type of user access. However, because MTE checks are not performed by > the PE for instruction fetches this doesn't matter. I'll update the > comment, how about: > > /* > * If the PTE would provide user space access to the tags associated > * with it then ensure that the MTE tags are synchronised. Although > * pte_access_permitted() returns false for exec only mappings, they > * don't expose tags (instruction fetches don't check tags). > */ This looks fine to me. We basically want to check the PTE_VALID and PTE_USER bits and pte_access_permitted() does this (we could come up with a new macro name like pte_valid_user() but since we don't care about execute-only, it gets unnecessarily complicated).
On Mon, May 17, 2021 at 01:32:34PM +0100, Steven Price wrote: > A KVM guest could store tags in a page even if the VMM hasn't mapped > the page with PROT_MTE. So when restoring pages from swap we will > need to check to see if there are any saved tags even if !pte_tagged(). > > However don't check pages for which pte_access_permitted() returns false > as these will not have been swapped out. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > arch/arm64/include/asm/pgtable.h | 9 +++++++-- > arch/arm64/kernel/mte.c | 16 ++++++++++++++-- > 2 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index 0b10204e72fc..275178a810c1 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, > if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) > __sync_icache_dcache(pte); > > - if (system_supports_mte() && > - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) > + /* > + * If the PTE would provide user space access to the tags associated > + * with it then ensure that the MTE tags are synchronised. Exec-only > + * mappings don't expose tags (instruction fetches don't check tags). > + */ > + if (system_supports_mte() && pte_present(pte) && > + pte_access_permitted(pte, false) && !pte_special(pte)) > mte_sync_tags(ptep, pte); Looking at the mte_sync_page_tags() logic, we bail out early if it's the old pte is not a swap one and the new pte is not tagged. So we only need to call mte_sync_tags() if it's a tagged new pte or the old one is swap. What about changing the set_pte_at() test to: if (system_supports_mte() && pte_present(pte) && !pte_special(pte) && (pte_tagged(pte) || is_swap_pte(READ_ONCE(*ptep)))) mte_sync_tags(ptep, pte); We can even change mte_sync_tags() to take the old pte directly: if (system_supports_mte() && pte_present(pte) && !pte_special(pte)) { pte_t old_pte = READ_ONCE(*ptep); if (pte_tagged(pte) || is_swap_pte(old_pte)) mte_sync_tags(old_pte, pte); } It would save a function call in most cases where the page is not tagged.
On 19/05/2021 19:06, Catalin Marinas wrote: > On Mon, May 17, 2021 at 01:32:34PM +0100, Steven Price wrote: >> A KVM guest could store tags in a page even if the VMM hasn't mapped >> the page with PROT_MTE. So when restoring pages from swap we will >> need to check to see if there are any saved tags even if !pte_tagged(). >> >> However don't check pages for which pte_access_permitted() returns false >> as these will not have been swapped out. >> >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> arch/arm64/include/asm/pgtable.h | 9 +++++++-- >> arch/arm64/kernel/mte.c | 16 ++++++++++++++-- >> 2 files changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >> index 0b10204e72fc..275178a810c1 100644 >> --- a/arch/arm64/include/asm/pgtable.h >> +++ b/arch/arm64/include/asm/pgtable.h >> @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, >> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) >> __sync_icache_dcache(pte); >> >> - if (system_supports_mte() && >> - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) >> + /* >> + * If the PTE would provide user space access to the tags associated >> + * with it then ensure that the MTE tags are synchronised. Exec-only >> + * mappings don't expose tags (instruction fetches don't check tags). >> + */ >> + if (system_supports_mte() && pte_present(pte) && >> + pte_access_permitted(pte, false) && !pte_special(pte)) >> mte_sync_tags(ptep, pte); > > Looking at the mte_sync_page_tags() logic, we bail out early if it's the > old pte is not a swap one and the new pte is not tagged. So we only need > to call mte_sync_tags() if it's a tagged new pte or the old one is swap. > What about changing the set_pte_at() test to: > > if (system_supports_mte() && pte_present(pte) && !pte_special(pte) && > (pte_tagged(pte) || is_swap_pte(READ_ONCE(*ptep)))) > mte_sync_tags(ptep, pte); > > We can even change mte_sync_tags() to take the old pte directly: > > if (system_supports_mte() && pte_present(pte) && !pte_special(pte)) { > pte_t old_pte = READ_ONCE(*ptep); > if (pte_tagged(pte) || is_swap_pte(old_pte)) > mte_sync_tags(old_pte, pte); > } > > It would save a function call in most cases where the page is not > tagged. > Yes that looks like a good optimisation - although you've missed the pte_access_permitted() part of the check ;) The problem I hit is one of include dependencies: is_swap_pte() is defined (as a static inline) in include/linux/swapops.h. However the definition depends on pte_none()/pte_present() which are defined in pgtable.h - so there's a circular dependency. Open coding is_swap_pte() in set_pte_at() works, but it's a bit ugly. Any ideas on how to improve on the below? if (system_supports_mte() && pte_present(pte) && pte_access_permitted(pte, false) && !pte_special(pte)) { pte_t old_pte = READ_ONCE(*ptep); /* * We only need to synchronise if the new PTE has tags enabled * or if swapping in (in which case another mapping may have * set tags in the past even if this PTE isn't tagged). * (!pte_none() && !pte_present()) is an open coded version of * is_swap_pte() */ if (pte_tagged(pte) || (!pte_none(pte) && !pte_present(pte))) mte_sync_tags(old_pte, pte); } Steve
On Thu, May 20, 2021 at 12:55:21PM +0100, Steven Price wrote: > On 19/05/2021 19:06, Catalin Marinas wrote: > > On Mon, May 17, 2021 at 01:32:34PM +0100, Steven Price wrote: > >> A KVM guest could store tags in a page even if the VMM hasn't mapped > >> the page with PROT_MTE. So when restoring pages from swap we will > >> need to check to see if there are any saved tags even if !pte_tagged(). > >> > >> However don't check pages for which pte_access_permitted() returns false > >> as these will not have been swapped out. > >> > >> Signed-off-by: Steven Price <steven.price@arm.com> > >> --- > >> arch/arm64/include/asm/pgtable.h | 9 +++++++-- > >> arch/arm64/kernel/mte.c | 16 ++++++++++++++-- > >> 2 files changed, 21 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > >> index 0b10204e72fc..275178a810c1 100644 > >> --- a/arch/arm64/include/asm/pgtable.h > >> +++ b/arch/arm64/include/asm/pgtable.h > >> @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, > >> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) > >> __sync_icache_dcache(pte); > >> > >> - if (system_supports_mte() && > >> - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) > >> + /* > >> + * If the PTE would provide user space access to the tags associated > >> + * with it then ensure that the MTE tags are synchronised. Exec-only > >> + * mappings don't expose tags (instruction fetches don't check tags). > >> + */ > >> + if (system_supports_mte() && pte_present(pte) && > >> + pte_access_permitted(pte, false) && !pte_special(pte)) > >> mte_sync_tags(ptep, pte); > > > > Looking at the mte_sync_page_tags() logic, we bail out early if it's the > > old pte is not a swap one and the new pte is not tagged. So we only need > > to call mte_sync_tags() if it's a tagged new pte or the old one is swap. > > What about changing the set_pte_at() test to: > > > > if (system_supports_mte() && pte_present(pte) && !pte_special(pte) && > > (pte_tagged(pte) || is_swap_pte(READ_ONCE(*ptep)))) > > mte_sync_tags(ptep, pte); > > > > We can even change mte_sync_tags() to take the old pte directly: > > > > if (system_supports_mte() && pte_present(pte) && !pte_special(pte)) { > > pte_t old_pte = READ_ONCE(*ptep); > > if (pte_tagged(pte) || is_swap_pte(old_pte)) > > mte_sync_tags(old_pte, pte); > > } > > > > It would save a function call in most cases where the page is not > > tagged. > > Yes that looks like a good optimisation - although you've missed the > pte_access_permitted() part of the check ;) I was actually wondering if we could remove it. I don't think it buys us much as we have a pte_present() check already, so we know it is pointing to a valid page. Currently we'd only get a tagged pte on user mappings, same with swap entries. When vmalloc kasan_hw will be added, I think we have a set_pte_at() with a tagged pte but init_mm and high address (we might as well add a warning if addr > TASK_SIZE_64 on the mte_sync_tags path so that we don't forget). > The problem I hit is one of include dependencies: > > is_swap_pte() is defined (as a static inline) in > include/linux/swapops.h. However the definition depends on > pte_none()/pte_present() which are defined in pgtable.h - so there's a > circular dependency. > > Open coding is_swap_pte() in set_pte_at() works, but it's a bit ugly. > Any ideas on how to improve on the below? > > if (system_supports_mte() && pte_present(pte) && > pte_access_permitted(pte, false) && !pte_special(pte)) { > pte_t old_pte = READ_ONCE(*ptep); > /* > * We only need to synchronise if the new PTE has tags enabled > * or if swapping in (in which case another mapping may have > * set tags in the past even if this PTE isn't tagged). > * (!pte_none() && !pte_present()) is an open coded version of > * is_swap_pte() > */ > if (pte_tagged(pte) || (!pte_none(pte) && !pte_present(pte))) > mte_sync_tags(old_pte, pte); > } That's why I avoided testing my suggestion ;). I think we should just add !pte_none() in there with a comment that it may be a swap pte and use the is_swap_pte() again on the mte_sync_tags() path. We already have the pte_present() check.
On Thu, May 20, 2021 at 01:25:50PM +0100, Catalin Marinas wrote: > On Thu, May 20, 2021 at 12:55:21PM +0100, Steven Price wrote: > > The problem I hit is one of include dependencies: > > > > is_swap_pte() is defined (as a static inline) in > > include/linux/swapops.h. However the definition depends on > > pte_none()/pte_present() which are defined in pgtable.h - so there's a > > circular dependency. > > > > Open coding is_swap_pte() in set_pte_at() works, but it's a bit ugly. > > Any ideas on how to improve on the below? > > > > if (system_supports_mte() && pte_present(pte) && > > pte_access_permitted(pte, false) && !pte_special(pte)) { > > pte_t old_pte = READ_ONCE(*ptep); > > /* > > * We only need to synchronise if the new PTE has tags enabled > > * or if swapping in (in which case another mapping may have > > * set tags in the past even if this PTE isn't tagged). > > * (!pte_none() && !pte_present()) is an open coded version of > > * is_swap_pte() > > */ > > if (pte_tagged(pte) || (!pte_none(pte) && !pte_present(pte))) > > mte_sync_tags(old_pte, pte); > > } > > That's why I avoided testing my suggestion ;). I think we should just > add !pte_none() in there with a comment that it may be a swap pte and > use the is_swap_pte() again on the mte_sync_tags() path. We already have > the pte_present() check. Correction - pte_present() checks the new pte only, we need another for the old pte. So it looks like we'll open-code is_swap_pte().
On 20/05/2021 13:25, Catalin Marinas wrote: > On Thu, May 20, 2021 at 12:55:21PM +0100, Steven Price wrote: >> On 19/05/2021 19:06, Catalin Marinas wrote: >>> On Mon, May 17, 2021 at 01:32:34PM +0100, Steven Price wrote: >>>> A KVM guest could store tags in a page even if the VMM hasn't mapped >>>> the page with PROT_MTE. So when restoring pages from swap we will >>>> need to check to see if there are any saved tags even if !pte_tagged(). >>>> >>>> However don't check pages for which pte_access_permitted() returns false >>>> as these will not have been swapped out. >>>> >>>> Signed-off-by: Steven Price <steven.price@arm.com> >>>> --- >>>> arch/arm64/include/asm/pgtable.h | 9 +++++++-- >>>> arch/arm64/kernel/mte.c | 16 ++++++++++++++-- >>>> 2 files changed, 21 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >>>> index 0b10204e72fc..275178a810c1 100644 >>>> --- a/arch/arm64/include/asm/pgtable.h >>>> +++ b/arch/arm64/include/asm/pgtable.h >>>> @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, >>>> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) >>>> __sync_icache_dcache(pte); >>>> >>>> - if (system_supports_mte() && >>>> - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) >>>> + /* >>>> + * If the PTE would provide user space access to the tags associated >>>> + * with it then ensure that the MTE tags are synchronised. Exec-only >>>> + * mappings don't expose tags (instruction fetches don't check tags). >>>> + */ >>>> + if (system_supports_mte() && pte_present(pte) && >>>> + pte_access_permitted(pte, false) && !pte_special(pte)) >>>> mte_sync_tags(ptep, pte); >>> >>> Looking at the mte_sync_page_tags() logic, we bail out early if it's the >>> old pte is not a swap one and the new pte is not tagged. So we only need >>> to call mte_sync_tags() if it's a tagged new pte or the old one is swap. >>> What about changing the set_pte_at() test to: >>> >>> if (system_supports_mte() && pte_present(pte) && !pte_special(pte) && >>> (pte_tagged(pte) || is_swap_pte(READ_ONCE(*ptep)))) >>> mte_sync_tags(ptep, pte); >>> >>> We can even change mte_sync_tags() to take the old pte directly: >>> >>> if (system_supports_mte() && pte_present(pte) && !pte_special(pte)) { >>> pte_t old_pte = READ_ONCE(*ptep); >>> if (pte_tagged(pte) || is_swap_pte(old_pte)) >>> mte_sync_tags(old_pte, pte); >>> } >>> >>> It would save a function call in most cases where the page is not >>> tagged. >> >> Yes that looks like a good optimisation - although you've missed the >> pte_access_permitted() part of the check ;) > > I was actually wondering if we could remove it. I don't think it buys us > much as we have a pte_present() check already, so we know it is pointing > to a valid page. Currently we'd only get a tagged pte on user mappings, > same with swap entries. Actually the other way round makes more sense surely? pte_access_permitted() is true if both PTE_VALID & PTE_USER are set. pte_present() is true if *either* PTE_VALID or PTE_PROT_NONE are set. So the pte_present() is actually redundant. > When vmalloc kasan_hw will be added, I think we have a set_pte_at() with > a tagged pte but init_mm and high address (we might as well add a > warning if addr > TASK_SIZE_64 on the mte_sync_tags path so that we > don't forget). While we might not yet have tagged kernel pages - I'm not sure there's much point weakening the check to have to then check addr as well in the future. >> The problem I hit is one of include dependencies: >> >> is_swap_pte() is defined (as a static inline) in >> include/linux/swapops.h. However the definition depends on >> pte_none()/pte_present() which are defined in pgtable.h - so there's a >> circular dependency. >> >> Open coding is_swap_pte() in set_pte_at() works, but it's a bit ugly. >> Any ideas on how to improve on the below? >> >> if (system_supports_mte() && pte_present(pte) && >> pte_access_permitted(pte, false) && !pte_special(pte)) { >> pte_t old_pte = READ_ONCE(*ptep); >> /* >> * We only need to synchronise if the new PTE has tags enabled >> * or if swapping in (in which case another mapping may have >> * set tags in the past even if this PTE isn't tagged). >> * (!pte_none() && !pte_present()) is an open coded version of >> * is_swap_pte() >> */ >> if (pte_tagged(pte) || (!pte_none(pte) && !pte_present(pte))) >> mte_sync_tags(old_pte, pte); >> } > > That's why I avoided testing my suggestion ;). I think we should just > add !pte_none() in there with a comment that it may be a swap pte and > use the is_swap_pte() again on the mte_sync_tags() path. We already have > the pte_present() check. Well of course I didn't test the above beyond building - and I've screwed up because the open coded is_swap_pte() should have been called on old_pte not pte! So the pte_present() check above (which I've just removed...) is for the *new* PTE. So I think we need to keep both here. Steve
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index 0b10204e72fc..275178a810c1 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -314,8 +314,13 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte)) __sync_icache_dcache(pte); - if (system_supports_mte() && - pte_present(pte) && pte_tagged(pte) && !pte_special(pte)) + /* + * If the PTE would provide user space access to the tags associated + * with it then ensure that the MTE tags are synchronised. Exec-only + * mappings don't expose tags (instruction fetches don't check tags). + */ + if (system_supports_mte() && pte_present(pte) && + pte_access_permitted(pte, false) && !pte_special(pte)) mte_sync_tags(ptep, pte); __check_racy_pte_update(mm, ptep, pte); diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index c88e778c2fa9..a604818c52c1 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -33,11 +33,15 @@ DEFINE_STATIC_KEY_FALSE(mte_async_mode); EXPORT_SYMBOL_GPL(mte_async_mode); #endif -static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) +static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap, + bool pte_is_tagged) { unsigned long flags; pte_t old_pte = READ_ONCE(*ptep); + if (!is_swap_pte(old_pte) && !pte_is_tagged) + return; + spin_lock_irqsave(&tag_sync_lock, flags); /* Recheck with the lock held */ @@ -53,6 +57,9 @@ static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) } } + if (!pte_is_tagged) + goto out; + page_kasan_tag_reset(page); /* * We need smp_wmb() in between setting the flags and clearing the @@ -76,10 +83,15 @@ void mte_sync_tags(pte_t *ptep, pte_t pte) bool check_swap = nr_pages == 1; bool pte_is_tagged = pte_tagged(pte); + /* Early out if there's nothing to do */ + if (!check_swap && !pte_is_tagged) + return; + /* if PG_mte_tagged is set, tags have already been initialised */ for (i = 0; i < nr_pages; i++, page++) { if (!test_bit(PG_mte_tagged, &page->flags)) - mte_sync_page_tags(page, ptep, check_swap); + mte_sync_page_tags(page, ptep, check_swap, + pte_is_tagged); } }
A KVM guest could store tags in a page even if the VMM hasn't mapped the page with PROT_MTE. So when restoring pages from swap we will need to check to see if there are any saved tags even if !pte_tagged(). However don't check pages for which pte_access_permitted() returns false as these will not have been swapped out. Signed-off-by: Steven Price <steven.price@arm.com> --- arch/arm64/include/asm/pgtable.h | 9 +++++++-- arch/arm64/kernel/mte.c | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-)