Re: [PATCH v3 4/4] ARM: Implement PAN for LPAE by TTBR0 page table walks disablement

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Arnd Bergmann <arnd@arndb.de>, Stefan Wahren <wahrenst@gmx.net>,
	Kees Cook <keescook@chromium.org>,
	linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH v3 4/4] ARM: Implement PAN for LPAE by TTBR0 page table walks disablement
Date: Wed, 15 May 2024 09:48:38 +0100	[thread overview]
Message-ID: <ZkR25mhjZ0z5XsmM@shell.armlinux.org.uk> (raw)
In-Reply-To: <CAMj1kXE34s2pSBT8q1G-xrMVCuvDbdKFwhM2bW60cA0_nX_GzQ@mail.gmail.com>

On Wed, May 15, 2024 at 10:36:48AM +0200, Ard Biesheuvel wrote:
> On Tue, 14 May 2024 at 22:34, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > On 5/14/24 13:33, Linus Walleij wrote:
> > > On Tue, May 14, 2024 at 8:26 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > >> On 5/14/24 10:03, Russell King (Oracle) wrote:
> > >
> > >>> I would imagine that the problem is cpu_set_ttbcr(). Please try adding
> > >>> a "memory" clobber to the asm() instruction in there.
> > >>>
> > >>
> > >> I can confirm that with CONFIG_CC_OPTIMIZE_FOR_SIZE=y and the hunk below:
> > >>
> > >> diff --git a/arch/arm/include/asm/proc-fns.h
> > >> b/arch/arm/include/asm/proc-fns.h
> > >> index 9b3105a2a5e0..1087bd2af433 100644
> > >> --- a/arch/arm/include/asm/proc-fns.h
> > >> +++ b/arch/arm/include/asm/proc-fns.h
> > >> @@ -187,7 +187,7 @@ static inline unsigned int cpu_get_ttbcr(void)
> > >>
> > >>    static inline void cpu_set_ttbcr(unsigned int ttbcr)
> > >>    {
> > >> -       asm("mcr p15, 0, %0, c2, c0, 2" : : "r" (ttbcr));
> > >> +       asm("mcr p15, 0, %0, c2, c0, 2" : : "r" (ttbcr) : "memory");
> > >>    }
> > >>
> > >>    #else  /*!CONFIG_MMU */
> > >>
> > >> my Raspberry Pi 4B in AArch32 mode boots and runs user-space properly.
> > >>
> > >> Thanks a lot Russell!
> > >
> > > Second that, very nicely pinpointed Russell!
> > >
> > > Florian, do you want to send a patch or should I?
> >
> > I was wondering if Russell was able to fold this directly into patch #2
> > where cpu_set_ttbr() is added, so as to not break functionality across
> > bisection.
> 
> Sadly, I can still reproduce this with the above fix.
> 
> I included TTBCR in the DEBUG_USER output, and (as expected), it has
> A1, EPD0 and T0SZ set to the 'uaccess disabled' values.
> 
> Using __always_inline on uaccess_save_and_enable() and
> uaccess_restore() (as the CONFIG_CPU_SW_DOMAIN_PAN does) seems to work
> around it. The "memory" clobber seems unnecessary in my case, but it
> is needed for correctness in any case.
> 
> It is unclear to me why placing these helpers out of line should make
> any difference, and I am not convinced it is a problem in the code
> (IIRC we've had other issues with -Os in the past)

Time to start comparing compilers / compiler versions?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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