Re: [PATCH v2 3/4] ARM: Reduce the number of #ifdef CONFIG_CPU_SW_DOMAIN_PAN

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Ard Biesheuvel <ardb@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Stefan Wahren <wahrenst@gmx.net>,
	Kees Cook <keescook@chromium.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH v2 3/4] ARM: Reduce the number of #ifdef CONFIG_CPU_SW_DOMAIN_PAN
Date: Tue, 12 Mar 2024 10:37:07 +0000	[thread overview]
Message-ID: <ZfAwU1HepYp4UApd@shell.armlinux.org.uk> (raw)
In-Reply-To: <CACRpkdZvwRpZLCOe69CyweRzkLWcMo=9tkvp2ag7aF8Gnht5xQ@mail.gmail.com>

On Tue, Mar 12, 2024 at 09:30:37AM +0100, Linus Walleij wrote:
> On Tue, Mar 12, 2024 at 9:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Mar 11, 2024 at 5:02 PM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > > On Wed, Feb 21, 2024 at 12:04:02AM +0100, Linus Walleij wrote:
> > > > @@ -24,9 +24,10 @@
> > > >   * perform such accesses (eg, via list poison values) which could then
> > > >   * be exploited for priviledge escalation.
> > > >   */
> > > > +#if defined(CONFIG_CPU_SW_DOMAIN_PAN)
> > > > +
> > > >  static __always_inline unsigned int uaccess_save_and_enable(void)
> > > >  {
> > > > -#ifdef CONFIG_CPU_SW_DOMAIN_PAN
> > >
> > > This is an interesting way to reduce the #ifdef count... why switch it
> > > from #ifdef to #if defined ?
> >
> > Hm that looks like a development artifact from a point where we
> > has if defined(A) && defined(B) and then one of them was removed.
> 
> Ah no, now I see it. In the next patch a pattern with
> 
> #elif defined(CONFIG_CPU_TTBR0_PAN)
> 
> is used, so in order to not look confusing the #elif defined() is paired
> with #if defined().

Looking closer, there is inconsistency in patch 2 and patch 3.

In patch 2, uaccess-asm.h, you move the #ifdef without changing it, and
in the other files it becomes #if defined.

In patch 3, you add #elif defined to all these files. So in
uaccess-asm.h, we end up with:

#ifdef
#elif defined

whereas others we have:

#if defined
#elif defined

So I think uaccess-asm.h needs to be changed as well. It probably would
make more sense in the patches if patch 2 if the #ifdef's were moved
without changing them, and in patch 3 which introduces the #elif
defined, to also then change the #ifdef to #if defined. Then the
commit messages don't need modification because it's obvious why the
change if #ifdef is happening.

-- 
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