Re: [PATCH] arm64: crypto: Don't allow v8.2 extensions to be used with BROKEN_GAS_INST

From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>, Ard Biesheuvel <ardb@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	"David S . Miller" <davem@davemloft.net>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] arm64: crypto: Don't allow v8.2 extensions to be used with BROKEN_GAS_INST
Date: Thu, 03 Mar 2022 11:16:28 +0000	[thread overview]
Message-ID: <87y21r1e2b.wl-maz@kernel.org> (raw)
In-Reply-To: <CAMj1kXHhTNsB8V=4LNMqSXrAWXroHHNXzjS0xWERboG7a3G-Lg@mail.gmail.com>

On Thu, 03 Mar 2022 07:26:45 +0000,
Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Wed, 2 Mar 2022 at 16:54, Mark Brown <broonie@kernel.org> wrote:
> >
> > We support building the kernel with archaic versions of binutils which
> > had some confusion regarding how instructions should be encoded for .inst
> > which we work around with the __emit_inst() macro. Unfortunately we have
> > not consistently used this macro, one of the places where it's missed being
> > the macros that manually encode v8.2 crypto instructions. This means that
> > kernels built with such toolchains have never supported use of the affected
> > instructions correctly.
> >
> > Since these toolchains are very old (some idle research suggested 2015
> > era) it seems more sensible to just refuse to build v8.2 crypto support
> > with them, in the unlikely event that someone has a need to use such a
> > toolchain to build a kernel which will run on a system with v8.2 crypto
> > support they can always fix this properly but it seems more likely that
> > we will deprecate support for these toolchains and remove __emit_inst()
> > before that happens.
> >
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> 
> IIRC this is not about .inst getting the encoding wrong, but about
> confusion over the size of the generated opcode, resulting in problems
> generating constants involving relative offsets between labels. (The
> endian swap is there so that .long can be used on BE to emit the LE
> opcodes)
>
> This is not an issue here, so I don't think this change is necessary.

Indeed. The only case where the broken GAS .inst has hit us was in
combination with alternatives (see eb7c11ee3c5c for details). The
encoding itself is always correct, and it is only the label generation
that was broken. If we were affected by this, the kernel would simply
fail to build with these toolchains.

If this ever happens (because we'd add some extra alternative
sequences to the crypto code?), we can revisit this. But in the
meantime, I don't see anything warranting this extra dependency.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.