| Message ID | 20190921010002.61006-1-aou@eecs.berkeley.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Fix memblock reservation for device tree blob | expand |
On Sat, Sep 21, 2019 at 6:30 AM Albert Ou <aou@eecs.berkeley.edu> wrote: > > This fixes an error with how the FDT blob is reserved in memblock. > An incorrect physical address calculation exposed the FDT header to > unintended corruption, which typically manifested with of_fdt_raw_init() > faulting during late boot after fdt_totalsize() returned a wrong value. > Systems with smaller physical memory sizes more frequently trigger this > issue, as the kernel is more likely to allocate from the DMA32 zone > where bbl places the DTB after the kernel image. > > Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") > changed the mapping of the DTB to reside in the fixmap area. > Consequently, early_init_fdt_reserve_self() cannot be used anymore in > setup_bootmem() since it relies on __pa() to derive a physical address, > which does not work with dtb_early_va that is no longer a valid kernel > logical address. > > The reserved[0x1] region shows the effect of the pointer underflow > resulting from the __pa(initial_boot_params) offset subtraction: > > [ 0.000000] MEMBLOCK configuration: > [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 > [ 0.000000] memory.cnt = 0x1 > [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 > [ 0.000000] reserved.cnt = 0x2 > [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 > [ 0.000000] reserved[0x1] [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0 > > With the fix applied: > > [ 0.000000] MEMBLOCK configuration: > [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 > [ 0.000000] memory.cnt = 0x1 > [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 > [ 0.000000] reserved.cnt = 0x2 > [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 > [ 0.000000] reserved[0x1] [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0 Thanks for catching this issue. Most of us did not notice this issue most likely because: 1. We generally have good enough RAM on QEMU and SiFive Unleashed 2. Most of people use OpenSBI FW_JUMP on QEMU and U-Boot on SiFive Unleashed to boot in Linux which places FDT quite far away from Linux kernel end Linux ARM64 kernel also uses FIXMAP to access FDT and over there as well early_init_fdt_reserve_self() is not used. > > Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") > Signed-off-by: Albert Ou <aou@eecs.berkeley.edu> > --- > arch/riscv/mm/init.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > index f0ba713..52d007c 100644 > --- a/arch/riscv/mm/init.c > +++ b/arch/riscv/mm/init.c > @@ -11,6 +11,7 @@ > #include <linux/swap.h> > #include <linux/sizes.h> > #include <linux/of_fdt.h> > +#include <linux/libfdt.h> > > #include <asm/fixmap.h> > #include <asm/tlbflush.h> > @@ -82,6 +83,8 @@ static void __init setup_initrd(void) > } > #endif /* CONFIG_BLK_DEV_INITRD */ > > +static phys_addr_t __dtb_pa __initdata; May be dtb_early_pa will be more consistent name instead of __dtb_pa because it matches dtb_early_va used below. > + > void __init setup_bootmem(void) > { > struct memblock_region *reg; > @@ -117,7 +120,12 @@ void __init setup_bootmem(void) > setup_initrd(); > #endif /* CONFIG_BLK_DEV_INITRD */ > > - early_init_fdt_reserve_self(); > + /* > + * Avoid using early_init_fdt_reserve_self() since __pa() does > + * not work for DTB pointers that are fixmap addresses > + */ > + memblock_reserve(__dtb_pa, fdt_totalsize(dtb_early_va)); > + > early_init_fdt_scan_reserved_mem(); > memblock_allow_resize(); > memblock_dump_all(); > @@ -333,6 +341,7 @@ static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size) > "not use absolute addressing." > #endif > > + Please remove this newline addition. > asmlinkage void __init setup_vm(uintptr_t dtb_pa) > { > uintptr_t va, end_va; > @@ -393,6 +402,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa) > > /* Save pointer to DTB for early FDT parsing */ > dtb_early_va = (void *)fix_to_virt(FIX_FDT) + (dtb_pa & ~PAGE_MASK); > + /* Save physical address for memblock reservation */ > + __dtb_pa = dtb_pa; > } > > static void __init setup_vm_final(void) > -- > 2.7.4 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv This deserves to be stable kernel fix as well. You should add: Cc: stable@vger.kernel.org in your commit description. Apart from minor nits above. Reviewed-by: Anup Patel <anup@brainfault.org> I tried this patch for both RV64 and RV32 on QEMU with Yocto rootfs. Tested-by: Anup Patel <anup@brainfault.org> Regards, Anup
On Fri, 20 Sep 2019 21:34:57 PDT (-0700), anup@brainfault.org wrote: > On Sat, Sep 21, 2019 at 6:30 AM Albert Ou <aou@eecs.berkeley.edu> wrote: >> >> This fixes an error with how the FDT blob is reserved in memblock. >> An incorrect physical address calculation exposed the FDT header to >> unintended corruption, which typically manifested with of_fdt_raw_init() >> faulting during late boot after fdt_totalsize() returned a wrong value. >> Systems with smaller physical memory sizes more frequently trigger this >> issue, as the kernel is more likely to allocate from the DMA32 zone >> where bbl places the DTB after the kernel image. >> >> Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") >> changed the mapping of the DTB to reside in the fixmap area. >> Consequently, early_init_fdt_reserve_self() cannot be used anymore in >> setup_bootmem() since it relies on __pa() to derive a physical address, >> which does not work with dtb_early_va that is no longer a valid kernel >> logical address. >> >> The reserved[0x1] region shows the effect of the pointer underflow >> resulting from the __pa(initial_boot_params) offset subtraction: >> >> [ 0.000000] MEMBLOCK configuration: >> [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 >> [ 0.000000] memory.cnt = 0x1 >> [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 >> [ 0.000000] reserved.cnt = 0x2 >> [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 >> [ 0.000000] reserved[0x1] [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0 >> >> With the fix applied: >> >> [ 0.000000] MEMBLOCK configuration: >> [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 >> [ 0.000000] memory.cnt = 0x1 >> [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 >> [ 0.000000] reserved.cnt = 0x2 >> [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 >> [ 0.000000] reserved[0x1] [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0 > > Thanks for catching this issue. > > Most of us did not notice this issue most likely because: > 1. We generally have good enough RAM on QEMU and SiFive Unleashed > 2. Most of people use OpenSBI FW_JUMP on QEMU and U-Boot on > SiFive Unleashed to boot in Linux which places FDT quite far away > from Linux kernel end > > Linux ARM64 kernel also uses FIXMAP to access FDT and over there > as well early_init_fdt_reserve_self() is not used. > >> >> Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") >> Signed-off-by: Albert Ou <aou@eecs.berkeley.edu> >> --- >> arch/riscv/mm/init.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c >> index f0ba713..52d007c 100644 >> --- a/arch/riscv/mm/init.c >> +++ b/arch/riscv/mm/init.c >> @@ -11,6 +11,7 @@ >> #include <linux/swap.h> >> #include <linux/sizes.h> >> #include <linux/of_fdt.h> >> +#include <linux/libfdt.h> >> >> #include <asm/fixmap.h> >> #include <asm/tlbflush.h> >> @@ -82,6 +83,8 @@ static void __init setup_initrd(void) >> } >> #endif /* CONFIG_BLK_DEV_INITRD */ >> >> +static phys_addr_t __dtb_pa __initdata; > > May be dtb_early_pa will be more consistent name > instead of __dtb_pa because it matches dtb_early_va > used below. > >> + >> void __init setup_bootmem(void) >> { >> struct memblock_region *reg; >> @@ -117,7 +120,12 @@ void __init setup_bootmem(void) >> setup_initrd(); >> #endif /* CONFIG_BLK_DEV_INITRD */ >> >> - early_init_fdt_reserve_self(); >> + /* >> + * Avoid using early_init_fdt_reserve_self() since __pa() does >> + * not work for DTB pointers that are fixmap addresses >> + */ >> + memblock_reserve(__dtb_pa, fdt_totalsize(dtb_early_va)); >> + >> early_init_fdt_scan_reserved_mem(); >> memblock_allow_resize(); >> memblock_dump_all(); >> @@ -333,6 +341,7 @@ static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size) >> "not use absolute addressing." >> #endif >> >> + > > Please remove this newline addition. > >> asmlinkage void __init setup_vm(uintptr_t dtb_pa) >> { >> uintptr_t va, end_va; >> @@ -393,6 +402,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa) >> >> /* Save pointer to DTB for early FDT parsing */ >> dtb_early_va = (void *)fix_to_virt(FIX_FDT) + (dtb_pa & ~PAGE_MASK); >> + /* Save physical address for memblock reservation */ >> + __dtb_pa = dtb_pa; >> } >> >> static void __init setup_vm_final(void) >> -- >> 2.7.4 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > This deserves to be stable kernel fix as well. > You should add: > Cc: stable@vger.kernel.org > in your commit description. > > Apart from minor nits above. > > Reviewed-by: Anup Patel <anup@brainfault.org> > > I tried this patch for both RV64 and RV32 on QEMU with > Yocto rootfs. > > Tested-by: Anup Patel <anup@brainfault.org> > > Regards, > Anup Albert: Do you plan on spinning a v2 of the patch set?
On 2019-10-08 15:38:15 -0700, Palmer Dabbelt <palmer@sifive.com> wrote: > On Fri, 20 Sep 2019 21:34:57 PDT (-0700), anup@brainfault.org wrote: > > On Sat, Sep 21, 2019 at 6:30 AM Albert Ou <aou@eecs.berkeley.edu> wrote: > >> > >> This fixes an error with how the FDT blob is reserved in memblock. > >> An incorrect physical address calculation exposed the FDT header to > >> unintended corruption, which typically manifested with of_fdt_raw_init() > >> faulting during late boot after fdt_totalsize() returned a wrong value. > >> Systems with smaller physical memory sizes more frequently trigger this > >> issue, as the kernel is more likely to allocate from the DMA32 zone > >> where bbl places the DTB after the kernel image. > >> > >> Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") > >> changed the mapping of the DTB to reside in the fixmap area. > >> Consequently, early_init_fdt_reserve_self() cannot be used anymore in > >> setup_bootmem() since it relies on __pa() to derive a physical address, > >> which does not work with dtb_early_va that is no longer a valid kernel > >> logical address. > >> > >> The reserved[0x1] region shows the effect of the pointer underflow > >> resulting from the __pa(initial_boot_params) offset subtraction: > >> > >> [ 0.000000] MEMBLOCK configuration: > >> [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 > >> [ 0.000000] memory.cnt = 0x1 > >> [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 > >> [ 0.000000] reserved.cnt = 0x2 > >> [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 > >> [ 0.000000] reserved[0x1] [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0 > >> > >> With the fix applied: > >> > >> [ 0.000000] MEMBLOCK configuration: > >> [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 > >> [ 0.000000] memory.cnt = 0x1 > >> [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 > >> [ 0.000000] reserved.cnt = 0x2 > >> [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 > >> [ 0.000000] reserved[0x1] [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0 > > > > Thanks for catching this issue. > > > > Most of us did not notice this issue most likely because: > > 1. We generally have good enough RAM on QEMU and SiFive Unleashed > > 2. Most of people use OpenSBI FW_JUMP on QEMU and U-Boot on > > SiFive Unleashed to boot in Linux which places FDT quite far away > > from Linux kernel end > > > > Linux ARM64 kernel also uses FIXMAP to access FDT and over there > > as well early_init_fdt_reserve_self() is not used. > > > >> > >> Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") > >> Signed-off-by: Albert Ou <aou@eecs.berkeley.edu> > >> --- > >> arch/riscv/mm/init.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > >> index f0ba713..52d007c 100644 > >> --- a/arch/riscv/mm/init.c > >> +++ b/arch/riscv/mm/init.c > >> @@ -11,6 +11,7 @@ > >> #include <linux/swap.h> > >> #include <linux/sizes.h> > >> #include <linux/of_fdt.h> > >> +#include <linux/libfdt.h> > >> > >> #include <asm/fixmap.h> > >> #include <asm/tlbflush.h> > >> @@ -82,6 +83,8 @@ static void __init setup_initrd(void) > >> } > >> #endif /* CONFIG_BLK_DEV_INITRD */ > >> > >> +static phys_addr_t __dtb_pa __initdata; > > > > May be dtb_early_pa will be more consistent name > > instead of __dtb_pa because it matches dtb_early_va > > used below. > > > >> + > >> void __init setup_bootmem(void) > >> { > >> struct memblock_region *reg; > >> @@ -117,7 +120,12 @@ void __init setup_bootmem(void) > >> setup_initrd(); > >> #endif /* CONFIG_BLK_DEV_INITRD */ > >> > >> - early_init_fdt_reserve_self(); > >> + /* > >> + * Avoid using early_init_fdt_reserve_self() since __pa() does > >> + * not work for DTB pointers that are fixmap addresses > >> + */ > >> + memblock_reserve(__dtb_pa, fdt_totalsize(dtb_early_va)); > >> + > >> early_init_fdt_scan_reserved_mem(); > >> memblock_allow_resize(); > >> memblock_dump_all(); > >> @@ -333,6 +341,7 @@ static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size) > >> "not use absolute addressing." > >> #endif > >> > >> + > > > > Please remove this newline addition. > > > >> asmlinkage void __init setup_vm(uintptr_t dtb_pa) > >> { > >> uintptr_t va, end_va; > >> @@ -393,6 +402,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa) > >> > >> /* Save pointer to DTB for early FDT parsing */ > >> dtb_early_va = (void *)fix_to_virt(FIX_FDT) + (dtb_pa & ~PAGE_MASK); > >> + /* Save physical address for memblock reservation */ > >> + __dtb_pa = dtb_pa; > >> } > >> > >> static void __init setup_vm_final(void) > >> -- > >> 2.7.4 > >> > >> > >> _______________________________________________ > >> linux-riscv mailing list > >> linux-riscv@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > This deserves to be stable kernel fix as well. > > You should add: > > Cc: stable@vger.kernel.org > > in your commit description. > > > > Apart from minor nits above. > > > > Reviewed-by: Anup Patel <anup@brainfault.org> > > > > I tried this patch for both RV64 and RV32 on QEMU with > > Yocto rootfs. > > > > Tested-by: Anup Patel <anup@brainfault.org> > > > > Regards, > > Anup > > Albert: Do you plan on spinning a v2 of the patch set? > v2 was sent last week and has already been applied as 922b0375fc93fb1a20c5617e37c389c26bbccb70 by Paul.
On Tue, 08 Oct 2019 16:31:17 PDT (-0700), aou@eecs.berkeley.edu wrote: > On 2019-10-08 15:38:15 -0700, Palmer Dabbelt <palmer@sifive.com> wrote: >> On Fri, 20 Sep 2019 21:34:57 PDT (-0700), anup@brainfault.org wrote: >> > On Sat, Sep 21, 2019 at 6:30 AM Albert Ou <aou@eecs.berkeley.edu> wrote: >> >> >> >> This fixes an error with how the FDT blob is reserved in memblock. >> >> An incorrect physical address calculation exposed the FDT header to >> >> unintended corruption, which typically manifested with of_fdt_raw_init() >> >> faulting during late boot after fdt_totalsize() returned a wrong value. >> >> Systems with smaller physical memory sizes more frequently trigger this >> >> issue, as the kernel is more likely to allocate from the DMA32 zone >> >> where bbl places the DTB after the kernel image. >> >> >> >> Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") >> >> changed the mapping of the DTB to reside in the fixmap area. >> >> Consequently, early_init_fdt_reserve_self() cannot be used anymore in >> >> setup_bootmem() since it relies on __pa() to derive a physical address, >> >> which does not work with dtb_early_va that is no longer a valid kernel >> >> logical address. >> >> >> >> The reserved[0x1] region shows the effect of the pointer underflow >> >> resulting from the __pa(initial_boot_params) offset subtraction: >> >> >> >> [ 0.000000] MEMBLOCK configuration: >> >> [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 >> >> [ 0.000000] memory.cnt = 0x1 >> >> [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 >> >> [ 0.000000] reserved.cnt = 0x2 >> >> [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 >> >> [ 0.000000] reserved[0x1] [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0 >> >> >> >> With the fix applied: >> >> >> >> [ 0.000000] MEMBLOCK configuration: >> >> [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 >> >> [ 0.000000] memory.cnt = 0x1 >> >> [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 >> >> [ 0.000000] reserved.cnt = 0x2 >> >> [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 >> >> [ 0.000000] reserved[0x1] [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0 >> > >> > Thanks for catching this issue. >> > >> > Most of us did not notice this issue most likely because: >> > 1. We generally have good enough RAM on QEMU and SiFive Unleashed >> > 2. Most of people use OpenSBI FW_JUMP on QEMU and U-Boot on >> > SiFive Unleashed to boot in Linux which places FDT quite far away >> > from Linux kernel end >> > >> > Linux ARM64 kernel also uses FIXMAP to access FDT and over there >> > as well early_init_fdt_reserve_self() is not used. >> > >> >> >> >> Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") >> >> Signed-off-by: Albert Ou <aou@eecs.berkeley.edu> >> >> --- >> >> arch/riscv/mm/init.c | 13 ++++++++++++- >> >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c >> >> index f0ba713..52d007c 100644 >> >> --- a/arch/riscv/mm/init.c >> >> +++ b/arch/riscv/mm/init.c >> >> @@ -11,6 +11,7 @@ >> >> #include <linux/swap.h> >> >> #include <linux/sizes.h> >> >> #include <linux/of_fdt.h> >> >> +#include <linux/libfdt.h> >> >> >> >> #include <asm/fixmap.h> >> >> #include <asm/tlbflush.h> >> >> @@ -82,6 +83,8 @@ static void __init setup_initrd(void) >> >> } >> >> #endif /* CONFIG_BLK_DEV_INITRD */ >> >> >> >> +static phys_addr_t __dtb_pa __initdata; >> > >> > May be dtb_early_pa will be more consistent name >> > instead of __dtb_pa because it matches dtb_early_va >> > used below. >> > >> >> + >> >> void __init setup_bootmem(void) >> >> { >> >> struct memblock_region *reg; >> >> @@ -117,7 +120,12 @@ void __init setup_bootmem(void) >> >> setup_initrd(); >> >> #endif /* CONFIG_BLK_DEV_INITRD */ >> >> >> >> - early_init_fdt_reserve_self(); >> >> + /* >> >> + * Avoid using early_init_fdt_reserve_self() since __pa() does >> >> + * not work for DTB pointers that are fixmap addresses >> >> + */ >> >> + memblock_reserve(__dtb_pa, fdt_totalsize(dtb_early_va)); >> >> + >> >> early_init_fdt_scan_reserved_mem(); >> >> memblock_allow_resize(); >> >> memblock_dump_all(); >> >> @@ -333,6 +341,7 @@ static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size) >> >> "not use absolute addressing." >> >> #endif >> >> >> >> + >> > >> > Please remove this newline addition. >> > >> >> asmlinkage void __init setup_vm(uintptr_t dtb_pa) >> >> { >> >> uintptr_t va, end_va; >> >> @@ -393,6 +402,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa) >> >> >> >> /* Save pointer to DTB for early FDT parsing */ >> >> dtb_early_va = (void *)fix_to_virt(FIX_FDT) + (dtb_pa & ~PAGE_MASK); >> >> + /* Save physical address for memblock reservation */ >> >> + __dtb_pa = dtb_pa; >> >> } >> >> >> >> static void __init setup_vm_final(void) >> >> -- >> >> 2.7.4 >> >> >> >> >> >> _______________________________________________ >> >> linux-riscv mailing list >> >> linux-riscv@lists.infradead.org >> >> http://lists.infradead.org/mailman/listinfo/linux-riscv >> > >> > This deserves to be stable kernel fix as well. >> > You should add: >> > Cc: stable@vger.kernel.org >> > in your commit description. >> > >> > Apart from minor nits above. >> > >> > Reviewed-by: Anup Patel <anup@brainfault.org> >> > >> > I tried this patch for both RV64 and RV32 on QEMU with >> > Yocto rootfs. >> > >> > Tested-by: Anup Patel <anup@brainfault.org> >> > >> > Regards, >> > Anup >> >> Albert: Do you plan on spinning a v2 of the patch set? >> > > v2 was sent last week and has already been applied as > 922b0375fc93fb1a20c5617e37c389c26bbccb70 by Paul. Sorry about that.
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index f0ba713..52d007c 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -11,6 +11,7 @@ #include <linux/swap.h> #include <linux/sizes.h> #include <linux/of_fdt.h> +#include <linux/libfdt.h> #include <asm/fixmap.h> #include <asm/tlbflush.h> @@ -82,6 +83,8 @@ static void __init setup_initrd(void) } #endif /* CONFIG_BLK_DEV_INITRD */ +static phys_addr_t __dtb_pa __initdata; + void __init setup_bootmem(void) { struct memblock_region *reg; @@ -117,7 +120,12 @@ void __init setup_bootmem(void) setup_initrd(); #endif /* CONFIG_BLK_DEV_INITRD */ - early_init_fdt_reserve_self(); + /* + * Avoid using early_init_fdt_reserve_self() since __pa() does + * not work for DTB pointers that are fixmap addresses + */ + memblock_reserve(__dtb_pa, fdt_totalsize(dtb_early_va)); + early_init_fdt_scan_reserved_mem(); memblock_allow_resize(); memblock_dump_all(); @@ -333,6 +341,7 @@ static uintptr_t __init best_map_size(phys_addr_t base, phys_addr_t size) "not use absolute addressing." #endif + asmlinkage void __init setup_vm(uintptr_t dtb_pa) { uintptr_t va, end_va; @@ -393,6 +402,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa) /* Save pointer to DTB for early FDT parsing */ dtb_early_va = (void *)fix_to_virt(FIX_FDT) + (dtb_pa & ~PAGE_MASK); + /* Save physical address for memblock reservation */ + __dtb_pa = dtb_pa; } static void __init setup_vm_final(void)
This fixes an error with how the FDT blob is reserved in memblock. An incorrect physical address calculation exposed the FDT header to unintended corruption, which typically manifested with of_fdt_raw_init() faulting during late boot after fdt_totalsize() returned a wrong value. Systems with smaller physical memory sizes more frequently trigger this issue, as the kernel is more likely to allocate from the DMA32 zone where bbl places the DTB after the kernel image. Commit 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") changed the mapping of the DTB to reside in the fixmap area. Consequently, early_init_fdt_reserve_self() cannot be used anymore in setup_bootmem() since it relies on __pa() to derive a physical address, which does not work with dtb_early_va that is no longer a valid kernel logical address. The reserved[0x1] region shows the effect of the pointer underflow resulting from the __pa(initial_boot_params) offset subtraction: [ 0.000000] MEMBLOCK configuration: [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 [ 0.000000] memory.cnt = 0x1 [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 [ 0.000000] reserved.cnt = 0x2 [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 [ 0.000000] reserved[0x1] [0xfffffff080100000-0xfffffff080100527], 0x0000000000000528 bytes flags: 0x0 With the fix applied: [ 0.000000] MEMBLOCK configuration: [ 0.000000] memory size = 0x000000001fe00000 reserved size = 0x0000000000a2e514 [ 0.000000] memory.cnt = 0x1 [ 0.000000] memory[0x0] [0x0000000080200000-0x000000009fffffff], 0x000000001fe00000 bytes flags: 0x0 [ 0.000000] reserved.cnt = 0x2 [ 0.000000] reserved[0x0] [0x0000000080200000-0x0000000080c2dfeb], 0x0000000000a2dfec bytes flags: 0x0 [ 0.000000] reserved[0x1] [0x0000000080e00000-0x0000000080e00527], 0x0000000000000528 bytes flags: 0x0 Fixes: 671f9a3e2e24 ("RISC-V: Setup initial page tables in two stages") Signed-off-by: Albert Ou <aou@eecs.berkeley.edu> --- arch/riscv/mm/init.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)