Re: [RFC PATCH] ARM: enable irq in translation/section permission fault handlers

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2019-02-15 21:57:56 [+0000], Bernd Edlinger wrote:
> > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> > index 58f69fa07df95..da82967865836 100644
> > --- a/arch/arm/mm/fault.c
> > +++ b/arch/arm/mm/fault.c
> > @@ -161,8 +161,6 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
> >  		unsigned int fsr, unsigned int sig, int code,
> >  		struct pt_regs *regs)
> >  {
> > -	if (addr > TASK_SIZE)
> > -		harden_branch_predictor();
> >  
> >  #ifdef CONFIG_DEBUG_USER
> >  	if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) ||
> > @@ -191,6 +189,11 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> >  	struct task_struct *tsk = current;
> >  	struct mm_struct *mm = tsk->active_mm;
> >  
> > +	if (addr > TASK_SIZE && user_mode(regs))
> > +		harden_branch_predictor();
> 
> This is somehow inconsisten with do_translation_fault, where
> we have this:
> 
> 	if (addr < TASK_SIZE)
> 		return do_page_fault(addr, fsr, regs);

yes but harden_branch_predictor() is only invoked for addr > TASK_SIZE.
What do I miss?

> > +
> > +	if (interrupts_enabled(regs))
> > +		local_irq_enable();
> >  	/*
> >  	 * If we are in kernel mode at this point, we
> >  	 * have no context to handle this fault with.
> > 
> 
> I have seen three different failure modes, pleas see the first 3 calls stacks
> here: https://marc.info/?l=linux-rt-users&m=155016888714927&w=2

yes, but this is only with the one patch in RT. So you should not see
this without the RT patch.

> I am concerned about this fist issue, because it removes the branch
> predictor hardening after the do_page_fault has executed:
> 
> do_DataAbort->do_page_fault(addr>TASK_SIZE)->__do_user_fault
>
> This is reachable because do_page_fault is not only called from
> do_translation_fault but also from here: arch/arm/mm/fsr-2level.c
> and here: arch/arm/mm/fsr-3level.c
> those are callable with addr > TASK_SIZE

okay. So 0xbffffff0 without LPAE would be left out. I wasn't ware of
that. And this indeed it hits the warning.

> And the following code path does enable the hard irqs before do_bad_area:
> do_DataAbort->do_sect_fault->do_bad_area->__do_user_fault
> 
> So this function, would need to be rewritten:
> 
> do_sect_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> {
> 	if (interrupts_enabled(regs))
> 		local_irq_enable();
> 
> 	do_bad_area(addr, fsr, regs);
> 	return 0;
> }

We would need to move the branch predictor piece before enabling
interrupts.

> 
> Thanks
> Bernd.

Sebastian

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