Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Russell King - ARM Linux admin]

On Wed, Jan 06, 2021 at 10:32:23PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 06, 2021 at 05:20:34PM +0000, Will Deacon wrote:
> > With that, I see the following after ten seconds or so:
> > 
> >   EXT4-fs error (device sda2): ext4_lookup:1707: inode #674497: comm md5sum: iget: checksum invalid
> > 
> > Russell, Mark -- does this recipe explode reliably for you too?
> 
> I've been working this evening on tracking down what change in the
> Kconfig file between your working 5.10 kernel binary you supplied me,
> and my failing 5.9 kernel.
> 
> I've found that _enabling_ CONFIG_STACKPROTECTOR appears to mask the
> inode checksum failure problem, at least from a short test.) I'm going
> to re-enable CONFIG_STACKPROTECTOR and leave it running for longer.
> 
> That is:
> 
> CONFIG_STACKPROTECTOR=y
> CONFIG_STACKPROTECTOR_STRONG=y
> 
> appears to mask the problem
> 
> # CONFIG_STACKPROTECTOR is not set
> 
> appears to unmask the problem.

We have finally got to the bottom of this - the "bug" is in the ext4
code:

static inline u32 ext4_chksum(struct ext4_sb_info *sbi, u32 crc,
                              const void *address, unsigned int length)
{
        struct {
                struct shash_desc shash;
                char ctx[4];
        } desc;

        BUG_ON(crypto_shash_descsize(sbi->s_chksum_driver)!=sizeof(desc.ctx));

        desc.shash.tfm = sbi->s_chksum_driver;
        *(u32 *)desc.ctx = crc;

        BUG_ON(crypto_shash_update(&desc.shash, address, length));

        return *(u32 *)desc.ctx;
}

This isn't always inlined, despite the "inline" keyword. With GCC
4.9.4, this is compiled to the following code when the stack protector
is disabled:

