[RFC PATCH] arm: Fix cache inconsistency when using fixmap

[tixy@linaro.org (Jon Medhurst (Tixy))]

On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote:

> However, we could make this a bit more private to the fixmap code by
> using something like
> 
> 
> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> index 5c17d2dec777..6c375aea8f22 100644
> --- a/arch/arm/include/asm/fixmap.h
> +++ b/arch/arm/include/asm/fixmap.h
> @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses =
> 
>  #define FIXMAP_PAGE_COMMON     (L_PTE_YOUNG | L_PTE_PRESENT |
> L_PTE_XN | L_PTE_DIRTY)
> 
> -#define FIXMAP_PAGE_NORMAL     (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
> -#define FIXMAP_PAGE_RO         (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
> +#define __FIXMAP_MT_NORMAL     ((pgprot_val(pgprot_kernel) ?:

The code above misses out setting L_PTE_XN when using pgprot_kernel

> FIXMAP_PAGE_COMMON) | \
> +                                L_PTE_MT_WRITEBACK)
> +#define FIXMAP_PAGE_NORMAL     __pgprot(__FIXMAP_MT_NORMAL)
> +#define FIXMAP_PAGE_RO         __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY)
> 
>  /* Used by set_fixmap_(io|nocache), both meant for mapping a device */
> -#define FIXMAP_PAGE_IO         (FIXMAP_PAGE_COMMON |
> L_PTE_MT_DEV_SHARED | L_PTE_SHARED)
> +#define FIXMAP_PAGE_IO         __pgprot(FIXMAP_PAGE_COMMON |
> L_PTE_MT_DEV_SHARED | \
> +                                        L_PTE_SHARED)
>  #define FIXMAP_PAGE_NOCACHE    FIXMAP_PAGE_IO
> 
>  #define __early_set_fixmap     __set_fixmap
> 
> (and drop the change to mm/mmu.c), which boils down to the same for
> fixmap but does not affect other users of pgprot_kernel. Also, it
> appears these definitions are broken under STRICT_MM_TYPECHECKS so
> this is a good opportunity to get that fixed as well.

I like this method better because as you say it keeps things private to
fixmap, and it doesn't risk affecting other things.

As for getting STRICT_MM_TYPECHECKS working that looks good too, but
should be a separate cleanup patch, especially as a fix for the cache
problem possibly should go to stable kernels.

I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__')
as it's an implementation detail an not a memory type used with fixmap.

So I was thinking, one patch as a bugfix:

----------------------------------------------------------------------
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
index 5c17d2dec777..4e6784dc5668 100644
--- a/arch/arm/include/asm/fixmap.h
+++ b/arch/arm/include/asm/fixmap.h
@@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses =
 
 #define FIXMAP_PAGE_COMMON	(L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY)
 
-#define FIXMAP_PAGE_NORMAL	(FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
+#define FIXMAP_PAGE_NORMAL	(pgprot_kernel ? pgprot_kernel | L_PTE_XN : \
+					FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
 #define FIXMAP_PAGE_RO		(FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
 
 /* Used by set_fixmap_(io|nocache), both meant for mapping a device */
----------------------------------------------------------------------

Second patch as a cleanup. Note this is different to Ard's version as
it uses PAGE_KERNEL rather than open coding the same code from
pgtable.h to add XN permisions (Also, the default generic definition of
FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change
it is to select an alternate value if that is not yet setup)

----------------------------------------------------------------------
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
index 4e6784dc5668..ec689c6aa644 100644
--- a/arch/arm/include/asm/fixmap.h
+++ b/arch/arm/include/asm/fixmap.h
@@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses =
 	__end_of_fixmap_region > __end_of_early_ioremap_region ?
 	__end_of_fixmap_region : __end_of_early_ioremap_region;
 
-#define FIXMAP_PAGE_COMMON	(L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY)
+#define __FIXMAP_PAGE_COMMON	(L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY)
 
-#define FIXMAP_PAGE_NORMAL	(pgprot_kernel ? pgprot_kernel | L_PTE_XN : \
-					FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
-#define FIXMAP_PAGE_RO		(FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
+#define FIXMAP_PAGE_NORMAL	(pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \
+				 __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK))
+#define FIXMAP_PAGE_RO		_MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY)
 
 /* Used by set_fixmap_(io|nocache), both meant for mapping a device */
-#define FIXMAP_PAGE_IO		(FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | L_PTE_SHARED)
+#define FIXMAP_PAGE_IO         __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | \
+                                        L_PTE_SHARED)
 #define FIXMAP_PAGE_NOCACHE	FIXMAP_PAGE_IO
 
 #define __early_set_fixmap	__set_fixmap
----------------------------------------------------------------------