ZFS doesn't work with CONFIG_INIT_STACK_ALL_PATTERN #16135
Labels
Type: Defect
Incorrect behavior (e.g. crash, hang)
Comments
|
I compiled 6.1.83 with So, more info required. Can you describe the steps you take in detail, and post the kernel config, pool topology and pool and dataset properties, and I can try to reproduce. When it hangs, do you have a working root shell? If so, it'd be good to inspect the system and find out exactly where the hang is. |
|
thanks for the answer! and sorry for the delay, it'll take a few more days until I can provide you with the info - but I will :) |
|
So here we go. Attached is my kernel config: config-6.7.12-nullpattern I have two computers where I can reproduce this - just take the current config and enable |
|
Seems the secret detail was that it was compiled with Clang. I did a clean build of both 6.7.12 based on your config, and then OpenZFS against it. With GCC 12, all is well. With Clang 18, explosion: It's a good trace, and likely pretty easy to track down the issue. I'm out of time tonight, so it'll have to wait until tomorrow. I'll see if I can get a test suite run against an all-Clang build as well, might shake a bit more out. |
|
perfect! That sounds great. |
|
Skimming over the mostly familiar code I'd say it chokes on
https://github.com/openzfs/zfs/blob/e675852bc1d50404fcbe4fa0e1f57b6c318e6349/module/icp/algs/modes/modes.c#L189-L192
The gcm code needs the `gcm_ctx_t` to be zeroed to work properly. This
is assured in the common code path by using `vmem_zalloc()`.
`aes_encrypt_atomic()` is the only consumer which allocates the
`gcm_ctx_t` on the stack:
https://github.com/openzfs/zfs/blob/e675852bc1d50404fcbe4fa0e1f57b6c318e6349/module/icp/io/aes.c#L835
So my guess would be that Clang explicitly ignores zero initialization
of stack variables if `CONFIG_INIT_STACK_ALL_PATTERN` is defined. We're
encrypting so `ctx->gcm_pt_buf` (buffer to hold decrypted plaintext)
must be `NULL`.
AFAIK `CONFIG_INIT_STACK_ALL_PATTERN` is a Clang specific setting. That
would explain why GCC behaves correctly.
|
|
@AttilaFueloep thanks for those pointers. I dug the rest of the way, and there's more bonkers than that! tl;dr GCC and Clang treat So, even though both compilers support (it's a bit beyond my ken, but it appears to be a difference of opinion between GCC and Clang on whether or not a union is an aggregate, as the initialiser rules are applied to aggregates). More info:
And some proof: I will do something soon. Simply zeroing the entire thing with |
|
Alright, I've done the read, and I think robn/zfs@4b4495d "fixes" this by changing all the places where I've put this on the test rig with a 6.7.12, all compiled with Clang 18. It passed the initial check of creating an encrypted dataset without crashing or hanging. I've left it running the full test suite overnight. If it looks clean, I'll then go and look for prior art on how to do this properly. I want it to be hard/impossible to screw up, which means either a compiler warning or a lint of some kind to draw attention to it, and a clear pattern for "the right way". |
|
Test suite passed, which is a pleasing indicator of something. |
This was referenced May 17, 2024
behlendorf
pushed a commit
that referenced
this issue
May 25, 2024
C99 6.7.8.17 says that when an undesignated initialiser is used, only the first element of a union is initialised. If the first element is not the largest within the union, how the remaining space is initialised is up to the compiler. GCC extends the initialiser to the entire union, while Clang treats the remainder as padding, and so initialises according to whatever automatic/implicit initialisation rules are currently active. When Linux is compiled with CONFIG_INIT_STACK_ALL_PATTERN, -ftrivial-auto-var-init=pattern is added to the kernel CFLAGS. This flag sets the policy for automatic/implicit initialisation of variables on the stack. Taken together, this means that when compiling under CONFIG_INIT_STACK_ALL_PATTERN on Clang, the "zero" initialiser will only zero the first element in a union, and the rest will be filled with a pattern. This is significant for aes_ctx_t, which in aes_encrypt_atomic() and aes_decrypt_atomic() is initialised to zero, but then used as a gcm_ctx_t, which is the fifth element in the union, and thus gets pattern initialisation. Later, it's assumed to be zero, resulting in a hang. As confusing and undiscoverable as it is, by the spec, we are at fault when we initialise a structure containing a union with the zero initializer. As such, this commit replaces these uses with an explicit memset(0). Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes #16135 Closes #16206
robn
added a commit
to robn/zfs
that referenced
this issue
Jul 17, 2024
C99 6.7.8.17 says that when an undesignated initialiser is used, only the first element of a union is initialised. If the first element is not the largest within the union, how the remaining space is initialised is up to the compiler. GCC extends the initialiser to the entire union, while Clang treats the remainder as padding, and so initialises according to whatever automatic/implicit initialisation rules are currently active. When Linux is compiled with CONFIG_INIT_STACK_ALL_PATTERN, -ftrivial-auto-var-init=pattern is added to the kernel CFLAGS. This flag sets the policy for automatic/implicit initialisation of variables on the stack. Taken together, this means that when compiling under CONFIG_INIT_STACK_ALL_PATTERN on Clang, the "zero" initialiser will only zero the first element in a union, and the rest will be filled with a pattern. This is significant for aes_ctx_t, which in aes_encrypt_atomic() and aes_decrypt_atomic() is initialised to zero, but then used as a gcm_ctx_t, which is the fifth element in the union, and thus gets pattern initialisation. Later, it's assumed to be zero, resulting in a hang. As confusing and undiscoverable as it is, by the spec, we are at fault when we initialise a structure containing a union with the zero initializer. As such, this commit replaces these uses with an explicit memset(0). Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16135 Closes openzfs#16206 (cherry picked from commit d0aa9db)
robn
added a commit
to robn/zfs
that referenced
this issue
Jul 17, 2024
C99 6.7.8.17 says that when an undesignated initialiser is used, only the first element of a union is initialised. If the first element is not the largest within the union, how the remaining space is initialised is up to the compiler. GCC extends the initialiser to the entire union, while Clang treats the remainder as padding, and so initialises according to whatever automatic/implicit initialisation rules are currently active. When Linux is compiled with CONFIG_INIT_STACK_ALL_PATTERN, -ftrivial-auto-var-init=pattern is added to the kernel CFLAGS. This flag sets the policy for automatic/implicit initialisation of variables on the stack. Taken together, this means that when compiling under CONFIG_INIT_STACK_ALL_PATTERN on Clang, the "zero" initialiser will only zero the first element in a union, and the rest will be filled with a pattern. This is significant for aes_ctx_t, which in aes_encrypt_atomic() and aes_decrypt_atomic() is initialised to zero, but then used as a gcm_ctx_t, which is the fifth element in the union, and thus gets pattern initialisation. Later, it's assumed to be zero, resulting in a hang. As confusing and undiscoverable as it is, by the spec, we are at fault when we initialise a structure containing a union with the zero initializer. As such, this commit replaces these uses with an explicit memset(0). Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16135 Closes openzfs#16206 (cherry picked from commit d0aa9db)
robn
added a commit
to robn/zfs
that referenced
this issue
Jul 17, 2024
C99 6.7.8.17 says that when an undesignated initialiser is used, only the first element of a union is initialised. If the first element is not the largest within the union, how the remaining space is initialised is up to the compiler. GCC extends the initialiser to the entire union, while Clang treats the remainder as padding, and so initialises according to whatever automatic/implicit initialisation rules are currently active. When Linux is compiled with CONFIG_INIT_STACK_ALL_PATTERN, -ftrivial-auto-var-init=pattern is added to the kernel CFLAGS. This flag sets the policy for automatic/implicit initialisation of variables on the stack. Taken together, this means that when compiling under CONFIG_INIT_STACK_ALL_PATTERN on Clang, the "zero" initialiser will only zero the first element in a union, and the rest will be filled with a pattern. This is significant for aes_ctx_t, which in aes_encrypt_atomic() and aes_decrypt_atomic() is initialised to zero, but then used as a gcm_ctx_t, which is the fifth element in the union, and thus gets pattern initialisation. Later, it's assumed to be zero, resulting in a hang. As confusing and undiscoverable as it is, by the spec, we are at fault when we initialise a structure containing a union with the zero initializer. As such, this commit replaces these uses with an explicit memset(0). Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16135 Closes openzfs#16206 (cherry picked from commit d0aa9db)
robn
added a commit
to robn/zfs
that referenced
this issue
Jul 18, 2024
C99 6.7.8.17 says that when an undesignated initialiser is used, only the first element of a union is initialised. If the first element is not the largest within the union, how the remaining space is initialised is up to the compiler. GCC extends the initialiser to the entire union, while Clang treats the remainder as padding, and so initialises according to whatever automatic/implicit initialisation rules are currently active. When Linux is compiled with CONFIG_INIT_STACK_ALL_PATTERN, -ftrivial-auto-var-init=pattern is added to the kernel CFLAGS. This flag sets the policy for automatic/implicit initialisation of variables on the stack. Taken together, this means that when compiling under CONFIG_INIT_STACK_ALL_PATTERN on Clang, the "zero" initialiser will only zero the first element in a union, and the rest will be filled with a pattern. This is significant for aes_ctx_t, which in aes_encrypt_atomic() and aes_decrypt_atomic() is initialised to zero, but then used as a gcm_ctx_t, which is the fifth element in the union, and thus gets pattern initialisation. Later, it's assumed to be zero, resulting in a hang. As confusing and undiscoverable as it is, by the spec, we are at fault when we initialise a structure containing a union with the zero initializer. As such, this commit replaces these uses with an explicit memset(0). Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16135 Closes openzfs#16206
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this issue
Sep 4, 2024
C99 6.7.8.17 says that when an undesignated initialiser is used, only the first element of a union is initialised. If the first element is not the largest within the union, how the remaining space is initialised is up to the compiler. GCC extends the initialiser to the entire union, while Clang treats the remainder as padding, and so initialises according to whatever automatic/implicit initialisation rules are currently active. When Linux is compiled with CONFIG_INIT_STACK_ALL_PATTERN, -ftrivial-auto-var-init=pattern is added to the kernel CFLAGS. This flag sets the policy for automatic/implicit initialisation of variables on the stack. Taken together, this means that when compiling under CONFIG_INIT_STACK_ALL_PATTERN on Clang, the "zero" initialiser will only zero the first element in a union, and the rest will be filled with a pattern. This is significant for aes_ctx_t, which in aes_encrypt_atomic() and aes_decrypt_atomic() is initialised to zero, but then used as a gcm_ctx_t, which is the fifth element in the union, and thus gets pattern initialisation. Later, it's assumed to be zero, resulting in a hang. As confusing and undiscoverable as it is, by the spec, we are at fault when we initialise a structure containing a union with the zero initializer. As such, this commit replaces these uses with an explicit memset(0). Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#16135 Closes openzfs#16206
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
System information
Describe the problem you're observing
I am using ZFS on Root with zfs native encryption. If I compile the kernel with
CONFIG_INIT_STACK_ALL_PATTERN, the zfs modules get loaded and I can enter the passphrase, but then everything just hangs without any further messages. Everything works as expected when usingCONFIG_INIT_STACK_ALL_ZERO. So I would assume there is an uninitialized variable somewhere in the zfs/spl code.The text was updated successfully, but these errors were encountered: