Re: [PATCH 1/6 v14] ARM: Handle a device tree in lowmem

[Ard Biesheuvel <ardb@kernel.org>]

On Thu, 1 Oct 2020 at 17:22, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> If the kernel grows big, it can happen that the kernel does not
> fit in the first memory block. This is normally out-of-spec
> but it turns out that the boot usually survives this.
>
> What it does not survive is that the prepare_page_table() code
> wipes all PMDs (page table pointers for the virtual memory)
> over lowmem, including the PMD where the attached DTB is
> stored. (The variable is named atags but this only really
> happens for DTBs.)
>
> Let's deal with this.
>
> First, this makes the code jump over two PMDs where the DTB
> is stored if it ends up in lowmem: it could happen that the
> DTB crosses a PMD_SIZE boundary so to be on the safe side we
> need to skip over the one we know it starts in and also the
> next.
>
> Next we make ARM use unflatten_and_copy_device_tree() instead
> of just unflattening it where it is. Currently we just assume
> that the device tree will be in some kind of "safe" memory and
> as proven by KASan it will sometimes end up in the kernel lowmem
> which will create crashes as the kernel will clear all lowmem
> PMDs while initializing the memory manager.
>
> Finally, we add a new call to the MMU code to go in and clear
> out the PMDs in lowmem if the device tree was there.
>
> Example boot log on an affected system:
>
> ATAGs/DTB found in lowmem, skip clearing PMD @0xc3000000
> ATAGs/DTB found in lowmem, skip clearing PMD @0xc3200000
> (...)
> DTB @430af0e0 (physical) copied to @cc394d80 (virtual)
> Clear ATAGs/DTB PMD @0xc3000000
> Clear ATAGs/DTB PMD @0xc3200000
>
> This fixes the problem where the Qualcomm APQ8060 would not
> boot while using KASan.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

OK, so if I am understanding this correctly, the root problem is that
the kernel unmaps the memory that the attached DTB resides in, right?
So how is it possible that the kernel does not fit in the first
memblock? Doesn't that mean that we are using memory that is not
available to begin with?

Do you have a dump of the memory layout on the platform in question?
How do 0xc3000000/0xc3200000 map onto physical addresses, and how are
those described?

If that memory is not actually available to begin with, this fix
papers over the problem, and we should be fixing this in the
decompressor instead.


