Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 5 May 2020 20:45:23 -0700
From:   Nathan Chancellor <natechancellor@gmail.com>
To:     Torsten Duwe <duwe@lst.de>
Cc:     Mark Rutland <mark.rutland@arm.com>, Arnd Bergmann <arnd@arndb.de>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Amit Daniel Kachhap <amit.kachhap@arm.com>,
        Torsten Duwe <duwe@suse.de>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        AKASHI Takahiro <takahiro.akashi@linaro.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Julien Thierry <jthierry@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Marc Zyngier <maz@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Alexandre Ghiti <alex@ghiti.fr>,
        Kristina Martsenko <kristina.martsenko@arm.com>,
        Ionela Voinescu <ionela.voinescu@arm.com>,
        Steve Capper <steve.capper@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com,
        Fangrui Song <maskray@google.com>
Subject: Re: [PATCH] arm64: disable patchable function entry on big-endian
 clang builds
Message-ID: <20200506034523.GA564255@ubuntu-s3-xlarge-x86>
References: <20200505141257.707945-1-arnd@arndb.de>
 <20200505142556.GF82823@C02TD0UTHF1T.local>
 <20200505194243.5bfc6ec6@blackhole>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200505194243.5bfc6ec6@blackhole>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+ Fangrui, who implemented patchable_function_entry in LLVM/clang

On Tue, May 05, 2020 at 07:42:43PM +0200, Torsten Duwe wrote:
> Hi Arnd, Mark and others,
> 
> this may not be worth arguing but I'm currently fighting excessive
> workarounds in another area and so this triggers me, so I have to make
> a remark ;-)
> 
> On Tue, 5 May 2020 15:25:56 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
> > > Clang only supports the patchable_function_entry attribute on
> > > little-endian arm64 builds, but not on big-endian:
> > > 
> > > include/linux/kasan-checks.h:16:8: error: unknown attribute
> > > 'patchable_function_entry' ignored [-Werror,-Wunknown-attributes]
> > > 
> > > Disable that configuration with another dependency. Unfortunately
> > > the existing check is not enough, as $(cc-option) at this point does
> > > not pass the -mbig-endian flag.
> > 
> > Well that's unfortunate. :(
> > 
> > Do we know if this is deliberate and/or likely to change in future?

I am not sure this is deliberate, I don't see anything about endianness
in the commits that added this:

https://github.com/llvm/llvm-project/commit/4d1e23e3b3cd7c72a8b24dc5acb7e13c58a8de37
https://github.com/llvm/llvm-project/commit/22467e259507f5ead2a87d989251b4c951a587e4
https://github.com/llvm/llvm-project/commit/06b8e32d4fd3f634f793e3bc0bc4fdb885e7a3ac

> > This practically rules out a BE distro kernel with things like PAC,
> > which isn't ideal.

To be fair, are there big endian AArch64 distros?

https://wiki.debian.org/Arm64Port: "There is also a big-endian version
of the architecture/ABI: aarch64_be-linux-gnu but we're not supporting
that in Debian (so there is no corresponding Debian architecture name)
and hopefully will never have to. Nevertheless you might want to check
for this by way of completeness in upstream code."

OpenSUSE and Fedora don't appear to have support for big endian.

> But still better than cumulating workarounds. If clang's flags aren't
> orthogonal then that's a bug in clang. If I get a vote here I'm against
> it.
> 
> > > Fixes: 3b23e4991fb6 ("arm64: implement ftrace with regs")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > This looks fine for now, and we can add a version check in future, so:
>                                       ^^^^^^^^^^^^^^^^^^^
> see what I mean? And in the end another line of code you'll never again
> get rid of.

That's a rather pessimistic attitude to have. We've been rather good
about trying to fix stuff in the compiler rather than hacking up the
kernel.

> I suggest to get a quote from clang folks first about their schedule and
> regarded importance of patchable-function-entries on BE, and leave it as
> is: broken on certain clang configurations. It's not the kernel's fault.

We can file an upstream PR (https://bugs.llvm.org) to talk about this
(although I've CC'd Fangrui) but you would rather the kernel fail to
work properly than prevent the user from being able to select that
option? Why even have the "select" or "depends on" keyword then?

That said, I do think we should hold off on this patch until we hear
from the LLVM developers.

> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > Mark.
> > 
> > > ---
> > >  arch/arm64/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > index 4b256fa6db7a..a33d6402b934 100644
> > > --- a/arch/arm64/Kconfig
> > > +++ b/arch/arm64/Kconfig
> > > @@ -151,7 +151,7 @@ config ARM64
> > >  	select HAVE_DMA_CONTIGUOUS
> > >  	select HAVE_DYNAMIC_FTRACE
> > >  	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> > > -		if $(cc-option,-fies on y=2)
> > > +		if $(cc-option,-fpatchable-function-entry=2) &&
> > > !(CC_IS_CLANG && CPU_BIG_ENDIAN) select
> > > HAVE_EFFICIENT_UNALIGNED_ACCESS select HAVE_FAST_GUP
> > >  	select HAVE_FTRACE_MCOUNT_RECORD
> > > -- 
> > > 2.26.0
> > > 
> 

Cheers,
Nathan