0000000000000004 <ext4_chksum.isra.14.constprop.19>:
   4:   a9be7bfd        stp     x29, x30, [sp, #-32]!		<------
   8:   2a0103e3        mov     w3, w1
   c:   aa0203e1        mov     x1, x2
  10:   910003fd        mov     x29, sp				<------
  14:   f9000bf3        str     x19, [sp, #16]
  18:   d10603ff        sub     sp, sp, #0x180			<------
  1c:   9101fff3        add     x19, sp, #0x7f
  20:   b9400002        ldr     w2, [x0]
  24:   9279e273        and     x19, x19, #0xffffffffffffff80	<------
  28:   7100105f        cmp     w2, #0x4
  2c:   540001a1        b.ne    60 <ext4_chksum.isra.14.constprop.19+0x5c>  // b.any
  30:   2a0303e4        mov     w4, w3
  34:   aa0003e3        mov     x3, x0
  38:   b9008264        str     w4, [x19, #128]
  3c:   aa1303e0        mov     x0, x19
  40:   f9000263        str     x3, [x19]			<------
  44:   94000000        bl      0 <crypto_shash_update>
                        44: R_AARCH64_CALL26    crypto_shash_update
  48:   350000e0        cbnz    w0, 64 <ext4_chksum.isra.14.constprop.19+0x60>
  4c:   910003bf        mov     sp, x29				<======
  50:   b9408260        ldr     w0, [x19, #128]			<======
  54:   f9400bf3        ldr     x19, [sp, #16]
  58:   a8c27bfd        ldp     x29, x30, [sp], #32
  5c:   d65f03c0        ret
  60:   d4210000        brk     #0x800
  64:   97ffffe7        bl      0 <ext4_chksum.isra.14.part.15>

Of the instructions that are highlighted with "<------" and "<======",
x29 is located at the bottom of the function's stack frame, excluding
local variables.  x19 is "desc", which is calculated to be safely below
x29 and aligned to a 128 byte boundary.

The bug is pointed to by the two "<======" markers - the instruction
at 4c restores the stack pointer _above_ "desc" before then loading
desc.ctx.

If an interrupt occurs right between these two instructions, then
desc.ctx will be corrupted, leading to the checksum failing.

Comments on irc are long the lines of this being "an impressive
compiler bug".

We now need to find which gcc versions are affected, so we know what
minimum version to require for aarch64.

Arnd has been unable to find anything in gcc bugzilla to explain this;
he's tested gcc-5.5.0, which appears to produce correct code, and is
trying to bisect between 4.9.4 and 5.1.0 to locate where this was
fixed.

Peter Zijlstra suggested adding linux-toolchains@ and asking compiler
folks for feedback on this bug. I guess a pointer to whether this is
a known bug, and which bug may be useful.

I am very relieved to have found a positive reason for this bug, rather
than just moving forward on the compiler and have the bug vanish
without explanation, never knowing if it would rear its head in future
and corrupt my filesystems, e.g. never knowing if it became a
temporarily masked memory ordering bug.

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

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Russell King - ARM Linux admin]

On Thu, Jan 07, 2021 at 11:18:41AM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 06, 2021 at 10:32:23PM +0000, Russell King - ARM Linux admin wrote:
> > On Wed, Jan 06, 2021 at 05:20:34PM +0000, Will Deacon wrote:
> > > With that, I see the following after ten seconds or so:
> > > 
> > >   EXT4-fs error (device sda2): ext4_lookup:1707: inode #674497: comm md5sum: iget: checksum invalid
> > > 
> > > Russell, Mark -- does this recipe explode reliably for you too?
> > 
> > I've been working this evening on tracking down what change in the
> > Kconfig file between your working 5.10 kernel binary you supplied me,
> > and my failing 5.9 kernel.
> > 
> > I've found that _enabling_ CONFIG_STACKPROTECTOR appears to mask the
> > inode checksum failure problem, at least from a short test.) I'm going
> > to re-enable CONFIG_STACKPROTECTOR and leave it running for longer.
> > 
> > That is:
> > 
> > CONFIG_STACKPROTECTOR=y
> > CONFIG_STACKPROTECTOR_STRONG=y
> > 
> > appears to mask the problem
> > 
> > # CONFIG_STACKPROTECTOR is not set
> > 
> > appears to unmask the problem.
> 
> We have finally got to the bottom of this - the "bug" is in the ext4
> code:
> 
> static inline u32 ext4_chksum(struct ext4_sb_info *sbi, u32 crc,
>                               const void *address, unsigned int length)
> {
>         struct {
>                 struct shash_desc shash;
>                 char ctx[4];
>         } desc;
> 
>         BUG_ON(crypto_shash_descsize(sbi->s_chksum_driver)!=sizeof(desc.ctx));
> 
>         desc.shash.tfm = sbi->s_chksum_driver;
>         *(u32 *)desc.ctx = crc;
> 
>         BUG_ON(crypto_shash_update(&desc.shash, address, length));
> 
>         return *(u32 *)desc.ctx;
> }
> 
> This isn't always inlined, despite the "inline" keyword. With GCC
> 4.9.4, this is compiled to the following code when the stack protector
> is disabled:
> 
> 0000000000000004 <ext4_chksum.isra.14.constprop.19>:
>    4:   a9be7bfd        stp     x29, x30, [sp, #-32]!		<------
>    8:   2a0103e3        mov     w3, w1
>    c:   aa0203e1        mov     x1, x2
>   10:   910003fd        mov     x29, sp				<------
>   14:   f9000bf3        str     x19, [sp, #16]
>   18:   d10603ff        sub     sp, sp, #0x180			<------
>   1c:   9101fff3        add     x19, sp, #0x7f
>   20:   b9400002        ldr     w2, [x0]
>   24:   9279e273        and     x19, x19, #0xffffffffffffff80	<------
>   28:   7100105f        cmp     w2, #0x4
>   2c:   540001a1        b.ne    60 <ext4_chksum.isra.14.constprop.19+0x5c>  // b.any
>   30:   2a0303e4        mov     w4, w3
>   34:   aa0003e3        mov     x3, x0
>   38:   b9008264        str     w4, [x19, #128]
>   3c:   aa1303e0        mov     x0, x19
>   40:   f9000263        str     x3, [x19]			<------
>   44:   94000000        bl      0 <crypto_shash_update>
>                         44: R_AARCH64_CALL26    crypto_shash_update
>   48:   350000e0        cbnz    w0, 64 <ext4_chksum.isra.14.constprop.19+0x60>
>   4c:   910003bf        mov     sp, x29				<======
>   50:   b9408260        ldr     w0, [x19, #128]			<======
>   54:   f9400bf3        ldr     x19, [sp, #16]
>   58:   a8c27bfd        ldp     x29, x30, [sp], #32
>   5c:   d65f03c0        ret
>   60:   d4210000        brk     #0x800
>   64:   97ffffe7        bl      0 <ext4_chksum.isra.14.part.15>
> 
> Of the instructions that are highlighted with "<------" and "<======",
> x29 is located at the bottom of the function's stack frame, excluding
> local variables.  x19 is "desc", which is calculated to be safely below
> x29 and aligned to a 128 byte boundary.
> 
> The bug is pointed to by the two "<======" markers - the instruction
> at 4c restores the stack pointer _above_ "desc" before then loading
> desc.ctx.
> 
> If an interrupt occurs right between these two instructions, then
> desc.ctx will be corrupted, leading to the checksum failing.
> 
> Comments on irc are long the lines of this being "an impressive
> compiler bug".
> 
> We now need to find which gcc versions are affected, so we know what
> minimum version to require for aarch64.
> 
> Arnd has been unable to find anything in gcc bugzilla to explain this;
> he's tested gcc-5.5.0, which appears to produce correct code, and is
> trying to bisect between 4.9.4 and 5.1.0 to locate where this was
> fixed.
> 
> Peter Zijlstra suggested adding linux-toolchains@ and asking compiler
> folks for feedback on this bug. I guess a pointer to whether this is
> a known bug, and which bug may be useful.
> 
> I am very relieved to have found a positive reason for this bug, rather
> than just moving forward on the compiler and have the bug vanish
> without explanation, never knowing if it would rear its head in future
> and corrupt my filesystems, e.g. never knowing if it became a
> temporarily masked memory ordering bug.

Arnd has found via bisecting gcc:

7e8c2bd54af ("[AArch64] fix unsafe access to deallocated stack")

which seems to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63293

That seems to suggest that gcc-5.0.0 is also affected. 

Looking at the changelog in Debian's gcc-8.3 packages, this doesn't
feature, so it's not easy just to look at the changelogs to work out
which versions are affected.

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

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Arnd Bergmann]

On Thu, Jan 7, 2021 at 1:47 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:

> Arnd has found via bisecting gcc:
>
> 7e8c2bd54af ("[AArch64] fix unsafe access to deallocated stack")
>
> which seems to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63293
>
> That seems to suggest that gcc-5.0.0 is also affected.
>
> Looking at the changelog in Debian's gcc-8.3 packages, this doesn't
> feature, so it's not easy just to look at the changelogs to work out
> which versions are affected.

I checked the history to confirm that all gcc-5 releases (5.0.x is pre-release)
and later have the fix.

The gcc bugzilla mentions backports into gcc-linaro, but I do not see
them in my git history.

       Arnd

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Russell King - ARM Linux admin]

On Thu, Jan 07, 2021 at 02:16:25PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 7, 2021 at 1:47 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> 
> > Arnd has found via bisecting gcc:
> >
> > 7e8c2bd54af ("[AArch64] fix unsafe access to deallocated stack")
> >
> > which seems to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63293
> >
> > That seems to suggest that gcc-5.0.0 is also affected.
> >
> > Looking at the changelog in Debian's gcc-8.3 packages, this doesn't
> > feature, so it's not easy just to look at the changelogs to work out
> > which versions are affected.
> 
> I checked the history to confirm that all gcc-5 releases (5.0.x is pre-release)
> and later have the fix.
> 
> The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> them in my git history.

So, do we raise the minimum gcc version for the kernel as a whole to 5.1
or just for aarch64?

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

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Theodore Ts'o]

On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > them in my git history.
> 
> So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> or just for aarch64?

Russell, Arnd, thanks so much for tracking down the root cause of the
bug!

I will note that RHEL 7 uses gcc 4.8.  I personally don't have an
objections to requiring developers using RHEL 7 to have to install a
more modern gcc (since I use Debian Testing and gcc 10.2.1, myself,
and gcc 5.1 is so five years ago :-), but I could imagine that being
considered inconvenient for some.

						- Ted

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Florian Weimer]

* Theodore Ts'o:

> On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
>> > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
>> > them in my git history.
>> 
>> So, do we raise the minimum gcc version for the kernel as a whole to 5.1
>> or just for aarch64?
>
> Russell, Arnd, thanks so much for tracking down the root cause of the
> bug!
>
> I will note that RHEL 7 uses gcc 4.8.  I personally don't have an
> objections to requiring developers using RHEL 7 to have to install a
> more modern gcc (since I use Debian Testing and gcc 10.2.1, myself,
> and gcc 5.1 is so five years ago :-), but I could imagine that being
> considered inconvenient for some.

Actually, RHEL 7 should have the fix (internal bug #1362635, curiously
we encountered it in the *XFS* CRC calculation code back then).

My understanding is that RHEL 7 aarch64 support ceased completely about
a month ago, so that shouldn't be an argument against bumping the
minimum version requirement to 5.1.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Arnd Bergmann]

On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > them in my git history.
> >
> > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > or just for aarch64?
>
> Russell, Arnd, thanks so much for tracking down the root cause of the
> bug!

There is one more thing that I wondered about when looking through
the ext4 code: Should it just call the crc32c_le() function directly
instead of going through the crypto layer? It seems that with Ard's
rework from 2018, that can just call the underlying architecture specific
implementation anyway.

> I will note that RHEL 7 uses gcc 4.8.  I personally don't have an
> objections to requiring developers using RHEL 7 to have to install a
> more modern gcc (since I use Debian Testing and gcc 10.2.1, myself,
> and gcc 5.1 is so five years ago :-), but I could imagine that being
> considered inconvenient for some.

The main users of gcc-4.9 that I recall from previous discussions
were Android and Debian 8, but both of them are done now: Debian 8
has reached its end of life last summer, and Android uses clang
for building new kernels.

       Arnd

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Russell King - ARM Linux admin]

On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > them in my git history.
> > >
> > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > or just for aarch64?
> >
> > Russell, Arnd, thanks so much for tracking down the root cause of the
> > bug!
> 
> There is one more thing that I wondered about when looking through
> the ext4 code: Should it just call the crc32c_le() function directly
> instead of going through the crypto layer? It seems that with Ard's
> rework from 2018, that can just call the underlying architecture specific
> implementation anyway.

Yes, I've been wondering about that too. To me, it looks like the
ext4 code performs a layering violation by going "under the covers"
- there are accessor functions to set the CRC and retrieve it. ext4
instead just makes the assumption that the CRC value is stored after
struct shash_desc. Especially as the crypto/crc32c code references
the value using:

	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);

Not even crypto drivers are allowed to assume that desc+1 is where
the CRC is stored.

However, struct shash_desc is already 128 bytes in size on aarch64,
and the proper way of doing it via SHASH_DESC_ON_STACK() is overkill,
being strangely 2 * sizeof(struct shash_desc) + 360 (which looks like
another bug to me!)

#define HASH_MAX_DESCSIZE       (sizeof(struct shash_desc) + 360)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
#define SHASH_DESC_ON_STACK(shash, ctx)                           \
        char __##shash##_desc[sizeof(struct shash_desc) +         \
                              ^^^^^^^^^^^^^^^^^^^^^^^^^
                HASH_MAX_DESCSIZE] CRYPTO_MINALIGN_ATTR; \
        struct shash_desc *shash = (struct shash_desc *)__##shash##_desc

So, I agree with you wrt crc32c_le(), especially as it would be more
efficient, and as the use of crc32c is already hard coded in the ext4
code - not only with crypto_alloc_shash("crc32c", 0, 0) but also with
the fixed-size structure in ext4_chksum().

However, it's ultimately up to the ext4 maintainers to decide.

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

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Eric Biggers]

On Thu, Jan 07, 2021 at 10:14:46PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> > On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
> > >
> > > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > > them in my git history.
> > > >
> > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > > or just for aarch64?
> > >
> > > Russell, Arnd, thanks so much for tracking down the root cause of the
> > > bug!
> > 
> > There is one more thing that I wondered about when looking through
> > the ext4 code: Should it just call the crc32c_le() function directly
> > instead of going through the crypto layer? It seems that with Ard's
> > rework from 2018, that can just call the underlying architecture specific
> > implementation anyway.
> 
> Yes, I've been wondering about that too. To me, it looks like the
> ext4 code performs a layering violation by going "under the covers"
> - there are accessor functions to set the CRC and retrieve it. ext4
> instead just makes the assumption that the CRC value is stored after
> struct shash_desc. Especially as the crypto/crc32c code references
> the value using:
> 
> 	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> 
> Not even crypto drivers are allowed to assume that desc+1 is where
> the CRC is stored.

It violates how the shash API is meant to be used in general, but there is a
test that enforces that the shash_desc_ctx for crc32c must be just the single
u32 crc value.  See alg_test_crc32c() in crypto/testmgr.c.  So it's apparently
intended to work.

> 
> However, struct shash_desc is already 128 bytes in size on aarch64,

Ard Biesheuvel recently sent a patch to reduce the alignment of struct
shash_desc to ARCH_SLAB_MINALIGN
(https://lkml.kernel.org/linux-crypto/20210107124128.19791-1-ardb@kernel.org/),
since apparently most of the bloat is from alignment for DMA, which isn't
necessary.  I think that reduces the size by a lot on arm64.

> and the proper way of doing it via SHASH_DESC_ON_STACK() is overkill,
> being strangely 2 * sizeof(struct shash_desc) + 360 (which looks like
> another bug to me!)

Are you referring to the '2 * sizeof(struct shash_desc)' rather than just
'sizeof(struct shash_desc)'?  As mentioned in the comment above
HASH_MAX_DESCSIZE, there can be a nested shash_desc due to HMAC.
So I believe the value is correct.

> So, I agree with you wrt crc32c_le(), especially as it would be more
> efficient, and as the use of crc32c is already hard coded in the ext4
> code - not only with crypto_alloc_shash("crc32c", 0, 0) but also with
> the fixed-size structure in ext4_chksum().
> 
> However, it's ultimately up to the ext4 maintainers to decide.

As I mentioned in my other response, crc32c_le() isn't a proper library API
(like some of the newer lib/crypto/ stuff) but rather just a wrapper for the
shash API, and it doesn't handle modules being dynamically loaded/unloaded.
So switching to it may cause a performance regression.

What I'd recommend is making crc32c_le() able to call architecture-speccific
implementations directly, similar to blake2s() and chacha20() in lib/crypto/.
Then there would be no concern about when modules get loaded, etc...

- Eric

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Ard Biesheuvel]

