Re: [RFC 11/12] arm64: tlbid: Pass domain to TLBI instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: mm: Dekker's algorithm for TLBID domains lacks Store-Load barriers
- [High] kvm: arm64: __tlbi macro incorrectly used for tlbip in ipas2e1is truncating IPA
--

commit f40d982ccca120afa0d082b4e36973eaef03f653
Author: Kristina Martšenko <kristina.martsenko@arm.com>
Subject: arm64: tlbid: Pass domain to TLBI instructions

This patch updates the TLB invalidation interfaces to pass a TLBI domain
to TLBI instructions when available, which may improve performance by
sending invalidations only to the necessary subset of CPUs. It uses the
mm domain bitmap to determine the domain and uses TLBIP instructions for
all shareable invalidations.

> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> index dac35568570ff..864d7459d5be7 100644
> --- a/arch/arm64/include/asm/tlbflush.h
> +++ b/arch/arm64/include/asm/tlbflush.h

[ ... ]

> @@ -219,22 +222,38 @@ static __always_inline void ipas2e1is(bool tlbip, u128 arg)
>  }

[Severity: High]
Does this code incorrectly invoke the 64-bit __tlbi macro instead of the
128-bit __tlbip macro for a TLBIP-formatted argument?

In ipas2e1is(), when tlbip is true, the code calls __tlbi(ipas2e1is, arg)
instead of __tlbip(ipas2e1is, arg). The __tlbi macro expands to a tlbi
assembly instruction which takes a single 64-bit register parameter. The
128-bit arg, which contains the target physical address (IPA) in its upper
64 bits, is silently truncated to 64 bits by the compiler when passed to
the inline assembly constraint.

Could this truncation cause the upper 64 bits containing the IPA to be lost,
resulting in the invalidation of IPA 0 instead of the requested address and
completely breaking stage-2 (guest) TLB invalidation?

[ ... ]

> @@ -418,12 +437,13 @@ static inline void flush_tlb_all(void)
>  
>  static inline void flush_tlb_mm(struct mm_struct *mm)
>  {
> -	unsigned long asid;
> +	unsigned long arg;
>  
>  	dsb(ishst);
> -	asid = __TLBI_VADDR(0, ASID(mm));
> -	__tlbi(aside1is, asid);
> -	__tlbi_user(aside1is, asid);
> +	arg = __TLBI_VADDR(0, ASID(mm));
> +	arg |= get_tlbid_domain(mm);

[Severity: High]
Is this barrier sufficient to prevent subsequent MMU Loads from reordering
before the domain Store?

The implementation of TLB invalidation domain tracking relies on a Dekker-like
synchronization between PTE updates and CPU context switches. The dsb(ishst)
used here acts as a Store-Store barrier.

Since ARM64's weakly ordered memory model allows subsequent Loads (like reading
the tlbid_domains bitmap) to be reordered before preceding Stores (like the
PTE update), could both threads miss each other's updates?

This might cause a TLB invalidation to skip a CPU that subsequently caches a
stale page table entry, potentially leading to memory corruption or
use-after-free in user space. Would a Store-Load barrier like dsb(ish) be
required here?

[ ... ]

> @@ -626,6 +661,7 @@ static __always_inline void __do_flush_tlb_range(struct vm_area_struct *vma,
>  {
>  	struct mm_struct *mm = vma->vm_mm;
>  	unsigned long asid, pages;
> +	int domain;
>  
>  	pages = (end - start) >> PAGE_SHIFT;
>  
> @@ -634,21 +670,23 @@ static __always_inline void __do_flush_tlb_range(struct vm_area_struct *vma,
>  		return;
>  	}
>  
> -	if (!(flags & TLBF_NOBROADCAST))
> +	if (!(flags & TLBF_NOBROADCAST)) {
>  		dsb(ishst);
> -	else
> +		domain = get_tlbid_domain(mm);

[Severity: High]
Similarly, is the dsb(ishst) barrier here sufficient for range TLB
invalidations?

Just as in flush_tlb_mm(), this Store-Store barrier may fail to prevent the
Load of tlbid_domains from reordering before the PTE Store.

> diff --git a/arch/arm64/mm/tlbid.c b/arch/arm64/mm/tlbid.c
> index b44fab2dca20d..c748e710e2ee9 100644
> --- a/arch/arm64/mm/tlbid.c
> +++ b/arch/arm64/mm/tlbid.c

[ ... ]

> @@ -31,6 +59,14 @@ void update_tlbid_domains(struct mm_struct *mm, unsigned int cpu)
>  
>  	for (int i = 0; i < BITS_TO_U64(domain_count); i++)
>  		atomic64_and(cpu_domains[cpu][i], &mm->context.tlbid_domains[i]);
> +
> +	/*
> +	 * Pairs with the DSB before reading tlbid_domains in the TLBI paths.
> +	 * Either a concurrent PTE update is visible to this CPU before it uses
> +	 * the mm, or the PTE updater observes the reduced domain bitmap and
> +	 * includes this CPU in its TLBI.
> +	 */
> +	dsb(ishst);
>  }

[Severity: High]
Is this barrier sufficient on the domain-update side of the Dekker-like
synchronization?

As with the PTE-update side, dsb(ishst) is a Store-Store barrier. Could this
allow subsequent Loads (like executing a speculative page table walk) to
reorder before the preceding domain bitmap Store?

If both the PTE updater and the domain updater use Store-Store barriers, the
Dekker-like synchronization lacks the necessary Store-Load ordering,
potentially leading to missed TLB invalidations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001110655.461473-1-kristina.martsenko@arm.com?part=11