ext4: fix ext4_tune_sb_params padding
PATCH 1/1 ext4: fix ext4_tune_sb_params padding
From: Arnd Bergmann <[email protected]>
The padding at the end of struct ext4_tune_sb_params is architecture specific and in particular is different between x86-32 and x86-64, since the __u64 member only enforces struct alignment on the latter.
This shows up as a new warning when test-building the headers with -Wpadded:
include/linux/ext4.h:144:1: error: padding struct size to alignment boundary with 4 bytes [-Werror=padded]
All members inside the structure are naturally aligned, so the only difference here is the amount of padding at the end. Make the padding explicit, to have a consistent sizeof(struct ext4_tune_sb_params) of 232 on all architectures and avoid adding compat ioctl handling for EXT4_IOC_GET_TUNE_SB_PARAM/EXT4_IOC_SET_TUNE_SB_PARAM.
This is an ABI break on x86-32 but hopefully this can go into 6.18.y early enough as a fixup so no actual users will be affected. Alternatively, the kernel could handle the ioctl commands for both sizes (232 and 228 bytes) on all architectures.
--- include/uapi/linux/ext4.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/ext4.h +1 -1
@@ -139,7 +139,7 @@ struct ext4_tune_sb_params {__u32 clear_feature_incompat_mask;__u32 clear_feature_ro_compat_mask;__u8 mount_opts[64 ]; -__u8 pad[64 ]; +__u8 pad[68 ]; };#define EXT4_TUNE_FL_ERRORS_BEHAVIOR 0x00000001
Signature · 1 line
2.39.5
Quoted 17 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
The padding at the end of struct
ext4_tune_sb_paramsis architecture specific and in particular is different between x86-32 and x86-64, since the__u64member only enforces struct alignment on the latter.This shows up as a new warning when test-building the headers with -Wpadded:
include/linux/ext4.h:144:1: error: padding struct size to alignment boundary with 4 bytes [-Werror=padded]All members inside the structure are naturally aligned, so the only difference here is the amount of padding at the end. Make the padding explicit, to have a consistent
sizeof(struct ext4_tune_sb_params)of 232 on all architectures and avoid adding compat ioctl handling for EXT4_IOC_GET_TUNE_SB_PARAM/EXT4_IOC_SET_TUNE_SB_PARAM.
Quoted 7 lines
This is an ABI break on x86-32 but hopefully this can go into 6.18.y early enough as a fixup so no actual users will be affected. Alternatively, the kernel could handle the ioctl commands for both sizes (232 and 228 bytes) on all architectures.
Fixes 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")Signed-off-by Arnd Bergmann <[email protected]>
Indeed. I agree this is fairly new so we can just fix the structure. Feel free to add:
Honza
Quoted 20 lines
--- include/uapi/linux/ext4.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/uapi/linux/ext4.h +1
-1@@ -139,7 +139,7 @@ struct ext4_tune_sb_params {__u32 clear_feature_incompat_mask;__u32 clear_feature_ro_compat_mask;__u8 mount_opts[64 ]; -__u8 pad[64 ]; +__u8 pad[68 ]; };#define EXT4_TUNE_FL_ERRORS_BEHAVIOR 0x00000001Signature · 1 line
2.39.5
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 27 lines · Jan Kara
Quoted 22 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
The padding at the end of struct
ext4_tune_sb_paramsis architecture specific and in particular is different between x86-32 and x86-64, since the__u64member only enforces struct alignment on the latter.This shows up as a new warning when test-building the headers with -Wpadded:
include/linux/ext4.h:144:1: error: padding struct size to alignment boundary with 4 bytes [-Werror=padded]All members inside the structure are naturally aligned, so the only difference here is the amount of padding at the end. Make the padding explicit, to have a consistent
sizeof(struct ext4_tune_sb_params)of 232 on all architectures and avoid adding compat ioctl handling for EXT4_IOC_GET_TUNE_SB_PARAM/EXT4_IOC_SET_TUNE_SB_PARAM.This is an ABI break on x86-32 but hopefully this can go into 6.18.y early enough as a fixup so no actual users will be affected. Alternatively, the kernel could handle the ioctl commands for both sizes (232 and 228 bytes) on all architectures.
Quoted 7 lines
Quoted 2 lines
Fixes 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")Signed-off-by Arnd Bergmann <[email protected]>Indeed. I agree this is fairly new so we can just fix the structure. Feel free to add:
Reviewed-by Jan Kara <[email protected]>
While this change isn't _wrong_ per-se, it does seem very strange to have a 68-byte padding at the end of the struct. You have to check the number of __u32 fields closely to see this, and I wonder if this will perpetuate errors in the future (e.g. adding a __u64 field after mount_opts[64]).
IMHO, it would be more clear to either add an explicit "__u32 pad_3;" field after mount_opts[64], or alternately declare mount_opts[68] so it will consume those bytes and leave the remaining fields properly aligned. It isn't critical if the user tools use the last 4 bytes of mount_opts[] or not, so they could be changed independently at some later time.
Either will ensure that new fields added in place of pad[64] will be properly aligned in the future.
Cheers, Andreas
Quoted 19 lines
Honza
Quoted 16 lines
---
include/uapi/linux/ext4.h| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)include/uapi/linux/ext4.h +1
-1@@ -139,7 +139,7 @@ struct ext4_tune_sb_params {__u32 clear_feature_incompat_mask;__u32 clear_feature_ro_compat_mask;__u8 mount_opts[64 ]; -__u8 pad[64 ]; +__u8 pad[68 ]; };
Quoted 7 lines
Quoted 4 lines
include/uapi/linux/ext4.h +0
-0#define EXT4_TUNE_FL_ERRORS_BEHAVIOR 0x00000001Signature · 1 line
2.39.5Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Cheers, Andreas
Quoted 6 lines · Andreas Dilger
Quoted 2 lines · Jan Kara
On Thu 04-12-25 11:19:10, Arnd Bergmann wrote:
While this change isn't _wrong_ per-se, it does seem very strange to have a 68-byte padding at the end of the struct. You have to check the number of
__u32fields closely to see this,
I had the same thought but decided against that because it would be an ABI break on all architectures. The version I posted only changes the structure size on x86-32, csky, m68k and microblaze, as far as I can tell.
Quoted 2 lines
and I wonder if this will perpetuate
errors in the future (e.g. adding a__u64field after mount_opts[64]).
Indeed, I can see how that could become worse.
Quoted 8 lines
IMHO, it would be more clear to either add an explicit "
__u32pad_3;" field after mount_opts[64], or alternately declare mount_opts[68] so it will consume those bytes and leave the remaining fields properly aligned. It isn't critical if the user tools use the last 4 bytes of mount_opts[] or not, so they could be changed independently at some later time.Either will ensure that new fields added in place of pad[64] will be properly aligned in the future.
Changing mount_opts[] to 68 bytes sounds fine to me, I'll send an updated patch for that. I've kept the Ack from Jan, please shout if I should drop that instead.
Arnd
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Andreas Dilger
Quoted 2 lines · Jan Kara
On Thu 04-12-25 11:19:10, Arnd Bergmann wrote:
While this change isn't _wrong_ per-se, it does seem very strange to have a 68-byte padding at the end of the struct. You have to check the number of
__u32fields closely to see this,I had the same thought but decided against that because it would be an ABI break on all architectures. The version I posted only changes the structure size on x86-32, csky, m68k and microblaze, as far as I can tell.
Indeed very unfortunate...
Quoted 7 lines
Quoted 2 lines
and I wonder if this will perpetuate
errors in the future (e.g. adding a__u64field after mount_opts[64]).Indeed, I can see how that could become worse.
Quoted 2 lines
IMHO, it would be more clear to either add an explicit "
__u32pad_3;" field after mount_opts[64], or alternately declare mount_opts[68] so it
FTR, I would have added "__u32 pad_3;" _before_ mount_opts[64].
Quoted 10 lines
Quoted 6 lines
will consume those bytes and leave the remaining fields properly aligned. It isn't critical if the user tools use the last 4 bytes of mount_opts[] or not, so they could be changed independently at some later time.
Either will ensure that new fields added in place of pad[64] will be properly aligned in the future.
Changing mount_opts[] to 68 bytes sounds fine to me, I'll send an updated patch for that. I've kept the Ack from Jan, please shout if I should drop that instead.
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Quoted 5 lines · On Thu, 4 Dec 2025 11:19:10 +0100 Arnd Bergmann <[email protected]> wrote:
From: Arnd Bergmann <[email protected]>
The padding at the end of struct
ext4_tune_sb_paramsis architecture specific and in particular is different between x86-32 and x86-64, since the__u64member only enforces struct alignment on the latter.
Is it worth adding a compile-time check for the size somewhere? Since the intention seems to be that any extensions will use the padding.
David
Quoted 36 lines
This shows up as a new warning when test-building the headers with -Wpadded:
include/linux/ext4.h:144:1: error: padding struct size to alignment boundary with 4 bytes [-Werror=padded]All members inside the structure are naturally aligned, so the only difference here is the amount of padding at the end. Make the padding explicit, to have a consistent
sizeof(struct ext4_tune_sb_params)of 232 on all architectures and avoid adding compat ioctl handling for EXT4_IOC_GET_TUNE_SB_PARAM/EXT4_IOC_SET_TUNE_SB_PARAM.This is an ABI break on x86-32 but hopefully this can go into 6.18.y early enough as a fixup so no actual users will be affected. Alternatively, the kernel could handle the ioctl commands for both sizes (232 and 228 bytes) on all architectures.
Fixes 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")Signed-off-by Arnd Bergmann <[email protected]>--- include/uapi/linux/ext4.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/uapi/linux/ext4.h +1
-1@@ -139,7 +139,7 @@ struct ext4_tune_sb_params {__u32 clear_feature_incompat_mask;__u32 clear_feature_ro_compat_mask;__u8 mount_opts[64 ]; -__u8 pad[64 ]; +__u8 pad[68 ]; };#define EXT4_TUNE_FL_ERRORS_BEHAVIOR 0x00000001
Quoted 11 lines · David Laight
Quoted 5 lines · On Thu, 4 Dec 2025 11:19:10 +0100 Arnd Bergmann <[email protected]> wrote:
From: Arnd Bergmann <[email protected]>
The padding at the end of struct
ext4_tune_sb_paramsis architecture specific and in particular is different between x86-32 and x86-64, since the__u64member only enforces struct alignment on the latter.Is it worth adding a compile-time check for the size somewhere? Since the intention seems to be that any extensions will use the padding.
There is already ABI checking with abigail that ensures that struct members and sizes don't change in the future, which I think covers that. I would also like to push my series to enable -Werror=padded in the header checks, but I'm not sure yet what others think of the idea.
Arnd
Quoted 10 lines · On Thu, 04 Dec 2025 14:42:06 +0100 "Arnd Bergmann" <[email protected]> wrote:
Quoted 9 lines · David Laight
Quoted 5 lines · On Thu, 4 Dec 2025 11:19:10 +0100 Arnd Bergmann <[email protected]> wrote:
From: Arnd Bergmann <[email protected]>
The padding at the end of struct
ext4_tune_sb_paramsis architecture specific and in particular is different between x86-32 and x86-64, since the__u64member only enforces struct alignment on the latter.
Quoted 8 lines
Quoted 2 lines
Is it worth adding a compile-time check for the size somewhere? Since the intention seems to be that any extensions will use the padding.
There is already ABI checking with abigail that ensures that struct members and sizes don't change in the future, which I think covers that. I would also like to push my series to enable -Werror=padded in the header checks, but I'm not sure yet what others think of the idea.
Putting it in the command line is going to be griefsome (at least in the short term) even for uapi headers - where you really don't want padding. (Tell that to some of the standards bodies...)
It is a shame there isn't an attribute, but you can wrap definitions:
#define check_padding(...) _Pragma("GCC diagnostic push"); \
_Pragma("GCC diagnostic error \"-Wpadded\""); \
__VA_ARGS__ \
_Pragma("GCC diagnostic pop");check_padding(
typedef struct fubar {
int a;
char b;
} fred;) /* check_padding */
I've thought about doing something similar to avoid the 'type-limits' check inside statically_true() and the like for W=1 builds.
David
Quoted 8 lines · Arnd Bergmann
The padding at the end of struct
ext4_tune_sb_paramsis architecture specific and in particular is different between x86-32 and x86-64, since the__u64member only enforces struct alignment on the latter.This shows up as a new warning when test-building the headers with -Wpadded:
[...]
Applied, thanks!
[1/1] ext4: fix ext4_tune_sb_params padding
(no commit info)
Best regards,
Signature · 1 line
Theodore Ts'o <[email protected]>