On Thu, 7 Jan 2021 at 23:42, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Jan 07, 2021 at 10:14:46PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> > > On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
> > > >
> > > > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > > > them in my git history.
> > > > >
> > > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > > > or just for aarch64?
> > > >
> > > > Russell, Arnd, thanks so much for tracking down the root cause of the
> > > > bug!
> > >
> > > There is one more thing that I wondered about when looking through
> > > the ext4 code: Should it just call the crc32c_le() function directly
> > > instead of going through the crypto layer? It seems that with Ard's
> > > rework from 2018, that can just call the underlying architecture specific
> > > implementation anyway.
> >
> > Yes, I've been wondering about that too. To me, it looks like the
> > ext4 code performs a layering violation by going "under the covers"
> > - there are accessor functions to set the CRC and retrieve it. ext4
> > instead just makes the assumption that the CRC value is stored after
> > struct shash_desc. Especially as the crypto/crc32c code references
> > the value using:
> >
> >       struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> >
> > Not even crypto drivers are allowed to assume that desc+1 is where
> > the CRC is stored.
>
> It violates how the shash API is meant to be used in general, but there is a
> test that enforces that the shash_desc_ctx for crc32c must be just the single
> u32 crc value.  See alg_test_crc32c() in crypto/testmgr.c.  So it's apparently
> intended to work.
>
> >
> > However, struct shash_desc is already 128 bytes in size on aarch64,
>
> Ard Biesheuvel recently sent a patch to reduce the alignment of struct
> shash_desc to ARCH_SLAB_MINALIGN
> (https://lkml.kernel.org/linux-crypto/20210107124128.19791-1-ardb@kernel.org/),
> since apparently most of the bloat is from alignment for DMA, which isn't
> necessary.  I think that reduces the size by a lot on arm64.
>
> > and the proper way of doing it via SHASH_DESC_ON_STACK() is overkill,
> > being strangely 2 * sizeof(struct shash_desc) + 360 (which looks like
> > another bug to me!)
>
> Are you referring to the '2 * sizeof(struct shash_desc)' rather than just
> 'sizeof(struct shash_desc)'?  As mentioned in the comment above
> HASH_MAX_DESCSIZE, there can be a nested shash_desc due to HMAC.
> So I believe the value is correct.
>
> > So, I agree with you wrt crc32c_le(), especially as it would be more
> > efficient, and as the use of crc32c is already hard coded in the ext4
> > code - not only with crypto_alloc_shash("crc32c", 0, 0) but also with
> > the fixed-size structure in ext4_chksum().
> >
> > However, it's ultimately up to the ext4 maintainers to decide.
>
> As I mentioned in my other response, crc32c_le() isn't a proper library API
> (like some of the newer lib/crypto/ stuff) but rather just a wrapper for the
> shash API, and it doesn't handle modules being dynamically loaded/unloaded.
> So switching to it may cause a performance regression.
>
> What I'd recommend is making crc32c_le() able to call architecture-speccific
> implementations directly, similar to blake2s() and chacha20() in lib/crypto/.
> Then there would be no concern about when modules get loaded, etc...
>

I have looked into this in the past, both for crc32(c) and crc-t10dif,
both of which use a horrid method of wrapping a shash into a library
API. This was before we had static calls, though, and this work kind
of stalled on that. It should be straight-forward to redefine the
crc32c() library function as a static call, and have an optimized
module (or builtin) perform the [conditional] static call update at
module_init() time. The only missing piece is autoloading such
modules, which is tricky with softdeps if the dependency is from the
core kernel.

Currently, we have many users of crc32(c) in the kernel that go via
the shash (or synchronous ahash) layer to perform crc32c, all of which
would be better served by a library API, given that the hash type is a
compile time constant, and only synchronous calls are made.




drivers/infiniband/hw/i40iw/i40iw_utils.c: tfm =
crypto_alloc_shash("crc32c", 0, 0);
drivers/infiniband/sw/rxe/rxe_verbs.c: tfm = crypto_alloc_shash("crc32", 0, 0);
drivers/infiniband/sw/siw/siw_main.c: siw_crypto_shash =
crypto_alloc_shash("crc32c", 0, 0);
drivers/md/dm-crypt.c: tcw->crc32_tfm = crypto_alloc_shash("crc32", 0,
drivers/nvme/host/tcp.c: tfm = crypto_alloc_ahash("crc32c", 0,
CRYPTO_ALG_ASYNC);
drivers/nvme/target/tcp.c: tfm = crypto_alloc_ahash("crc32c", 0,
CRYPTO_ALG_ASYNC);
drivers/scsi/iscsi_tcp.c: tfm = crypto_alloc_ahash("crc32c", 0,
CRYPTO_ALG_ASYNC);
drivers/target/iscsi/iscsi_target_login.c: tfm =
crypto_alloc_ahash("crc32c", 0, CRYPTO_ALG_ASYNC);
fs/ext4/super.c: sbi->s_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
fs/f2fs/super.c: sbi->s_chksum_driver = crypto_alloc_shash("crc32", 0, 0);
fs/jbd2/journal.c: journal->j_chksum_driver =
crypto_alloc_shash("crc32c", 0, 0);
fs/jbd2/journal.c: journal->j_chksum_driver =
crypto_alloc_shash("crc32c", 0, 0);
lib/libcrc32c.c: tfm = crypto_alloc_shash("crc32c", 0, 0);

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Eric Biggers]

On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > them in my git history.
> > >
> > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > or just for aarch64?
> >
> > Russell, Arnd, thanks so much for tracking down the root cause of the
> > bug!
> 
> There is one more thing that I wondered about when looking through
> the ext4 code: Should it just call the crc32c_le() function directly
> instead of going through the crypto layer? It seems that with Ard's
> rework from 2018, that can just call the underlying architecture specific
> implementation anyway.
> 

It looks like that would work, although note that crc32c_le() uses the shash API
too, so it isn't any more "direct" than what ext4 does now.

Also, a potential issue is that the implementation of crc32c that crc32c_le()
uses might be chosen too early if the architecture-specific implementation of
crc32c is compiled as a module (e.g. crc32c-intel.ko).  There are two ways this
could be fixed -- either by making it a proper library API like blake2s() that
can call the architecture-specific code directly, or by reconfiguring things
when a new crypto module is loaded (like what lib/crc-t10dif.c does).

Until one of those is done, switching to crc32c_le() might cause performance
regressions.

- Eric

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Darrick J. Wong]

