| Message ID | 20211221134242.98877-1-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 | expand |
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? 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. > Tackle this issue by disabling preemption. > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> > Cc: Andrey Konovalov <andreyknvl@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > To: linux-arm-kernel@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.
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@gmail.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> > > Cc: Andrey Konovalov <andreyknvl@gmail.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > To: linux-arm-kernel@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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; }
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 Tackle this issue by disabling preemption. Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> Cc: Andrey Konovalov <andreyknvl@gmail.com> Cc: Arnd Bergmann <arnd@arndb.de> To: linux-arm-kernel@lists.infradead.org --- arch/arm64/include/asm/uaccess.h | 2 ++ 1 file changed, 2 insertions(+)