From: Arnd Bergmann <arnd@kernel.org> To: "Darrick J. Wong" <darrick.wong@oracle.com> Cc: Eric Biggers <ebiggers@kernel.org>, "Theodore Ts'o" <tytso@mit.edu>, Russell King - ARM Linux admin <linux@armlinux.org.uk>, Will Deacon <will@kernel.org>, linux-toolchains@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Andreas Dilger <adilger.kernel@dilger.ca>, Ext4 Developers List <linux-ext4@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org> Subject: Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues Date: Fri, 8 Jan 2021 09:05:05 +0100 [thread overview] Message-ID: <CAK8P3a24jhd13FrQsvt9kryRwn6p4bVxCn3LtGBUhO8a-q75iQ@mail.gmail.com> (raw) In-Reply-To: <20210107235328.GI6908@magnolia> 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
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@kernel.org> To: "Darrick J. Wong" <darrick.wong@oracle.com> Cc: Mark Rutland <mark.rutland@arm.com>, Theodore Ts'o <tytso@mit.edu>, Russell King - ARM Linux admin <linux@armlinux.org.uk>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Eric Biggers <ebiggers@kernel.org>, Andreas Dilger <adilger.kernel@dilger.ca>, linux-toolchains@vger.kernel.org, Ext4 Developers List <linux-ext4@vger.kernel.org>, Will Deacon <will@kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org> Subject: Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues Date: Fri, 8 Jan 2021 09:05:05 +0100 [thread overview] Message-ID: <CAK8P3a24jhd13FrQsvt9kryRwn6p4bVxCn3LtGBUhO8a-q75iQ@mail.gmail.com> (raw) In-Reply-To: <20210107235328.GI6908@magnolia> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-01-08 8:06 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-05 15:47 Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues 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 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 [this message] 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=CAK8P3a24jhd13FrQsvt9kryRwn6p4bVxCn3LtGBUhO8a-q75iQ@mail.gmail.com \ --to=arnd@kernel.org \ --cc=adilger.kernel@dilger.ca \ --cc=darrick.wong@oracle.com \ --cc=ebiggers@kernel.org \ --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=linux@armlinux.org.uk \ --cc=mark.rutland@arm.com \ --cc=tytso@mit.edu \ --cc=will@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.