On Thu, Jan 07, 2021 at 02:27:51PM -0800, Eric Biggers wrote:
> On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> > On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
> > >
> > > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > > them in my git history.
> > > >
> > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > > or just for aarch64?
> > >
> > > Russell, Arnd, thanks so much for tracking down the root cause of the
> > > bug!
> > 
> > There is one more thing that I wondered about when looking through
> > the ext4 code: Should it just call the crc32c_le() function directly
> > instead of going through the crypto layer? It seems that with Ard's
> > rework from 2018, that can just call the underlying architecture specific
> > implementation anyway.
> > 
> 
> It looks like that would work, although note that crc32c_le() uses the shash API
> too, so it isn't any more "direct" than what ext4 does now.

Yes.

> Also, a potential issue is that the implementation of crc32c that crc32c_le()
> uses might be chosen too early if the architecture-specific implementation of
> crc32c is compiled as a module (e.g. crc32c-intel.ko).

This was the primary reason I chose to do it this way for ext4.

The other is that ext4 didn't use crc32c before metadata_csum, so
there's no point in pulling in the crypto layer if you're only going to
use older ext2 or ext3 filesystems.  That was 2010, maybe people have
stopped doing that?

> There are two ways this
> could be fixed -- either by making it a proper library API like blake2s() that
> can call the architecture-specific code directly, or by reconfiguring things
> when a new crypto module is loaded (like what lib/crc-t10dif.c does).

Though I would like to see the library functions gain the ability to use
whatever is the fastest mechanism available once we can be reasonably
certain that all the platform-specific drivers have been loaded.

That said, IIRC most distros compile all of them into their
(increasingly large) vmlinuz files so maybe this isn't much of practical
concern?

--D
> 
> Until one of those is done, switching to crc32c_le() might cause performance
> regressions.
> 
> - Eric

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Arnd Bergmann]

On Fri, Jan 8, 2021 at 12:53 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Thu, Jan 07, 2021 at 02:27:51PM -0800, Eric Biggers wrote:
> > On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> > > On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@mit.edu> wrote:
> > > >
> > > > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > > > them in my git history.
> > > > >
> > > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > > > or just for aarch64?
> > > >
> > > > Russell, Arnd, thanks so much for tracking down the root cause of the
> > > > bug!
> > >
> > > There is one more thing that I wondered about when looking through
> > > the ext4 code: Should it just call the crc32c_le() function directly
> > > instead of going through the crypto layer? It seems that with Ard's
> > > rework from 2018, that can just call the underlying architecture specific
> > > implementation anyway.
> > >
> >
> > It looks like that would work, although note that crc32c_le() uses the shash API
> > too, so it isn't any more "direct" than what ext4 does now.
>
> Yes.

Ah, I see. I had only noticed the architecture specific overrides for
__crc32c_le(),
and the global __weak crc32_le() function in lib/crc32.c, but failed to notice
the crc32c_le() macro that redirects to crc32c().

> > Also, a potential issue is that the implementation of crc32c that crc32c_le()
> > uses might be chosen too early if the architecture-specific implementation of
> > crc32c is compiled as a module (e.g. crc32c-intel.ko).
>
> This was the primary reason I chose to do it this way for ext4.
>
> The other is that ext4 didn't use crc32c before metadata_csum, so
> there's no point in pulling in the crypto layer if you're only going to
> use older ext2 or ext3 filesystems.  That was 2010, maybe people have
> stopped doing that?

