Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d63250048e4b224973b5a8d50e4c92547d4a9c34.camel@linux.ibm.com>
Subject: Re: [PATCH] mm: kfence: Fix false positives on big endian
From:   Benjamin Gray <bgray@linux.ibm.com>
To:     Michael Ellerman <mpe@ellerman.id.au>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Laight <David.Laight@ACULAB.COM>
Cc:     "zhangpeng.00@bytedance.com" <zhangpeng.00@bytedance.com>,
        "elver@google.com" <elver@google.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "glider@google.com" <glider@google.com>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Date:   Fri, 19 May 2023 16:29:43 +1000
In-Reply-To: <87o7mgzyw1.fsf@mail.lhotse>
References: <20230505035127.195387-1-mpe@ellerman.id.au>
         <826f836f41db41eeb0fc32061994ac39@AcuMS.aculab.com>
         <20230517152028.86b6d2d5afa4541b4269131b@linux-foundation.org>
         <87o7mgzyw1.fsf@mail.lhotse>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-05-19 at 15:14 +1000, Michael Ellerman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> > On Fri, 5 May 2023 16:02:17 +0000 David Laight
> > <David.Laight@ACULAB.COM> wrote:
> >=20
> > > From: Michael Ellerman
> > > > Sent: 05 May 2023 04:51
> > > >=20
> > > > Since commit 1ba3cbf3ec3b ("mm: kfence: improve the performance
> > > > of
> > > > __kfence_alloc() and __kfence_free()"), kfence reports failures
> > > > in
> > > > random places at boot on big endian machines.
> > > >=20
> > > > The problem is that the new KFENCE_CANARY_PATTERN_U64 encodes
> > > > the
> > > > address of each byte in its value, so it needs to be byte
> > > > swapped on big
> > > > endian machines.
> > > >=20
> > > > The compiler is smart enough to do the le64_to_cpu() at compile
> > > > time, so
> > > > there is no runtime overhead.
> > > >=20
> > > > Fixes: 1ba3cbf3ec3b ("mm: kfence: improve the performance of
> > > > __kfence_alloc() and __kfence_free()")
> > > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > > > ---
> > > > =C2=A0mm/kfence/kfence.h | 2 +-
> > > > =C2=A01 file changed, 1 insertion(+), 1 deletion(-)
> > > >=20
> > > > diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
> > > > index 2aafc46a4aaf..392fb273e7bd 100644
> > > > --- a/mm/kfence/kfence.h
> > > > +++ b/mm/kfence/kfence.h
> > > > @@ -29,7 +29,7 @@
> > > > =C2=A0 * canary of every 8 bytes is the same. 64-bit memory can be
> > > > filled and checked
> > > > =C2=A0 * at a time instead of byte by byte to improve performance.
> > > > =C2=A0 */
> > > > -#define KFENCE_CANARY_PATTERN_U64 ((u64)0xaaaaaaaaaaaaaaaa ^
> > > > (u64)(0x0706050403020100))
> > > > +#define KFENCE_CANARY_PATTERN_U64 ((u64)0xaaaaaaaaaaaaaaaa ^
> > > > (u64)(le64_to_cpu(0x0706050403020100)))
> > >=20
> > > What at the (u64) casts for?
> > > The constants should probably have a ul (or ull) suffix.
> > >=20
> >=20
> > I tried that, didn't fix the sparse warnings described at
> > https://lkml.kernel.org/r/202305132244.DwzBUcUd-lkp@intel.com.
> >=20
> > Michael, have you looked into this?
>=20
> I haven't sorry, been chasing other bugs.
>=20
> > I'll merge it upstream - I guess we can live with the warnings for
> > a while.
>=20
> Thanks, yeah spurious WARNs are more of a pain than some sparse
> warnings.
>=20
> Maybe using le64_to_cpu() is too fancy, could just do it with an
> ifdef? eg.
>=20
> diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
> index 392fb273e7bd..510355a5382b 100644
> --- a/mm/kfence/kfence.h
> +++ b/mm/kfence/kfence.h
> @@ -29,7 +29,11 @@
> =C2=A0 * canary of every 8 bytes is the same. 64-bit memory can be filled
> and checked
> =C2=A0 * at a time instead of byte by byte to improve performance.
> =C2=A0 */
> -#define KFENCE_CANARY_PATTERN_U64 ((u64)0xaaaaaaaaaaaaaaaa ^
> (u64)(le64_to_cpu(0x0706050403020100)))
> +#ifdef __LITTLE_ENDIAN__
> +#define KFENCE_CANARY_PATTERN_U64 (0xaaaaaaaaaaaaaaaaULL ^
> 0x0706050403020100ULL)
> +#else
> +#define KFENCE_CANARY_PATTERN_U64 (0xaaaaaaaaaaaaaaaaULL ^
> 0x0001020304050607ULL)
> +#endif
> =C2=A0
> =C2=A0/* Maximum stack depth for reports. */
> =C2=A0#define KFENCE_STACK_DEPTH 64
>=20
>=20
> cheers

(for the sparse errors)

As I understand, we require memory to look like "00 01 02 03 04 05 06
07" such that iterating byte-by-byte gives 00, 01, etc. (with
everything XORed with aaa...)

I think it would be most semantically correct to use cpu_to_le64 on
KFENCE_CANARY_PATTERN_U64 and annotate the values being compared
against it as __le64.=C2=A0This is because we want the integer literal
0x0706050403020100 to be stored as "00 01 02 03 04 05 06 07", which is
the definition of little endian.

Masking this with an #ifdef leaves the type as cpu endian, which could
result in future issues.

(or I've just misunderstood and can disregard this)