All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Will Deacon <will@kernel.org>, linux-toolchains@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>,
	Theodore Ts'o <tytso@mit.edu>,
	linux-kernel@vger.kernel.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	linux-ext4@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues
Date: Thu, 7 Jan 2021 11:18:41 +0000	[thread overview]
Message-ID: <20210107111841.GN1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210106223223.GM1551@shell.armlinux.org.uk>

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!

WARNING: multiple messages have this Message-ID
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Will Deacon <will@kernel.org>, linux-toolchains@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>,
	Theodore Ts'o <tytso@mit.edu>,
	linux-kernel@vger.kernel.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	linux-ext4@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues
Date: Thu, 7 Jan 2021 11:18:41 +0000	[thread overview]
Message-ID: <20210107111841.GN1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210106223223.GM1551@shell.armlinux.org.uk>

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-01-07 11:20 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-05 15:47 Russell King - ARM Linux admin
2021-01-05 15:47 ` Russell King - ARM Linux admin
2021-01-05 18:27 ` Darrick J. Wong
2021-01-05 18:27   ` Darrick J. Wong
2021-01-05 19:50   ` Russell King - ARM Linux admin
2021-01-05 19:50     ` Russell King - ARM Linux admin
2021-01-06 11:53 ` Mark Rutland
2021-01-06 11:53   ` Mark Rutland
2021-01-06 12:13   ` Russell King - ARM Linux admin
2021-01-06 12:13     ` Russell King - ARM Linux admin
2021-01-06 13:52   ` Russell King - ARM Linux admin
2021-01-06 17:20     ` Will Deacon
2021-01-06 17:20       ` Will Deacon
2021-01-06 17:46       ` Russell King - ARM Linux admin
2021-01-06 17:46         ` Russell King - ARM Linux admin
2021-01-06 21:04       ` Arnd Bergmann
2021-01-06 21:04         ` Arnd Bergmann
2021-01-06 22:00         ` Arnd Bergmann
2021-01-06 22:00           ` Arnd Bergmann
2021-01-06 22:32       ` Russell King - ARM Linux admin
2021-01-06 22:32         ` Russell King - ARM Linux admin
2021-01-07 11:18         ` Russell King - ARM Linux admin [this message]
2021-01-07 11:18           ` Russell King - ARM Linux admin
2021-01-07 12:45           ` Russell King - ARM Linux admin
2021-01-07 12:45             ` Russell King - ARM Linux admin
2021-01-07 13:16             ` Arnd Bergmann
2021-01-07 13:16               ` Arnd Bergmann
2021-01-07 13:37               ` Russell King - ARM Linux admin
2021-01-07 13:37                 ` Russell King - ARM Linux admin
2021-01-07 16:27                 ` Theodore Ts'o
2021-01-07 16:27                   ` Theodore Ts'o
2021-01-07 17:00                   ` Florian Weimer
2021-01-07 17:00                     ` Florian Weimer
2021-01-07 21:48                   ` Arnd Bergmann
2021-01-07 21:48                     ` Arnd Bergmann
2021-01-07 22:14                     ` Russell King - ARM Linux admin
2021-01-07 22:14                       ` Russell King - ARM Linux admin
2021-01-07 22:41                       ` Eric Biggers
2021-01-07 22:41                         ` Eric Biggers
2021-01-08  8:21                         ` Ard Biesheuvel
2021-01-08  8:21                           ` Ard Biesheuvel
2021-01-07 22:27                     ` Eric Biggers
2021-01-07 22:27                       ` Eric Biggers
2021-01-07 23:53                       ` Darrick J. Wong
2021-01-07 23:53                         ` Darrick J. Wong
2021-01-08  8:05                         ` Arnd Bergmann
2021-01-08  8:05                           ` Arnd Bergmann
2021-01-08  9:13                   ` Peter Zijlstra
2021-01-08  9:13                     ` Peter Zijlstra
2021-01-08 10:31                   ` Pavel Machek
2021-01-08 10:31                     ` Pavel Machek
2021-01-07 21:20                 ` Arnd Bergmann
2021-01-07 21:20                   ` Arnd Bergmann
2021-01-08  9:21                   ` Peter Zijlstra
2021-01-08  9:21                     ` Peter Zijlstra
2021-01-08  9:26                     ` Will Deacon
2021-01-08  9:26                       ` Will Deacon
2021-01-08 20:02                       ` Linus Torvalds
2021-01-08 20:02                         ` Linus Torvalds
2021-01-08 20:22                         ` Arnd Bergmann
2021-01-08 20:22                           ` Arnd Bergmann
2021-01-08 21:20                           ` Nick Desaulniers
2021-01-08 21:20                             ` Nick Desaulniers
2021-01-08 20:29                         ` Russell King - ARM Linux admin
2021-01-08 20:29                           ` Russell King - ARM Linux admin
2021-01-12 13:20                         ` Lukas Wunner
2021-01-12 13:31                           ` Florian Weimer
2021-01-12 13:31                             ` Florian Weimer
2021-01-12 13:46                             ` David Laight
2021-01-12 13:46                               ` David Laight
2021-01-12 17:28                           ` Linus Torvalds
2021-01-12 17:28                             ` Linus Torvalds
2021-01-14 13:13                             ` Lukas Wunner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210107111841.GN1551@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=adilger.kernel@dilger.ca \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-toolchains@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=tytso@mit.edu \
    --cc=will@kernel.org \
    --subject='Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.