The per-architecture overrides for __crc32c_le() are from 2018. With that
it should be possible to just always have the fastest implementation
(forcing them to be built-in normally), but not all architectures do this.

> > There are two ways this
> > could be fixed -- either by making it a proper library API like blake2s() that
> > can call the architecture-specific code directly, or by reconfiguring things
> > when a new crypto module is loaded (like what lib/crc-t10dif.c does).
>
> Though I would like to see the library functions gain the ability to use
> whatever is the fastest mechanism available once we can be reasonably
> certain that all the platform-specific drivers have been loaded.
>
> That said, IIRC most distros compile all of them into their
> (increasingly large) vmlinuz files so maybe this isn't much of practical
> concern?

I recently made checked the missing dependencies of drivers that
fail to 'select CRC32' but do call it directly. With those added, there
are now around 200 drivers that include it, and in practice you would
hardly find any kernel that doesn't have it built-in already. Most notably,
jbd2 already calls crc32_be(), so it is impossible to build an EXT4
without it. For memory-constrained embedded devices, it would probably
be more valuable to build without the crypto layer than without crc32.

       Arnd

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Peter Zijlstra]

On Thu, Jan 07, 2021 at 11:27:22AM -0500, Theodore Ts'o wrote:
> I will note that RHEL 7 uses gcc 4.8.  I personally don't have an
> objections to requiring developers using RHEL 7 to have to install a
> more modern gcc (since I use Debian Testing and gcc 10.2.1, myself,
> and gcc 5.1 is so five years ago :-), but I could imagine that being
> considered inconvenient for some.

The kernel minimum (4.9) is already past that, so RHEL7 people are
already out in the cold.

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Hi!

> > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > them in my git history.
> > 
> > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > or just for aarch64?
> 
> Russell, Arnd, thanks so much for tracking down the root cause of the
> bug!
> 
> I will note that RHEL 7 uses gcc 4.8.  I personally don't have an
> objections to requiring developers using RHEL 7 to have to install a
> more modern gcc (since I use Debian Testing and gcc 10.2.1, myself,
> and gcc 5.1 is so five years ago :-), but I could imagine that being
> considered inconvenient for some.

I'm on gcc 4.9.2 on a machine that is hard to upgrade :-(.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Arnd Bergmann]

On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Thu, Jan 07, 2021 at 02:16:25PM +0100, Arnd Bergmann wrote:
> > On Thu, Jan 7, 2021 at 1:47 PM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
>
> > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > them in my git history.

Correction: I looked in the wrong branch, gcc-linaro does have it, as
does the Android gcc, which was recently still at 4.9 before they dropped it
in favor of clang.

> So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> or just for aarch64?

I'd personally love to see gcc-5 as the global minimum version, as that
would let us finally use --std=gnu11 features instead of gnu89. [There are
a couple of useful features that are incompatible with gnu89, and
gnu99/gnu11 support in gcc didn't like the kernel sources]

If we make it arm64 specific, I'd propose only making it a build-time
warning instead of an error, as there are no other benefits to increasing
the minimum version if gcc-4.9 is still an option for other architectures,
and most gcc-4.9 users (Android, Red Hat and everyone using gcc-linaro)
have backported this bugfix already.

         Arnd

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Peter Zijlstra]

On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin

> > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > or just for aarch64?
> 
> I'd personally love to see gcc-5 as the global minimum version, as that
> would let us finally use --std=gnu11 features instead of gnu89. [There are
> a couple of useful features that are incompatible with gnu89, and
> gnu99/gnu11 support in gcc didn't like the kernel sources]

+1 for raising the tree-wide minimum (again!). We actually have a bunch
of work-arounds for 4.9 bugs we can get rid of as well.

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Will Deacon]

On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
> > On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
> 
> > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > or just for aarch64?
> > 
> > I'd personally love to see gcc-5 as the global minimum version, as that
> > would let us finally use --std=gnu11 features instead of gnu89. [There are
> > a couple of useful features that are incompatible with gnu89, and
> > gnu99/gnu11 support in gcc didn't like the kernel sources]
> 
> +1 for raising the tree-wide minimum (again!). We actually have a bunch
> of work-arounds for 4.9 bugs we can get rid of as well.

We even just added another one for arm64 KVM! [1]

So yes, I'm in favour of leaving gcc 4.9 to rot as well, especially after
this ext4 debugging experience.

Will

[1] https://git.kernel.org/linus/9fd339a45be5

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Linus Torvalds]

On Fri, Jan 8, 2021 at 1:27 AM Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
> > On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
> > > On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
> >
> > > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > > or just for aarch64?
> > >
> > > I'd personally love to see gcc-5 as the global minimum version, as that
> > > would let us finally use --std=gnu11 features instead of gnu89. [There are
> > > a couple of useful features that are incompatible with gnu89, and
> > > gnu99/gnu11 support in gcc didn't like the kernel sources]
> >
> > +1 for raising the tree-wide minimum (again!). We actually have a bunch
> > of work-arounds for 4.9 bugs we can get rid of as well.
>
> We even just added another one for arm64 KVM! [1]
>
> So yes, I'm in favour of leaving gcc 4.9 to rot as well, especially after
> this ext4 debugging experience.

Well, honestly, I'm always in favor of having people not use ancient
compilers, but both of the issues at hand do seem to be specific to
arm64.

The "gcc before 5.1 generates incorrect stack pointer writes on arm64"
sounds pretty much deadly, and I think means that yes, for arm64 we
simply need to require 5.1 or newer.

I also suspect there is much less reason to use old gcc's on arm64. I
can't imagine that people really run very old setups, Is some old RHEL
version even relevant for arm64?

So while I'd love to just say "everybody needs to make sure they have
an up-to-date compiler", my git feel is that at least with the current
crop of issues, there is little to really push us globally.

I appreciate Arnd pointing out "--std=gnu11", though. What are the
actual relevant language improvements?

Variable declarations in for-loops is the only one I can think of. I
think that would clean up some code (and some macros), but might not
be compelling on its own.

               Linus

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Arnd Bergmann]

