From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-07 11:20:06
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:
quoted
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!
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-07 12:46:07
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:
quoted
On Wed, Jan 06, 2021 at 05:20:34PM +0000, Will Deacon wrote:
quoted
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!
On Thu, Jan 7, 2021 at 1:47 PM Russell King - ARM Linux admin
[off-list ref] 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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-07 13:38:48
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
[off-list ref] wrote:
quoted
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!
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
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
On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o [off-list ref] wrote:
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-07 22:15:51
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 [off-list ref] wrote:
quoted
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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!
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-07 22:42:20
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:
quoted
On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o [off-list ref] wrote:
quoted
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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
On Thu, 7 Jan 2021 at 23:42, Eric Biggers [off-list ref] wrote:
On Thu, Jan 07, 2021 at 10:14:46PM +0000, Russell King - ARM Linux admin wrote:
quoted
On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
quoted
On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o [off-list ref] wrote:
quoted
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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.
quoted
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.
quoted
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.
quoted
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);
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-07 22:28:35
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 [off-list ref] wrote:
quoted
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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
From: Darrick J. Wong <hidden> Date: 2021-01-07 23:54:52
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:
quoted
On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o [off-list ref] wrote:
quoted
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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
On Fri, Jan 8, 2021 at 12:53 AM Darrick J. Wong [off-list ref] wrote:
On Thu, Jan 07, 2021 at 02:27:51PM -0800, Eric Biggers wrote:
quoted
On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
quoted
On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o [off-list ref] wrote:
quoted
On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
quoted
quoted
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().
quoted
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.
quoted
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
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-01-08 09:14:37
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.
From: Pavel Machek <hidden> Date: 2021-01-08 10:32:31
Hi!
quoted
quoted
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.
On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
[off-list ref] wrote:
On Thu, Jan 07, 2021 at 02:16:25PM +0100, Arnd Bergmann wrote:
quoted
On Thu, Jan 7, 2021 at 1:47 PM Russell King - ARM Linux admin
[off-list ref] wrote:
quoted
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
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-01-08 09:23:05
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
quoted
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.
From: Will Deacon <will@kernel.org> Date: 2021-01-08 09:27:45
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:
quoted
On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
quoted
quoted
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
On Fri, Jan 8, 2021 at 1:27 AM Will Deacon [off-list ref] wrote:
On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
quoted
On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
quoted
On Thu, Jan 7, 2021 at 2:37 PM Russell King - ARM Linux admin
quoted
quoted
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
On Fri, Jan 8, 2021 at 9:02 PM Linus Torvalds
[off-list ref] wrote:
On Fri, Jan 8, 2021 at 1:27 AM Will Deacon [off-list ref] wrote:
quoted
On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
quoted
On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
quoted
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
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2021-01-08 21:21:36
On Fri, Jan 8, 2021 at 12:34 PM Arnd Bergmann [off-list ref] wrote:
On Fri, Jan 8, 2021 at 9:02 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Fri, Jan 8, 2021 at 1:27 AM Will Deacon [off-list ref] wrote:
quoted
On Fri, Jan 08, 2021 at 10:21:54AM +0100, Peter Zijlstra wrote:
quoted
On Thu, Jan 07, 2021 at 10:20:38PM +0100, Arnd Bergmann wrote:
quoted
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-08 20:30:34
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!
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
On Tue, Jan 12, 2021 at 5:20 AM Lukas Wunner [off-list ref] wrote:
quoted
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
On Tue, Jan 12, 2021 at 09:28:32AM -0800, Linus Torvalds wrote:
On Tue, Jan 12, 2021 at 5:20 AM Lukas Wunner [off-list ref] wrote:
quoted
quoted
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