Re: [kvm-unit-tests PATCH v3 04/18] lib: arm/arm64: Use WRITE_ONCE to update the translation tables

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 31 Dec 2019 16:09:35 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi,

> Use WRITE_ONCE to prevent store tearing when updating an entry in the
> translation tables. Without WRITE_ONCE, the compiler, even though it is
> unlikely, can emit several stores when changing the table, and we might
> end up with bogus TLB entries.
> 
> It's worth noting that the existing code is mostly fine without any
> changes because the translation tables are updated in one of the
> following situations:
> 
> - When the tables are being created with the MMU off, which means no TLB
>   caching is being performed.
> 
> - When new page table entries are added as a result of vmalloc'ing a
>   stack for a secondary CPU, which doesn't happen very often.
> 
> - When clearing the PTE_USER bit for the cache test, and store tearing
>   has no effect on the table walker because there are no intermediate
>   values between bit values 0 and 1. We still use WRITE_ONCE in this case
>   for consistency.
> 
> However, the functions are global and there is nothing preventing someone
> from writing a test that uses them in a different scenario. Let's make
> sure that when that happens, there will be no breakage once in a blue
> moon.

I haven't checked whether there are more places where this would be needed, but it seems the right thing to do, also the changes below look valid.
 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  lib/arm/asm/pgtable.h   | 12 ++++++++----
>  lib/arm64/asm/pgtable.h |  7 +++++--
>  lib/arm/mmu.c           | 19 +++++++++++++------
>  3 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> index 241dff69b38a..794514b8c927 100644
> --- a/lib/arm/asm/pgtable.h
> +++ b/lib/arm/asm/pgtable.h
> @@ -19,6 +19,8 @@
>   * because we always allocate their pages with alloc_page(), and
>   * alloc_page() always returns identity mapped pages.
>   */
> +#include <linux/compiler.h>
> +
>  #define pgtable_va(x)		((void *)(unsigned long)(x))
>  #define pgtable_pa(x)		((unsigned long)(x))
>  
> @@ -58,8 +60,9 @@ static inline pmd_t *pmd_alloc_one(void)
>  static inline pmd_t *pmd_alloc(pgd_t *pgd, unsigned long addr)
>  {
>  	if (pgd_none(*pgd)) {
> -		pmd_t *pmd = pmd_alloc_one();
> -		pgd_val(*pgd) = pgtable_pa(pmd) | PMD_TYPE_TABLE;
> +		pgd_t entry;
> +		pgd_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
> +		WRITE_ONCE(*pgd, entry);
>  	}
>  	return pmd_offset(pgd, addr);
>  }
> @@ -84,8 +87,9 @@ static inline pte_t *pte_alloc_one(void)
>  static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>  {
>  	if (pmd_none(*pmd)) {
> -		pte_t *pte = pte_alloc_one();
> -		pmd_val(*pmd) = pgtable_pa(pte) | PMD_TYPE_TABLE;
> +		pmd_t entry;
> +		pmd_val(entry) = pgtable_pa(pte_alloc_one()) | PMD_TYPE_TABLE;
> +		WRITE_ONCE(*pmd, entry);
>  	}
>  	return pte_offset(pmd, addr);
>  }
> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> index ee0a2c88cc18..dbf9e7253b71 100644
> --- a/lib/arm64/asm/pgtable.h
> +++ b/lib/arm64/asm/pgtable.h
> @@ -18,6 +18,8 @@
>  #include <asm/page.h>
>  #include <asm/pgtable-hwdef.h>
>  
> +#include <linux/compiler.h>
> +
>  /*
>   * We can convert va <=> pa page table addresses with simple casts
>   * because we always allocate their pages with alloc_page(), and
> @@ -66,8 +68,9 @@ static inline pte_t *pte_alloc_one(void)
>  static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>  {
>  	if (pmd_none(*pmd)) {
> -		pte_t *pte = pte_alloc_one();
> -		pmd_val(*pmd) = pgtable_pa(pte) | PMD_TYPE_TABLE;
> +		pmd_t entry;
> +		pmd_val(entry) = pgtable_pa(pte_alloc_one()) | PMD_TYPE_TABLE;
> +		WRITE_ONCE(*pmd, entry);
>  	}
>  	return pte_offset(pmd, addr);
>  }
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 5c31c00ccb31..86a829966a3c 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -17,6 +17,8 @@
>  #include <asm/pgtable-hwdef.h>
>  #include <asm/pgtable.h>
>  
> +#include <linux/compiler.h>
> +
>  extern unsigned long etext;
>  
>  pgd_t *mmu_idmap;
> @@ -86,7 +88,7 @@ static pteval_t *install_pte(pgd_t *pgtable, uintptr_t vaddr, pteval_t pte)
>  {
>  	pteval_t *p_pte = get_pte(pgtable, vaddr);
>  
> -	*p_pte = pte;
> +	WRITE_ONCE(*p_pte, pte);
>  	flush_tlb_page(vaddr);
>  	return p_pte;
>  }
> @@ -131,12 +133,15 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
>  	phys_addr_t paddr = phys_start & PGDIR_MASK;
>  	uintptr_t vaddr = virt_offset & PGDIR_MASK;
>  	uintptr_t virt_end = phys_end - paddr + vaddr;
> +	pgd_t *pgd;
> +	pgd_t entry;
>  
>  	for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
> -		pgd_t *pgd = pgd_offset(pgtable, vaddr);
> -		pgd_val(*pgd) = paddr;
> -		pgd_val(*pgd) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> -		pgd_val(*pgd) |= pgprot_val(prot);
> +		pgd_val(entry) = paddr;
> +		pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> +		pgd_val(entry) |= pgprot_val(prot);
> +		pgd = pgd_offset(pgtable, vaddr);
> +		WRITE_ONCE(*pgd, entry);
>  		flush_tlb_page(vaddr);
>  	}
>  }
> @@ -210,6 +215,7 @@ void mmu_clear_user(unsigned long vaddr)
>  {
>  	pgd_t *pgtable;
>  	pteval_t *pte;
> +	pteval_t entry;
>  
>  	if (!mmu_enabled())
>  		return;
> @@ -217,6 +223,7 @@ void mmu_clear_user(unsigned long vaddr)
>  	pgtable = current_thread_info()->pgtable;
>  	pte = get_pte(pgtable, vaddr);
>  
> -	*pte &= ~PTE_USER;
> +	entry = *pte & ~PTE_USER;
> +	WRITE_ONCE(*pte, entry);
>  	flush_tlb_page(vaddr);
>  }