On Fri, Jan 8, 2021 at 9:02 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jan 8, 2021 at 1:27 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
> > > On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
> > > > On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
>
> I appreciate Arnd pointing out "--std=gnu11", though. What are the
> actual relevant language improvements?
>
> Variable declarations in for-loops is the only one I can think of. I
> think that would clean up some code (and some macros), but might not
> be compelling on its own.

I think that was the main one, as most of --std=c11 is already part
of --std=gnu89 as a gnu extension. There were a few things that
came up with clang porting, as clang is somewhat closer to gnu11
than to gnu89, but I don't remember exactly what that was.

I would still like to improve READ_ONCE()/get_user()/cmpxchg()
further using __auto_type and _Generic where possible, but I think
that was already supported in gcc-4.9, and does not require gcc-5.

       Arnd

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Nick Desaulniers]

On Fri, Jan 8, 2021 at 12:34 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Fri, Jan 8, 2021 at 9:02 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Jan 8, 2021 at 1:27 AM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
> > > > On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
> > > > > On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
> >
> > I appreciate Arnd pointing out "--std=gnu11", though. What are the
> > actual relevant language improvements?

It's hard to say, since a lot of new language features were already
GNU C extensions.

The only semantic difference I'm aware of is the semantics of `extern
inline` changed 100% from c89 to c99 (so jumping from gnu89 to gnu11
would change that).  We already #define inline to
__attribute__((__gnu_inline)) (there's also a -fgnu-inline flag), but
I worry for places that don't include that header or drop
KBUILD_CFLAGS (like every vdso), though `extern inline` is awful (and
I should be put in jail for introducing it to the kernel; now we have
__attribute__((no_stack_protector)) in both toolchains, and should be
using that instead, but we don't have it yet for all supported
compiler versions).

A quick grep through clang's sources shows mostly parser changes for
_Noreturn, _Alignof and friends etc..  New to me are unicode literal
strings (u or U suffix or prefix?) and something about loops expected
to make forward progress???

Another thing I've been worried about is Makefiles that reset
KBUILD_CFLAGS, since that's a constant source of pain/breakage for
cross compiling from Clang.  That tends to drop -std=gnu89.  For
instance:

$ make LLVM=1 -j71 defconfig
$ make LLVM=1 -j71 V=1 &>log.txt
$ grep -v std=gnu89 log.txt | grep clang | rev | cut -d ' ' -f 1 | rev
| grep -v \\.S
arch/x86/realmode/rm/wakemain.c
arch/x86/realmode/rm/video-mode.c
arch/x86/realmode/rm/regs.c
arch/x86/realmode/rm/video-vga.c
arch/x86/realmode/rm/video-vesa.c
arch/x86/realmode/rm/video-bios.c
drivers/firmware/efi/libstub/efi-stub-helper.c
drivers/firmware/efi/libstub/gop.c
drivers/firmware/efi/libstub/secureboot.c
drivers/firmware/efi/libstub/tpm.c
drivers/firmware/efi/libstub/file.c
drivers/firmware/efi/libstub/mem.c
drivers/firmware/efi/libstub/random.c
drivers/firmware/efi/libstub/randomalloc.c
drivers/firmware/efi/libstub/pci.c
drivers/firmware/efi/libstub/skip_spaces.c
lib/cmdline.c
lib/ctype.c
drivers/firmware/efi/libstub/alignedmem.c
drivers/firmware/efi/libstub/relocate.c
drivers/firmware/efi/libstub/vsprintf.c
drivers/firmware/efi/libstub/x86-stub.c
arch/x86/boot/a20.c
arch/x86/boot/cmdline.c
arch/x86/boot/cpuflags.c
arch/x86/boot/cpucheck.c
arch/x86/boot/early_serial_console.c
arch/x86/boot/edd.c
arch/x86/boot/main.c
arch/x86/boot/memory.c
arch/x86/boot/pm.c
arch/x86/boot/printf.c
arch/x86/boot/regs.c
arch/x86/boot/string.c
arch/x86/boot/tty.c
arch/x86/boot/video.c
arch/x86/boot/video-mode.c
arch/x86/boot/version.c
arch/x86/boot/video-vga.c
arch/x86/boot/video-vesa.c
arch/x86/boot/video-bios.c
arch/x86/boot/cpu.c
arch/x86/boot/compressed/string.c
arch/x86/boot/compressed/cmdline.c
arch/x86/boot/compressed/error.c
arch/x86/boot/compressed/cpuflags.c
arch/x86/boot/compressed/early_serial_console.c
arch/x86/boot/compressed/kaslr.c
arch/x86/boot/compressed/ident_map_64.c
arch/x86/boot/compressed/idt_64.c
arch/x86/boot/compressed/pgtable_64.c
arch/x86/boot/compressed/acpi.c
arch/x86/boot/compressed/misc.c

So it looks like parts of the tree are already built with -std=gnu11
or -std=gnu17, as they rely on the implicit default C language mode
when unspecified.  Oops?
-- 
Thanks,
~Nick Desaulniers

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Russell King - ARM Linux admin]

On Fri, Jan 08, 2021 at 12:02:53PM -0800, Linus Torvalds wrote:
> Well, honestly, I'm always in favor of having people not use ancient
> compilers, but both of the issues at hand do seem to be specific to
> arm64.
> 
> The "gcc before 5.1 generates incorrect stack pointer writes on arm64"
> sounds pretty much deadly, and I think means that yes, for arm64 we
> simply need to require 5.1 or newer.
> 
> I also suspect there is much less reason to use old gcc's on arm64. I
> can't imagine that people really run very old setups, Is some old RHEL
> version even relevant for arm64?

For me, six years old for a compiler is really not "very old" - and,
when I first encountered this problem, it was over 12 months ago.
Apart from the kernel, I am not in the habbit of upgrading stuff for
the sake of upgrading - I tend to stick with what I have and what
works. Not everyone on this planet has a desire to have the latest
and greatest all the time.

Since then, I've _not_ wanted to change the compiler in case the
problem vanishes without explanation - it had the feeling of being
way more serious than a compiler bug, potentially a memory ordering
bug.

It took about a year just to start being able to work out what was
going on - it would take up to about three months to show for me,
and when it did, it spat out an ext4 inode checksum error and made
the rootfs read-only.

To "hide" that by upgrading the compiler, and then to be in the
situation where you do not trust any aarch64 machine with your data
is no real solution. That's exactly where I was until this had been
found. The aarch64 architecture had completely lost my trust as a
viable computing platform - and I was at the point of considering
disposing of all my aarch64 hardware and replacing it with x86.

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

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Lukas Wunner]

On Fri, Jan 08, 2021 at 12:02:53PM -0800, Linus Torvalds wrote:
> I appreciate Arnd pointing out "--std=gnu11", though. What are the
> actual relevant language improvements?
> 
> Variable declarations in for-loops is the only one I can think of. I
> think that would clean up some code (and some macros), but might not
> be compelling on its own.

Anonymous structs/unions.  I used to have a use case for that in
struct efi_dev_path in include/linux/efi.h, but Ard Biesheuvel
refactored it in a gnu89-compatible way for v5.7 with db8952e7094f.

[The above was copy-pasted from last time this discussion came up
in July 2020.  Back then, Kirill Shutemov likewise mentioned the
local variables in loops feature:
https://lore.kernel.org/lkml/20200710111724.m4jaci73pykalxys@wunner.de/
]

Thanks,

Lukas

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Florian Weimer]

* Lukas Wunner:

> On Fri, Jan 08, 2021 at 12:02:53PM -0800, Linus Torvalds wrote:
>> I appreciate Arnd pointing out "--std=gnu11", though. What are the
>> actual relevant language improvements?
>> 
>> Variable declarations in for-loops is the only one I can think of. I
>> think that would clean up some code (and some macros), but might not
>> be compelling on its own.
>
> Anonymous structs/unions.  I used to have a use case for that in
> struct efi_dev_path in include/linux/efi.h, but Ard Biesheuvel
> refactored it in a gnu89-compatible way for v5.7 with db8952e7094f.

Aren't those a GNU extension supported since GCC 3.0?

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

RE: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[David Laight]

From: Florian Weimer
> Sent: 12 January 2021 13:32
> 
> * Lukas Wunner:
> 
> > On Fri, Jan 08, 2021 at 12:02:53PM -0800, Linus Torvalds wrote:
> >> I appreciate Arnd pointing out "--std=gnu11", though. What are the
> >> actual relevant language improvements?
> >>
> >> Variable declarations in for-loops is the only one I can think of. I
> >> think that would clean up some code (and some macros), but might not
> >> be compelling on its own.
> >
> > Anonymous structs/unions.  I used to have a use case for that in
> > struct efi_dev_path in include/linux/efi.h, but Ard Biesheuvel
> > refactored it in a gnu89-compatible way for v5.7 with db8952e7094f.
> 
> Aren't those a GNU extension supported since GCC 3.0?

They are certainly pretty old.
The 15 year old gcc we use for release builds (so binaries work
on old distributions) supports them.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Linus Torvalds]

On Tue, Jan 12, 2021 at 5:20 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> > Variable declarations in for-loops is the only one I can think of. I
> > think that would clean up some code (and some macros), but might not
> > be compelling on its own.
>
> Anonymous structs/unions.  I used to have a use case for that in
> struct efi_dev_path in include/linux/efi.h, but Ard Biesheuvel
> refactored it in a gnu89-compatible way for v5.7 with db8952e7094f.

We use anonymous structs/unions extensively and all over the place already.

We've had a couple of special cases where some versions of gcc didn't
do things right iirc (it was something like "nested anonymous struct"
or similar), but those weren't actually about the feature itself.

Was it perhaps the nested case you were thinking of? If so, I think
that's not really a --std=gun11 thing, it's more of a "certain
versions of gcc didn't do it at all".

               Linus

Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

[Lukas Wunner]

On Tue, Jan 12, 2021 at 09:28:32AM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2021 at 5:20 AM Lukas Wunner <lukas@wunner.de> wrote:
> > > Variable declarations in for-loops is the only one I can think of. I
> > > think that would clean up some code (and some macros), but might not
> > > be compelling on its own.
> >
> > Anonymous structs/unions.  I used to have a use case for that in
> > struct efi_dev_path in include/linux/efi.h, but Ard Biesheuvel
> > refactored it in a gnu89-compatible way for v5.7 with db8952e7094f.
> 
> We use anonymous structs/unions extensively and all over the place already.

Yes, my apologies, I mixed things up.

Back in 2016 when I authored 46cd4b75cd0e, what I wanted to do was
include an unnamed "struct efi_generic_dev_path;" in struct efi_dev_path:

struct efi_dev_path {
	struct efi_generic_dev_path;
	union {
		struct {
			u32 hid;
			u32 uid;
		} acpi;
		struct {
			u8 fn;
			u8 dev;
		} pci;
	};
} __attribute ((packed));

The alternative is to copy-paste the elements of struct efi_dev_path
or to give it a name such as "header" (which is what db8952e7094f
subsequently did).  Both options seemed inelegant to me.

However it turns out this feature requires -fms-extensions.
It's not part of -std=gnu11.

So coming back to topic, yes there doesn't seem to be too much to
be gained from moving to -std=gnu11 aside from variable declarations
in for-loops.

(And it really has to be -std=gnu11 because -std=c11 fails to compile.)

Thanks,

Lukas