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 12:45:06 +0000	[thread overview]
Message-ID: <20210107124506.GO1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210107111841.GN1551@shell.armlinux.org.uk>

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!

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 12:45:06 +0000	[thread overview]
Message-ID: <20210107124506.GO1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210107111841.GN1551@shell.armlinux.org.uk>

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!

_______________________________________________
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 12:46 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
2021-01-07 11:18           ` Russell King - ARM Linux admin
2021-01-07 12:45           ` Russell King - ARM Linux admin [this message]
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=20210107124506.GO1551@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.