Re: [PATCH] MIPS: kernel: Reserve exception base early to prevent corruption

[Florian Fainelli]

On 3/3/21 1:14 PM, Serge Semin wrote:
> Hello  Thomas,
> Thanks for the patch. My comments are below.
> 
> On Wed, Mar 03, 2021 at 07:57:13PM +0100, Thomas Bogendoerfer wrote:
>> BMIPS is one of the few platforms that do change the exception base.
>> After commit 2dcb39645441 ("memblock: do not start bottom-up allocations
>> with kernel_end") we started seeing BMIPS boards fail to boot with the
>> built-in FDT being corrupted.
>>
>> Before the cited commit, early allocations would be in the [kernel_end,
>> RAM_END] range, but after commit they would be within [RAM_START +
>> PAGE_SIZE, RAM_END].
>>
>> The custom exception base handler that is installed by
>> bmips_ebase_setup() done for BMIPS5000 CPUs ends-up trampling on the
>> memory region allocated by unflatten_and_copy_device_tree() thus
>> corrupting the FDT used by the kernel.
>>
>> To fix this, we need to perform an early reservation of the custom
>> exception space. So we reserve it already in cpu_probe() for the CPUs
>> where this is fixed. For CPU with an ebase config register allocation
>> of exception space will be done in trap_init().
>>
>> Huge thanks to Serget for analysing and proposing a solution to this
>> issue.
>>
> 
>> Fixes: Fixes: 2dcb39645441 ("memblock: do not start bottom-up allocations with kernel_end")
> 
> Fixes tag is used twice.
> 
>> Debugged-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
>> Reported-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
> 
> I'd switch these tags order. First it was reported, then the
> problem was debugged. I suppose it would be also nice to add
> Florian under the second Reported-by tag if he doesn't mind. I haven't
> seen any Kamal' email message, but a report posted by Florian only.

Kamal reported it to me privately and then I brought it publicly, still
wanted to give him credit.

> 
>> Signed-off-by: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
>> ---
>>  arch/mips/include/asm/traps.h    |  4 ++++
>>  arch/mips/kernel/cpu-probe.c     |  7 +++++++
>>  arch/mips/kernel/cpu-r3k-probe.c |  3 +++
>>  arch/mips/kernel/smp-bmips.c     |  9 +--------
>>  arch/mips/kernel/traps.c         | 31 ++++++++++++++++---------------
>>  5 files changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/traps.h b/arch/mips/include/asm/traps.h
>> index 6aa8f126a43d..d74d829e1655 100644
>> --- a/arch/mips/include/asm/traps.h
>> +++ b/arch/mips/include/asm/traps.h
>> @@ -24,7 +24,11 @@ extern void (*board_ebase_setup)(void);
>>  extern void (*board_cache_error_setup)(void);
>>  
>>  extern int register_nmi_notifier(struct notifier_block *nb);
>> +extern void reserve_exception_space(unsigned long addr, unsigned long size);
>>  extern char except_vec_nmi[];
>> +extern unsigned long ebase_size;
>> +
>> +#define VECTORSPACING 0x100	/* for EI/VI mode */
>>  
>>  #define nmi_notifier(fn, pri)						\
>>  ({									\
>> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
>> index 9a89637b4ecf..effc45cbb351 100644
>> --- a/arch/mips/kernel/cpu-probe.c
>> +++ b/arch/mips/kernel/cpu-probe.c
>> @@ -26,6 +26,7 @@
>>  #include <asm/elf.h>
>>  #include <asm/pgtable-bits.h>
>>  #include <asm/spram.h>
>> +#include <asm/traps.h>
>>  #include <linux/uaccess.h>
>>  
>>  #include "fpu-probe.h"
>> @@ -1628,6 +1629,7 @@ static inline void cpu_probe_broadcom(struct cpuinfo_mips *c, unsigned int cpu)
>>  		c->cputype = CPU_BMIPS3300;
>>  		__cpu_name[cpu] = "Broadcom BMIPS3300";
>>  		set_elf_platform(cpu, "bmips3300");
>> +		reserve_exception_space(0x80000400, VECTORSPACING * 64);
>>  		break;
>>  	case PRID_IMP_BMIPS43XX: {
>>  		int rev = c->processor_id & PRID_REV_MASK;
>> @@ -1638,6 +1640,7 @@ static inline void cpu_probe_broadcom(struct cpuinfo_mips *c, unsigned int cpu)
>>  			__cpu_name[cpu] = "Broadcom BMIPS4380";
>>  			set_elf_platform(cpu, "bmips4380");
>>  			c->options |= MIPS_CPU_RIXI;
>> +			reserve_exception_space(0x80000400, VECTORSPACING * 64);
>>  		} else {
>>  			c->cputype = CPU_BMIPS4350;
>>  			__cpu_name[cpu] = "Broadcom BMIPS4350";
>> @@ -1654,6 +1657,7 @@ static inline void cpu_probe_broadcom(struct cpuinfo_mips *c, unsigned int cpu)
>>  			__cpu_name[cpu] = "Broadcom BMIPS5000";
>>  		set_elf_platform(cpu, "bmips5000");
>>  		c->options |= MIPS_CPU_ULRI | MIPS_CPU_RIXI;
>> +		reserve_exception_space(0x80001000, VECTORSPACING * 64);
>>  		break;
>>  	}
>>  }
>> @@ -2133,6 +2137,9 @@ void cpu_probe(void)
>>  	if (cpu == 0)
>>  		__ua_limit = ~((1ull << cpu_vmbits) - 1);
>>  #endif
>> +
>> +	if (ebase_size == 0 && !cpu_has_mips_r2_r6)
>> +		reserve_exception_space(CAC_BASE, 0x400);
>>  }
>>  
>>  void cpu_report(void)
>> diff --git a/arch/mips/kernel/cpu-r3k-probe.c b/arch/mips/kernel/cpu-r3k-probe.c
>> index abdbbe8c5a43..6e3f4c17b810 100644
>> --- a/arch/mips/kernel/cpu-r3k-probe.c
>> +++ b/arch/mips/kernel/cpu-r3k-probe.c
>> @@ -21,6 +21,7 @@
>>  #include <asm/fpu.h>
>>  #include <asm/mipsregs.h>
>>  #include <asm/elf.h>
>> +#include <asm/traps.h>
>>  
>>  #include "fpu-probe.h"
>>  
>> @@ -158,6 +159,8 @@ void cpu_probe(void)
>>  		cpu_set_fpu_opts(c);
>>  	else
>>  		cpu_set_nofpu_opts(c);
>> +
>> +	reserve_exception_space(CAC_BASE, 0x400);
>>  }
>>  
>>  void cpu_report(void)
>> diff --git a/arch/mips/kernel/smp-bmips.c b/arch/mips/kernel/smp-bmips.c
>> index b6ef5f7312cf..ad3f2282a65a 100644
>> --- a/arch/mips/kernel/smp-bmips.c
>> +++ b/arch/mips/kernel/smp-bmips.c
>> @@ -528,10 +528,6 @@ static void bmips_set_reset_vec(int cpu, u32 val)
>>  
>>  void bmips_ebase_setup(void)
>>  {
>> -	unsigned long new_ebase = ebase;
>> -
>> -	BUG_ON(ebase != CKSEG0);
>> -
>>  	switch (current_cpu_type()) {
>>  	case CPU_BMIPS4350:
>>  		/*
> 
>> @@ -554,7 +550,6 @@ void bmips_ebase_setup(void)
>>  		 * 0x8000_0000: reset/NMI (initially in kseg1)
>>  		 * 0x8000_0400: normal vectors
>>  		 */
>> -		new_ebase = 0x80000400;
>>  		bmips_set_reset_vec(0, RESET_FROM_KSEG0);
>>  		break;
>>  	case CPU_BMIPS5000:
>> @@ -562,16 +557,14 @@ void bmips_ebase_setup(void)
>>  		 * 0x8000_0000: reset/NMI (initially in kseg1)
>>  		 * 0x8000_1000: normal vectors
>>  		 */
>> -		new_ebase = 0x80001000;
>>  		bmips_set_reset_vec(0, RESET_FROM_KSEG0);
>> -		write_c0_ebase(new_ebase);
>> +		write_c0_ebase(ebase);
>>  		break;
>>  	default:
>>  		return;
>>  	}
>>  
>>  	board_nmi_handler_setup = &bmips_nmi_handler_setup;
> 
> I've just realized that Broadcom MIPS actually needs to reserve a
> space above 0x80000000 too. See the in-situ comment here, 0x8000_0000
> is said to be a space for reset/NMI. That space is then rewritten by
> the method bmips_nmi_handler_setup() called in trap_init(). Of course
> memblock allocates a memory starting from PAGE_SIZE so we are on safe
> side at boot-stage. At the same time memblock doesn't mark the lowest
> region as reserved. Thus we can't be sure that the buddy allocator
> won't ever try to use that physical memory. AFAICS bcm63xx
> also copies some vector to 0xa0000200.

Yes, good point, I missed that part as well.

> 
> Similar thing concerns all the platforms, which initialize the
> pointers: board_nmi_handler_setup and board_ejtag_handler_setup. All
> of them rewrite some lowest memory space with NMI/eJTAG vectors and
> most likely expects that data being left unchanged too.
> 
> I am a bit surprised we haven't got any bug report in that matter so
> far, because AFAIR the MIPS arch's stopped reserving memory below the
> kernel since bootmem allocator was removed. Anyway at least for the
> sake of consistency the lowest page should be reserved in the affected
> platforms.

My guess is that MIPS is just becoming less and less used so all of
these issues tend to be overlooked and/or harder to discover.
-- 
Florian