| Message ID | 20180625113921.21854-3-yaojun8558363@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jun, On 25/06/18 12:39, Jun Yao wrote: > When CONFIG_ARM64_VA_BITS_36/CONFIG_ARM64_VA_BITS_39/ > CONFIG_ARM64_VA_BITS_42 are selected, a block-mapping can be > written to swapper_pg_dir. To defend 'KSMA', we move swapper_pg_dir > to .rodata section when these configurations are selected. At the > same time, we update swapper_pg_dir by fixmap. I don't think we should consider individual MMU configurations. KSMA is the motivation for making swapper_pg_dir read-only. When making swapper_pg_dir read-only it should work in the same way for all configurations as its simpler. > diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h > index 62908eeedcdc..881784b43965 100644 > --- a/arch/arm64/include/asm/fixmap.h > +++ b/arch/arm64/include/asm/fixmap.h > @@ -83,6 +83,7 @@ enum fixed_addresses { > FIX_PTE, > FIX_PMD, > FIX_PUD, > + FIX_SWAPPER, Please leave this as FIX_PGD that you removed in the previous patch. Depending on the folding pmd/pud can be used as the top level. If we always use the fixmap entry that goes with the name, its clear how these operate. > __end_of_fixed_addresses > }; > diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h > index 2e05bcd944c8..62512ad9c310 100644 > --- a/arch/arm64/include/asm/pgalloc.h > +++ b/arch/arm64/include/asm/pgalloc.h How come you don't touch pgd_populate()? Surely with a 4-level configuration we may need to put a new pud-table in when we use a fresh chunk of vmalloc space... (ah, this is because you are conditionally doing all this on configurations where the MMU has top-level block mappings. I think this is unnecessarily complicated, we should just do it all the time) > @@ -49,6 +49,22 @@ static inline void __pud_populate(pud_t *pudp, phys_addr_t pmdp, pudval_t prot) > > static inline void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t *pmdp) > { > +#ifdef CONFIG_ARM64_VA_BITS_39 Please make this depend on the number of levels not the VA size. We also have 3-level page tables for 64K with 48bit VA space, and 16K with 47bit VA space. I'd prefer some "if (CONFIG_PGTABLE_LEVELS == 3)" style check, as that way the compiler will always parse the code that is currently hidden behind #ifdefs. We can avoid use of fixmap entries for subsequent levels by making the check something like: | if (CONFIG_PGTABLE_LEVELS == 3 && magic_helper(pud) == swapper_pg_dir) > + if (mm == &init_mm) { > + pud_t *pud; > + unsigned long offset; > + extern spinlock_t pgdir_lock; > + > + spin_lock(&pgdir_lock); > + pud = (pud_t *)swapper_set_fixmap(); Can't we use pud_set_fixmap for pud levels? Do we need an extra spinlock?, won't init_mm.page_table_lock always be held? (we can put a lockdep_assert_held() in here to find cases that aren't). > + offset = (unsigned long)pudp - (unsigned long)swapper_pg_dir; > + pud = (pud_t *)((unsigned long)pud + offset); It feels like there should be a helper for this. I'm not comfortable assuming mm=init_mm means we are actually modifying swapper_pg_dir. Hibernate does this on the grounds that the tables it generates will be used as the kernel's mm when it starts restoring. This field wasn't used before, so it wasn't clear what the field was for. I'm sure we will find something else doing this... Could we test that the provided pudp is within swapper_pg_dir before invoking the fixmap path. > + __pud_populate(pud, __pa(pmdp), PMD_TYPE_TABLE); > + swapper_clear_fixmap(); > + spin_unlock(&pgdir_lock); > + return; > + } > +#endif > __pud_populate(pudp, __pa(pmdp), PMD_TYPE_TABLE); > } > #else > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > index 9677deb7b6c7..9db187024b44 100644 > --- a/arch/arm64/kernel/head.S > +++ b/arch/arm64/kernel/head.S (Changes here are cleanup, they should be in a separate patch) > @@ -300,7 +300,7 @@ __create_page_tables: > * dirty cache lines being evicted. > */ > adrp x0, idmap_pg_dir > - adrp x1, swapper_pg_end > + adrp x1, idmap_pg_end > sub x1, x1, x0 > bl __inval_dcache_area As we no longer modify swapper_pg_dir with the MMU disabled I think this is safe. > @@ -313,7 +313,7 @@ __create_page_tables: > * Clear the idmap and init page tables. > */ > adrp x0, idmap_pg_dir > - adrp x1, swapper_pg_end > + adrp x1, idmap_pg_end > sub x1, x1, x0 > clear_pages x0, x1 With the current placement of swapper_pg_dir after the BSS we rely on this to zero that memory. It may have junk left over from the boot loader as this is the area where the kernel's memory-footprint is bigger than the Image file. Your vmlinux.lds.S changes below conditionally move swapper_pg_dir around. Sometimes you need this, sometimes you don't. Please always keep these pgd in the read-only section. I think we should keep the page-zeroing as a sanity check, we only do it once during boot, and corruption here isn't something we would be able to debug easily. > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > index b0e4255fcba4..db72c4680f1d 100644 > --- a/arch/arm64/kernel/vmlinux.lds.S > +++ b/arch/arm64/kernel/vmlinux.lds.S > @@ -219,6 +219,28 @@ SECTIONS > . = ALIGN(PAGE_SIZE); > idmap_pg_dir = .; > . += IDMAP_DIR_SIZE; > + idmap_pg_end = .; > + > +#if defined(CONFIG_ARM64_VA_BITS_39) || \ > + defined(CONFIG_ARM64_VA_BITS_36) || \ > + defined(CONFIG_ARM64_VA_BITS_42) I think we should always do this read-only-swapper, even if there aren't any top-level block-mappings. The code will be much more maintainable this way. > + .rodata : { > +#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > + . = ALIGN(PAGE_SIZE); > + tramp_pg_dir = .; > + . += PAGE_SIZE; > +#endif > + > +#ifdef CONFIG_ARM64_SW_TTBR0_PAN > + reserved_ttbr0 = .; > + . += RESERVED_TTBR0_SIZE; > +#endif > + swapper_pg_dir = .; > + . += PAGE_SIZE; > + swapper_pg_end = .; > + } > + > +#else > > #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > tramp_pg_dir = .; > @@ -232,6 +254,7 @@ SECTIONS > swapper_pg_dir = .; > . += PAGE_SIZE; > swapper_pg_end = .; > +#endif Thanks, James
Hi Jun On 25/06/18 12:39, Jun Yao wrote: > When CONFIG_ARM64_VA_BITS_36/CONFIG_ARM64_VA_BITS_39/ > CONFIG_ARM64_VA_BITS_42 are selected, a block-mapping can be > written to swapper_pg_dir. To defend 'KSMA', we move swapper_pg_dir > to .rodata section when these configurations are selected. At the > same time, we update swapper_pg_dir by fixmap. > > Signed-off-by: Jun Yao <yaojun8558363@gmail.com> > --- > arch/arm64/include/asm/fixmap.h | 1 + > arch/arm64/include/asm/pgalloc.h | 33 ++++++++++++++++++++++++++++++++ > arch/arm64/include/asm/pgtable.h | 5 +++++ > arch/arm64/kernel/head.S | 6 +++--- > arch/arm64/kernel/vmlinux.lds.S | 23 ++++++++++++++++++++++ > arch/arm64/mm/mmu.c | 6 ++++++ > 6 files changed, 71 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h > index 62908eeedcdc..881784b43965 100644 > --- a/arch/arm64/include/asm/fixmap.h > +++ b/arch/arm64/include/asm/fixmap.h > @@ -83,6 +83,7 @@ enum fixed_addresses { > FIX_PTE, > FIX_PMD, > FIX_PUD, > + FIX_SWAPPER, > > __end_of_fixed_addresses > }; > diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h > index 2e05bcd944c8..62512ad9c310 100644 > --- a/arch/arm64/include/asm/pgalloc.h > +++ b/arch/arm64/include/asm/pgalloc.h > @@ -49,6 +49,22 @@ static inline void __pud_populate(pud_t *pudp, phys_addr_t pmdp, pudval_t prot) > > static inline void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t *pmdp) > { > +#ifdef CONFIG_ARM64_VA_BITS_39 Could we please use #ifdef __PAGETABLE_PUD_FOLDED ^^ > + } > +#endif > __pud_populate(pudp, __pa(pmdp), PMD_TYPE_TABLE); > } > #else > @@ -142,6 +158,23 @@ pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmdp, pte_t *ptep) > /* > * The pmd must be loaded with the physical address of the PTE table > */ > +#if defined(CONFIG_ARM64_VA_BITS_42) || \ > + defined(CONFIG_ARM64_VA_BITS_36) and #ifdef __PGTABLE_PMD_FOLDED here ^^ > +#if defined(CONFIG_ARM64_VA_BITS_39) || \ > + defined(CONFIG_ARM64_VA_BITS_36) || \ > + defined(CONFIG_ARM64_VA_BITS_42) and #ifdef __PGTABLE_PMD_FOLDED ^^ (as it implies __PGTABLE_PUD_FOLDED ) instead ? Cheers Suzuki
Hi Suzuki, Jun, On 27/06/18 12:07, Suzuki K Poulose wrote: > On 25/06/18 12:39, Jun Yao wrote: >> When CONFIG_ARM64_VA_BITS_36/CONFIG_ARM64_VA_BITS_39/ >> CONFIG_ARM64_VA_BITS_42 are selected, a block-mapping can be >> written to swapper_pg_dir. To defend 'KSMA', we move swapper_pg_dir >> to .rodata section when these configurations are selected. At the >> same time, we update swapper_pg_dir by fixmap. >> diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h >> index 2e05bcd944c8..62512ad9c310 100644 >> --- a/arch/arm64/include/asm/pgalloc.h >> +++ b/arch/arm64/include/asm/pgalloc.h >> @@ -49,6 +49,22 @@ static inline void __pud_populate(pud_t *pudp, phys_addr_t >> pmdp, pudval_t prot) >> static inline void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t >> *pmdp) >> { >> +#ifdef CONFIG_ARM64_VA_BITS_39 > > Could we please use > > #ifdef __PAGETABLE_PUD_FOLDED ^^ >> @@ -142,6 +158,23 @@ pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmdp, >> pte_t *ptep) >> /* >> * The pmd must be loaded with the physical address of the PTE table >> */ >> +#if defined(CONFIG_ARM64_VA_BITS_42) || \ >> + defined(CONFIG_ARM64_VA_BITS_36) > > and > > #ifdef __PGTABLE_PMD_FOLDED here ^^ > > >> +#if defined(CONFIG_ARM64_VA_BITS_39) || \ >> + defined(CONFIG_ARM64_VA_BITS_36) || \ >> + defined(CONFIG_ARM64_VA_BITS_42) > > and > > #ifdef __PGTABLE_PMD_FOLDED ^^ (as it implies > __PGTABLE_PUD_FOLDED ) > > instead ? Aha, I didn't know these existed. As we are only interested in matching one level for now, this is even clearer. Thanks, James
diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h index 62908eeedcdc..881784b43965 100644 --- a/arch/arm64/include/asm/fixmap.h +++ b/arch/arm64/include/asm/fixmap.h @@ -83,6 +83,7 @@ enum fixed_addresses { FIX_PTE, FIX_PMD, FIX_PUD, + FIX_SWAPPER, __end_of_fixed_addresses }; diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h index 2e05bcd944c8..62512ad9c310 100644 --- a/arch/arm64/include/asm/pgalloc.h +++ b/arch/arm64/include/asm/pgalloc.h @@ -49,6 +49,22 @@ static inline void __pud_populate(pud_t *pudp, phys_addr_t pmdp, pudval_t prot) static inline void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t *pmdp) { +#ifdef CONFIG_ARM64_VA_BITS_39 + if (mm == &init_mm) { + pud_t *pud; + unsigned long offset; + extern spinlock_t pgdir_lock; + + spin_lock(&pgdir_lock); + pud = (pud_t *)swapper_set_fixmap(); + offset = (unsigned long)pudp - (unsigned long)swapper_pg_dir; + pud = (pud_t *)((unsigned long)pud + offset); + __pud_populate(pud, __pa(pmdp), PMD_TYPE_TABLE); + swapper_clear_fixmap(); + spin_unlock(&pgdir_lock); + return; + } +#endif __pud_populate(pudp, __pa(pmdp), PMD_TYPE_TABLE); } #else @@ -142,6 +158,23 @@ pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmdp, pte_t *ptep) /* * The pmd must be loaded with the physical address of the PTE table */ +#if defined(CONFIG_ARM64_VA_BITS_42) || \ + defined(CONFIG_ARM64_VA_BITS_36) + if (mm == &init_mm) { + pmd_t *pmd; + unsigned long offset; + extern spinlock_t pgdir_lock; + + spin_lock(&pgdir_lock); + pmd = (pmd_t *)swapper_set_fixmap(); + offset = (unsigned long)pmdp - (unsigned long)swapper_pg_dir; + pmd = (pmd_t *)((unsigned long)pmd + offset); + __pmd_populate(pmd, __pa(ptep), PMD_TYPE_TABLE); + swapper_clear_fixmap(); + spin_unlock(&pgdir_lock); + return; + } +#endif __pmd_populate(pmdp, __pa(ptep), PMD_TYPE_TABLE); } diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index b2435e8b975b..85743ea0709b 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -592,6 +592,11 @@ static inline phys_addr_t pgd_page_paddr(pgd_t pgd) /* to find an entry in a kernel page-table-directory */ #define pgd_offset_k(addr) pgd_offset(&init_mm, addr) +#define swapper_set_fixmap() \ + set_fixmap_offset(FIX_SWAPPER, __pa_symbol(swapper_pg_dir)) + +#define swapper_clear_fixmap() clear_fixmap(FIX_SWAPPER) + static inline pte_t pte_modify(pte_t pte, pgprot_t newprot) { const pteval_t mask = PTE_USER | PTE_PXN | PTE_UXN | PTE_RDONLY | diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 9677deb7b6c7..9db187024b44 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -300,7 +300,7 @@ __create_page_tables: * dirty cache lines being evicted. */ adrp x0, idmap_pg_dir - adrp x1, swapper_pg_end + adrp x1, idmap_pg_end sub x1, x1, x0 bl __inval_dcache_area @@ -313,7 +313,7 @@ __create_page_tables: * Clear the idmap and init page tables. */ adrp x0, idmap_pg_dir - adrp x1, swapper_pg_end + adrp x1, idmap_pg_end sub x1, x1, x0 clear_pages x0, x1 @@ -404,7 +404,7 @@ __create_page_tables: * tables again to remove any speculatively loaded cache lines. */ adrp x0, idmap_pg_dir - adrp x1, swapper_pg_end + adrp x1, idmap_pg_end sub x1, x1, x0 dmb sy bl __inval_dcache_area diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S index b0e4255fcba4..db72c4680f1d 100644 --- a/arch/arm64/kernel/vmlinux.lds.S +++ b/arch/arm64/kernel/vmlinux.lds.S @@ -219,6 +219,28 @@ SECTIONS . = ALIGN(PAGE_SIZE); idmap_pg_dir = .; . += IDMAP_DIR_SIZE; + idmap_pg_end = .; + +#if defined(CONFIG_ARM64_VA_BITS_39) || \ + defined(CONFIG_ARM64_VA_BITS_36) || \ + defined(CONFIG_ARM64_VA_BITS_42) + .rodata : { +#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 + . = ALIGN(PAGE_SIZE); + tramp_pg_dir = .; + . += PAGE_SIZE; +#endif + +#ifdef CONFIG_ARM64_SW_TTBR0_PAN + reserved_ttbr0 = .; + . += RESERVED_TTBR0_SIZE; +#endif + swapper_pg_dir = .; + . += PAGE_SIZE; + swapper_pg_end = .; + } + +#else #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 tramp_pg_dir = .; @@ -232,6 +254,7 @@ SECTIONS swapper_pg_dir = .; . += PAGE_SIZE; swapper_pg_end = .; +#endif __pecoff_data_size = ABSOLUTE(. - __initdata_begin); _end = .; diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index a3b5f1dffb84..fbaf3e9b4a43 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -66,6 +66,12 @@ static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss; static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused; static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused; +#if defined(CONFIG_ARM64_VA_BITS_39) || \ + defined(CONFIG_ARM64_VA_BITS_36) || \ + defined(CONFIG_ARM64_VA_BITS_42) +DEFINE_SPINLOCK(pgdir_lock); +#endif + pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, unsigned long size, pgprot_t vma_prot) {
When CONFIG_ARM64_VA_BITS_36/CONFIG_ARM64_VA_BITS_39/ CONFIG_ARM64_VA_BITS_42 are selected, a block-mapping can be written to swapper_pg_dir. To defend 'KSMA', we move swapper_pg_dir to .rodata section when these configurations are selected. At the same time, we update swapper_pg_dir by fixmap. Signed-off-by: Jun Yao <yaojun8558363@gmail.com> --- arch/arm64/include/asm/fixmap.h | 1 + arch/arm64/include/asm/pgalloc.h | 33 ++++++++++++++++++++++++++++++++ arch/arm64/include/asm/pgtable.h | 5 +++++ arch/arm64/kernel/head.S | 6 +++--- arch/arm64/kernel/vmlinux.lds.S | 23 ++++++++++++++++++++++ arch/arm64/mm/mmu.c | 6 ++++++ 6 files changed, 71 insertions(+), 3 deletions(-)