> ---
> ChangeLog v13->v14:
> - New patch fixing the regression when using KASan on at least
>   one of the affected platforms.
> ---
>  arch/arm/include/asm/prom.h |  5 +++++
>  arch/arm/kernel/devtree.c   | 14 +++++++++++++-
>  arch/arm/kernel/setup.c     |  8 ++++++--
>  arch/arm/mm/mmu.c           | 37 +++++++++++++++++++++++++++++++++++--
>  4 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
> index 1e36c40533c1..41e189a2631c 100644
> --- a/arch/arm/include/asm/prom.h
> +++ b/arch/arm/include/asm/prom.h
> @@ -11,6 +11,7 @@
>
>  extern const struct machine_desc *setup_machine_fdt(unsigned int dt_phys);
>  extern void __init arm_dt_init_cpu_maps(void);
> +void __init setup_machine_copy_fdt(unsigned int dt_phys);
>
>  #else /* CONFIG_OF */
>
> @@ -21,5 +22,9 @@ static inline const struct machine_desc *setup_machine_fdt(unsigned int dt_phys)
>
>  static inline void arm_dt_init_cpu_maps(void) { }
>
> +static inline void setup_machine_copy_fdt(unsigned int dt_phys)
> +{
> +}
> +
>  #endif /* CONFIG_OF */
>  #endif /* ASMARM_PROM_H */
> diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> index 39c978698406..267011e133a7 100644
> --- a/arch/arm/kernel/devtree.c
> +++ b/arch/arm/kernel/devtree.c
> @@ -23,7 +23,7 @@
>  #include <asm/smp_plat.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach-types.h>
> -
> +#include <asm/mach/map.h>
>
>  #ifdef CONFIG_SMP
>  extern struct of_cpu_method __cpu_method_of_table[];
> @@ -257,3 +257,15 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
>
>         return mdesc;
>  }
> +
> +extern void arm_mm_clear_atag_pages(void);
> +
> +void __init setup_machine_copy_fdt(unsigned int dt_phys)
> +{
> +       /* This will point the DT parser to the new location */
> +       unflatten_and_copy_device_tree();
> +       pr_info("DTB @%08x (physical) copied to @%px (virtual)\n",
> +               dt_phys, initial_boot_params);
> +       /* Clear out any mappings over the device tree */
> +       arm_mm_clear_atag_pages();
> +}
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index d8e18cdd96d3..d0562cdc919e 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -1076,11 +1076,14 @@ void __init hyp_mode_check(void)
>  void __init setup_arch(char **cmdline_p)
>  {
>         const struct machine_desc *mdesc;
> +       bool using_dt = false;
>
>         setup_processor();
>         mdesc = setup_machine_fdt(__atags_pointer);
>         if (!mdesc)
>                 mdesc = setup_machine_tags(__atags_pointer, __machine_arch_type);
> +       else
> +               using_dt = true;
>         if (!mdesc) {
>                 early_print("\nError: invalid dtb and unrecognized/unsupported machine ID\n");
>                 early_print("  r1=0x%08x, r2=0x%08x\n", __machine_arch_type,
> @@ -1109,7 +1112,6 @@ void __init setup_arch(char **cmdline_p)
>
>         early_fixmap_init();
>         early_ioremap_init();
> -
>         parse_early_param();
>
>  #ifdef CONFIG_MMU
> @@ -1135,7 +1137,9 @@ void __init setup_arch(char **cmdline_p)
>         if (mdesc->restart)
>                 arm_pm_restart = mdesc->restart;
>
> -       unflatten_device_tree();
> +       /* Remap and copy the device tree */
> +       if (using_dt)
> +               setup_machine_copy_fdt(__atags_pointer);
>
>         arm_dt_init_cpu_maps();
>         psci_dt_init();
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index c36f977b2ccb..732c856c357d 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -68,6 +68,8 @@ pgprot_t pgprot_kernel;
>  EXPORT_SYMBOL(pgprot_user);
>  EXPORT_SYMBOL(pgprot_kernel);
>
> +extern unsigned long __atags_pointer;
> +
>  struct cachepolicy {
>         const char      policy[16];
>         unsigned int    cr_mask;
> @@ -1258,6 +1260,7 @@ void __init adjust_lowmem_bounds(void)
>  static inline void prepare_page_table(void)
>  {
>         unsigned long addr;
> +       unsigned long atags;
>         phys_addr_t end;
>
>         /*
> @@ -1284,9 +1287,40 @@ static inline void prepare_page_table(void)
>          * Clear out all the kernel space mappings, except for the first
>          * memory bank, up to the vmalloc region.
>          */
> +       atags = __phys_to_virt(__atags_pointer);
> +       atags &= PMD_MASK;
>         for (addr = __phys_to_virt(end);
> -            addr < VMALLOC_START; addr += PMD_SIZE)
> +            addr < VMALLOC_START; addr += PMD_SIZE) {
> +               if (addr >= atags && addr < (atags + 2 * PMD_SIZE)) {
> +                       pr_info("ATAGs/DTB found in lowmem, skip clearing PMD @0x%08lx\n", addr);
> +                       continue;
> +               }
>                 pmd_clear(pmd_off_k(addr));
> +       }
> +}
> +
> +void __init arm_mm_clear_atag_pages(void)
> +{
> +       unsigned long addr;
> +       unsigned long atags;
> +       phys_addr_t end;
> +
> +       /*
> +        * Clear out the kernel space mappings used by the attached DTB if
> +        * it happens to end up in lowmem.
> +        */
> +       end = memblock.memory.regions[0].base + memblock.memory.regions[0].size;
> +       if (end >= arm_lowmem_limit)
> +               end = arm_lowmem_limit;
> +       atags = __phys_to_virt(__atags_pointer);
> +       atags &= PMD_MASK;
> +       for (addr = __phys_to_virt(end);
> +            addr < VMALLOC_START; addr += PMD_SIZE) {
> +               if (addr >= atags && addr < (atags + 2 * PMD_SIZE)) {
> +                       pr_info("Clear ATAGs/DTB PMD @0x%08lx\n", addr);
> +                       pmd_clear(pmd_off_k(addr));
> +               }
> +       }
>  }
>
>  #ifdef CONFIG_ARM_LPAE
> @@ -1503,7 +1537,6 @@ static void __init map_lowmem(void)
>  }
>
>  #ifdef CONFIG_ARM_PV_FIXUP
> -extern unsigned long __atags_pointer;
>  typedef void pgtables_remap(long long offset, unsigned long pgd, void *bdata);
>  pgtables_remap lpae_pgtables_remap_asm;
>
> --
> 2.26.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel