Linux 6.5 compat: UBSAN now complains about flex-array declarations with array[1] #15145
Labels
Type: Defect
Incorrect behavior (e.g. crash, hang)
Comments
satmandu
changed the title
13 tasks
|
This won't be the final rev, but can you try applying the below patches and let me know if it quiets the UBSAN stuff? I'm not 100% on the diff --git a/include/sys/efi_partition.h b/include/sys/efi_partition.h
index c4d7fd508..9fae68ea1 100644
--- a/include/sys/efi_partition.h
+++ b/include/sys/efi_partition.h
@@ -324,7 +324,7 @@ typedef struct dk_gpt {
uint_t efi_reserved1; /* future use - set to zero */
diskaddr_t efi_altern_lba; /* lba of alternate GPT header */
uint_t efi_reserved[12]; /* future use - set to zero */
- struct dk_part efi_parts[1]; /* array of partitions */
+ struct dk_part efi_parts[]; /* array of partitions */
} dk_gpt_t;
/* possible values for "efi_flags" */
diff --git a/include/sys/sa_impl.h b/include/sys/sa_impl.h
index 744c8dcb7..fab7cd9e7 100644
--- a/include/sys/sa_impl.h
+++ b/include/sys/sa_impl.h
@@ -177,7 +177,7 @@ typedef struct sa_hdr_phys {
*
*/
uint16_t sa_layout_info;
- uint16_t sa_lengths[1]; /* optional sizes for variable length attrs */
+ uint16_t sa_lengths[]; /* optional sizes for variable length attrs */
/* ... Data follows the lengths. */
} sa_hdr_phys_t;
diff --git a/include/sys/zap_impl.h b/include/sys/zap_impl.h
index 74853f5fa..bbc105596 100644
--- a/include/sys/zap_impl.h
+++ b/include/sys/zap_impl.h
@@ -61,7 +61,7 @@ typedef struct mzap_phys {
uint64_t mz_salt;
uint64_t mz_normflags;
uint64_t mz_pad[5];
- mzap_ent_phys_t mz_chunk[1];
+ mzap_ent_phys_t mz_chunk[];
/* actually variable size depending on block size */
} mzap_phys_t;
diff --git a/include/sys/zap_leaf.h b/include/sys/zap_leaf.h
index ebc67c2bf..4f35832d6 100644
--- a/include/sys/zap_leaf.h
+++ b/include/sys/zap_leaf.h
@@ -132,7 +132,7 @@ typedef struct zap_leaf_phys {
* with the ZAP_LEAF_CHUNK() macro.
*/
- uint16_t l_hash[1];
+ uint16_t l_hash[];
} zap_leaf_phys_t;
typedef union zap_leaf_chunk { |
|
I will try this today... once my machines aren't tied up being used as buildbots... |
|
Sigh. Reworked the patch because of tabs. :( |
|
Looks like this does eliminate the ubsan errors in dmesg! |
Ok cool - I'll see if I can whip up an @behlendorf do you have any thoughts? Seems to me the current behavior was made to overcome some older compiler limitation that used to not allow variable-length arrays in struct definitions? (probably because it complicates things like |
13 tasks
looks like this patch or fix for this issue was not included in 2.1.13 release |
|
I was hoping this could get into 2.2.0 :) I haven't checked though to see if it has entered the debian patches for their zfs 2.2 release for the Ubuntu 23.10 kernels... |
|
Bit updated patch: changes Looks like |
|
There were some worries mentioned above by @behlendorf about the potential of these changes creating a situation where it could cause data corruption on existing pools, or potentially result in pools that aren't backward compatible, due to that the change needed to silence the UBSAN warning also results in changing the |
|
The core issue is that the syntax that the UBSAN warnings want us to change is actually compliant C code, and designed to function in that manner (overflowing the array at the end of a struct is supposed to be acceptable), so the UBSAN warning is getting over-zealous in these cases. I think the fix should make both things happy, but I don't have an adequate setup to stress test it in the ways it needs stress testing to allay concerns. |
|
Ended up disabling UBSAN in kernel config. Started to complain in syslog about Nvidia drivers too. |
ThomasLamprecht
added a commit
to ThomasLamprecht/zfs
that referenced
this issue
Oct 25, 2023
Use the more common C99 syntax to declare variable length arrays,
while older ones, where a (fake) array length of one was declared,
works too in C, it's making it impossible to distinguish an
out-of-bound access fro a variable array access.
That's why this fixes various "UBSAN: array-index-out-of-bounds"
errors logged on boot with Linux kernel 6.5 or later, since linux
commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC") enabling
stricter bound checks in the UBSAN (Undefined Behavior Sanitizer).
That is actually the main reason for changing this now in the first
place.
The change to modern VLA syntax does not affect the data layout
whatsoever, but can result in a smaller container size as reported by
the `sizeof` operator, from what I can tell this shouldn't be
problematic.
I tried to checked each changed struct for potential breakage, some
short notes (size differences are always for x86_64 with gcc 12, but
as types are consisting of fixed bit width ones it shouldn't matter):
- dk_gpt_t is an internal struct and doesn't seem to be written
directly to disk as is
- sa_hdr_phys_t stays the same size and the call sites where it's
sizeof is used seem resilient of such size changes
- mzap_ent_phys_t shrinks from 128 to 64 bytes, it is not used in
sizeof calculations anywhere
- zap_leaf_phys_t shrinks from 56 to 48 bytes but isn't used in sizeof
calculations.
Link: openzfs#15145
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
ThomasLamprecht
added a commit
to ThomasLamprecht/zfs
that referenced
this issue
Oct 25, 2023
Use the more common C99 syntax to declare variable length arrays,
while older ones, where a (fake) array length of one was declared,
works too in C, it's making it impossible to distinguish an
out-of-bound access fro a variable array access.
That's why this fixes various "UBSAN: array-index-out-of-bounds"
errors logged on boot with Linux kernel 6.5 or later, since linux
commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC") enabling
stricter bound checks in the UBSAN (Undefined Behavior Sanitizer).
That is actually the main reason for changing this now in the first
place.
The change to modern VLA syntax does not affect the data layout
whatsoever, but can result in a smaller container size as reported by
the `sizeof` operator, from what I can tell this shouldn't be
problematic.
I tried to checked each changed struct for potential breakage, some
short notes (size differences are always for x86_64 with gcc 12, but
as types are consisting of fixed bit width ones it shouldn't matter):
- dk_gpt_t is an internal struct and doesn't seem to be written
directly to disk as is
- sa_hdr_phys_t stays the same size and the call sites where it's
sizeof is used seem resilient of such size changes
- mzap_ent_phys_t shrinks from 128 to 64 bytes, it is not used in
sizeof calculations anywhere
- zap_leaf_phys_t shrinks from 56 to 48 bytes but isn't used in sizeof
calculations.
Link: openzfs#15145
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
ThomasLamprecht
added a commit
to ThomasLamprecht/zfs
that referenced
this issue
Oct 25, 2023
Use the more common C99 syntax to declare variable length arrays,
while older ones, where a (fake) array length of one was declared,
works too in C, it's making it impossible to distinguish an
out-of-bound access fro a variable array access.
That's why this fixes various "UBSAN: array-index-out-of-bounds"
errors logged on boot with Linux kernel 6.5 or later, since linux
commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC") enabling
stricter bound checks in the UBSAN (Undefined Behavior Sanitizer).
That is actually the main reason for changing this now in the first
place.
The change to modern VLA syntax does not affect the data layout
whatsoever, but can result in a smaller container size as reported by
the `sizeof` operator, from what I can tell this shouldn't be
problematic.
I tried to checked each changed struct for potential breakage, some
short notes (size differences are always for x86_64 with gcc 12, but
as types are consisting of fixed bit width ones it shouldn't matter):
- dk_gpt_t is an internal struct and doesn't seem to be written
directly to disk as is
- sa_hdr_phys_t stays the same size and the call sites where it's
sizeof is used seem resilient of such size changes
- mzap_ent_phys_t shrinks from 128 to 64 bytes, it is not used in
sizeof calculations anywhere
- zap_leaf_phys_t shrinks from 56 to 48 bytes but isn't used in sizeof
calculations.
Link: openzfs#15145
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
13 tasks
ProxBot
pushed a commit
to proxmox/pve-kernel
that referenced
this issue
Oct 28, 2023
until ZFS can cope with them: openzfs/zfs#15145 Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
ThomasLamprecht
added a commit
to ThomasLamprecht/zfs
that referenced
this issue
Nov 9, 2023
Use the modern, and more common C99 syntax to declare flexible array
members instead of the fixed-size array with a length of
one. While the latter works in C just fine, with some extra handling
for the extra space in length/array-size calculations, it's making it
impossible to distinguish an out-of-bound access fro a variable array
access.
Why now? Linux 6.5 made it's Undefined Behavior Sanitizer (UBSAN)
integration more strict with commit 2d47c6956ab3 ("ubsan: Tighten
UBSAN_BOUNDS on GCC"), causing quite some oopses with "UBSAN:
array-index-out-of-bounds" errors logged on boot when that kernel is
used and UBSAN_BOUNDS check is enabled.
The change to modern VLA syntax does not affect the data layout
whatsoever, but can result in a smaller container size as reported by
the `sizeof` operator, basically the size reported by `sizeof (struct
foo)` will be now the same as the one from `offsetof(struct foo,
flexible_array_member)`.
So we need to carefully check all call-sites that use the changed
struct in sizeof calculations, especially when calculating the full
data length the base-struct plus the data in the flexible array
occupy, because previously one had to bias that by -1 previously, as
the fixed-sized array used to model flexible arrays, provided space
for one in the struct directly.
I tried to checked each changed struct for potential breakage, some
short notes (size differences are always for x86_64 with gcc 12, but
as types are consisting of fixed bit width ones it shouldn't matter):
- dk_gpt_t is an internal struct and doesn't seem to be written
directly to disk as is. But, there are a few places where the length
calculation needs to be adjusted, as struct dk_gpt doesn't holds a
fixed extra element anymore that previously had to be accounted for.
- sa_hdr_phys_t stays the same size due to padding, but the code using
its sizeof value for overall length calculation needs to drop the -1
bias for the length, e.g., when multiplying to get total size.
- mzap_ent_t shrinks from 128 to 64 bytes, it is not used in sizeof
calculations anywhere FWICT.
- zap_leaf_phys_t shrinks from 56 to 48 bytes but isn't used in sizeof
calculations, and the l_hash entry count is calculated explicitly
from the block-size / 2^5 (= 32).
Link: openzfs#15145
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
13 tasks
behlendorf
pushed a commit
that referenced
this issue
Nov 13, 2023
This gets around UBSAN errors when using arrays at the end of structs. It converts some zero-length arrays to variable length arrays and disables UBSAN checking on certain modules. It is based off of the patch from #15460. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Issue #15145 Closes #15510
ixhamza
pushed a commit
to truenas/zfs
that referenced
this issue
Nov 20, 2023
This gets around UBSAN errors when using arrays at the end of structs. It converts some zero-length arrays to variable length arrays and disables UBSAN checking on certain modules. It is based off of the patch from openzfs#15460. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Issue openzfs#15145 Closes openzfs#15510
|
Resolved by #15510 which suppresses the warnings and preserves known correct code. |
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this issue
Dec 12, 2023
This gets around UBSAN errors when using arrays at the end of structs. It converts some zero-length arrays to variable length arrays and disables UBSAN checking on certain modules. It is based off of the patch from openzfs#15460. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Issue openzfs#15145 Closes openzfs#15510
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this issue
Jan 26, 2024
This gets around UBSAN errors when using arrays at the end of structs. It converts some zero-length arrays to variable length arrays and disables UBSAN checking on certain modules. It is based off of the patch from openzfs#15460. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Issue openzfs#15145 Closes openzfs#15510
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this issue
Jan 29, 2024
This gets around UBSAN errors when using arrays at the end of structs. It converts some zero-length arrays to variable length arrays and disables UBSAN checking on certain modules. It is based off of the patch from openzfs#15460. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Issue openzfs#15145 Closes openzfs#15510
behlendorf
pushed a commit
that referenced
this issue
Feb 5, 2024
This gets around UBSAN errors when using arrays at the end of structs. It converts some zero-length arrays to variable length arrays and disables UBSAN checking on certain modules. It is based off of the patch from #15460. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Tested-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Issue #15145 Closes #15510
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See https://www.spinics.net/lists/linux-xfs/msg73588.html & https://patchwork.kernel.org/project/xfs/patch/168934592239.3368057.13821438121542148084.stgit@frogsfrogsfrogs/ for more information on changes which were also made to xfs to fix such issues.
System information
Describe the problem you're observing
Describe how to reproduce the problem
I have a packaged PPA for use with ubuntu 22.10 (Ubuntu Lunar) here: https://launchpad.net/~satadru-umich/+archive/ubuntu/zfs-experimental/
Relevant kernel config options:
Linux Kernel config 6.5.0-rc4
.configused:config.txt
The text was updated successfully, but these errors were encountered: