[PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0

[Pingfan Liu]

On Wed, Dec 22, 2021 at 12:59:32PM +0000, Catalin Marinas wrote:
> On Tue, Dec 21, 2021 at 09:42:41PM +0800, Pingfan Liu wrote:
> > If using software PAN, the ttbr0 should keep unchanged, otherwise,
> > considering the following scenario:
> >   task1
> >     __uaccess_ttbr0_enable()
> >     switch_mm(this,next,tsk), which resets ttbr0 to __pa_symbol(reserved_pg_dir)
> >     switch_mm(prev,this,tsk), which can not re-install the user page table automatically
> 
> Have you found a real problem with this in practice or just by code
> inspection?
> 
By code inspection.

> The assumption is that during uaccess_ttbr0_enable/disable regions, the
> only way to get into switch_mm() is as a result of a page fault or
> interrupt. The __swpan_{entry,exit}_el1 functions should take care of
> restoring ttbr0 when returning to the interrupted context.
> 
Yes, I read the code careless and miss it. I am chewing it.

Thank you very much for pointing me it.

> > Tackle this issue by disabling preemption.
> > 
> > Signed-off-by: Pingfan Liu <kernelfans at gmail.com>
> > Cc: Catalin Marinas <catalin.marinas at arm.com>
> > Cc: Will Deacon <will at kernel.org>
> > Cc: Mark Rutland <mark.rutland at arm.com>
> > Cc: Vincenzo Frascino <vincenzo.frascino at arm.com>
> > Cc: Andrey Konovalov <andreyknvl at gmail.com>
> > Cc: Arnd Bergmann <arnd at arndb.de>
> > To: linux-arm-kernel at lists.infradead.org
> > ---
> >  arch/arm64/include/asm/uaccess.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index 3a5ff5e20586..406888877bbd 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -121,6 +121,7 @@ static inline bool uaccess_ttbr0_disable(void)
> >  	if (!system_uses_ttbr0_pan())
> >  		return false;
> >  	__uaccess_ttbr0_disable();
> > +	preempt_enable();
> >  	return true;
> >  }
> >  
> > @@ -128,6 +129,7 @@ static inline bool uaccess_ttbr0_enable(void)
> >  {
> >  	if (!system_uses_ttbr0_pan())
> >  		return false;
> > +	preempt_disable();
> >  	__uaccess_ttbr0_enable();
> >  	return true;
> >  }
> 
> preempt_disable() won't help much here if, for example, the subsequent
> uaccess gets a fault and need to sleep until the accessed page gets
> available. I suspect you'd get some sleeping in atomic warning as well
> with the right debug options enabled.
> 

Yes, the user page may be swapped out.


Thank you for the guide.

Regards,

	Pingfan

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