[Added some Cc's, and updated subject to reflect what this is really about]
On Tue, Jan 17, 2023 at 05:10:55PM -0700, Andreas Dilger wrote:
> On Jan 17, 2023, at 11:31 AM, Eric Whitney <enwlinux@xxxxxxxxx> wrote:
> >
> > My 6.2-rc1 regression run on the current x86-64 test appliance revealed a new
> > failure for generic/454 on the 4k file system configuration and all other
> > configurations using a 4k block size. This failure reproduces with 100%
> > reliability and continues to appear as of 6.2-rc4.
> >
> > The test output indicates that the file system under test is inconsistent.
>
> There is actually support in the superblock for both signed and unsigned char
> hash calculations, exactly because there was a bug like this in the past.
> It looks like the ext4 code/build is still using the signed hash functions:
>
>
> static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> {
> :
> :
> if (i & EXT2_FLAGS_UNSIGNED_HASH)
> sbi->s_hash_unsigned = 3;
> else if ((i & EXT2_FLAGS_SIGNED_HASH) == 0) {
> #ifdef __CHAR_UNSIGNED__
> if (!sb_rdonly(sb))
> es->s_flags |=
> cpu_to_le32(EXT2_FLAGS_UNSIGNED_HASH);
> sbi->s_hash_unsigned = 3;
> #else
> if (!sb_rdonly(sb))
> es->s_flags |=
> cpu_to_le32(EXT2_FLAGS_SIGNED_HASH);
> #endif
> }
>
> It looks like this *should* be detecting the unsigned/signed char type
> automatically based on __CHAR_UNSIGNED__, but that isn't working properly
> in this case. I have no idea whether this is a compiler or kernel issue,
> just thought I'd point out the background of what ext4 is doing here.
>
> Cheers, Andreas
Well, since v6.2-rc1 the kernel is always compiled with -funsigned-char, so of
course the above no longer works to detect the "default" signedness of a char.
Below is one very ugly solution. It seems to work, based on the output of
'make V=1'; fs/ext4/char.c is compiled *without* -funsigned-char, and everything
else is still compiled with -funsigned-char. Though, I'm not sure that the
trick I'm using with KBUILD_CFLAGS is meant to be supported.
Better ideas would be appreciated. If the default signedness of 'char' is a
per-arch thing, maybe each arch could explicitly select
ARCH_HAVE_DEFAULT_SIGNED_CHAR or ARCH_HAVE_DEFAULT_UNSIGNED_CHAR? Or is there
any chance that this code is obsolete and can be removed from ext4?
>From 87b77d02c399d684d906832862ad234ec321ff12 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@xxxxxxxxxx>
Date: Tue, 17 Jan 2023 19:21:35 -0800
Subject: [PATCH] ext4: fix detection of default char signedness
For strange reasons involving a historical bug in ext4's on-disk format,
ext4 needs to know the default signedness of a char. Since the kernel
is now always compiled with -funsigned-char, checking __CHAR_UNSIGNED__
no longer works. To make it work again, check __CHAR_UNSIGNED__ in a
separate translation unit that is compiled without -funsigned-char.
Fixes: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
Reported-by: Eric Whitney <enwlinux@xxxxxxxxx>
Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
---
fs/ext4/Makefile | 43 +++++++++++++++++++++++++++++++++++++------
fs/ext4/char.c | 24 ++++++++++++++++++++++++
fs/ext4/ext4.h | 2 ++
fs/ext4/super.c | 20 ++++++++++----------
4 files changed, 73 insertions(+), 16 deletions(-)
create mode 100644 fs/ext4/char.c
diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
index 72206a2926765..fa7dc62fa1a2c 100644
--- a/fs/ext4/Makefile
+++ b/fs/ext4/Makefile
@@ -5,12 +5,43 @@
obj-$(CONFIG_EXT4_FS) += ext4.o
-ext4-y := balloc.o bitmap.o block_validity.o dir.o ext4_jbd2.o extents.o \
- extents_status.o file.o fsmap.o fsync.o hash.o ialloc.o \
- indirect.o inline.o inode.o ioctl.o mballoc.o migrate.o \
- mmp.o move_extent.o namei.o page-io.o readpage.o resize.o \
- super.o symlink.o sysfs.o xattr.o xattr_hurd.o xattr_trusted.o \
- xattr_user.o fast_commit.o orphan.o
+ext4-y := balloc.o \
+ bitmap.o \
+ block_validity.o \
+ char.o \
+ dir.o \
+ ext4_jbd2.o \
+ extents.o \
+ extents_status.o \
+ fast_commit.o \
+ file.o \
+ fsmap.o \
+ fsync.o \
+ hash.o \
+ ialloc.o \
+ indirect.o \
+ inline.o \
+ inode.o \
+ ioctl.o \
+ mballoc.o \
+ migrate.o \
+ mmp.o \
+ move_extent.o \
+ namei.o \
+ orphan.o \
+ page-io.o \
+ readpage.o \
+ resize.o \
+ super.o \
+ symlink.o \
+ sysfs.o \
+ xattr.o \
+ xattr_hurd.o \
+ xattr_trusted.o \
+ xattr_user.o
+
+# char.c needs to be compiled with the default char signedness.
+$(obj)/char.o: KBUILD_CFLAGS := $(filter-out -funsigned-char,$(KBUILD_CFLAGS))
ext4-$(CONFIG_EXT4_FS_POSIX_ACL) += acl.o
ext4-$(CONFIG_EXT4_FS_SECURITY) += xattr_security.o
diff --git a/fs/ext4/char.c b/fs/ext4/char.c
new file mode 100644
index 0000000000000..2a8b3df44262c
--- /dev/null
+++ b/fs/ext4/char.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Detect whether char is signed or unsigned by default on this platform,
+ * disregarding the fact that since v6.2, char is always unsigned in the kernel,
+ * i.e. the kernel is now always built with -funsigned char.
+ *
+ * To do this, check __CHAR_UNSIGNED__ in a translation unit that is compiled
+ * *without* -funsigned-char.
+ *
+ * Do *not* include any headers in this file, since it's no longer being tested
+ * that kernel-internal headers build cleanly without -funsigned-char.
+ */
+
+int ext4_is_char_unsigned(void);
+
+int ext4_is_char_unsigned(void)
+{
+#ifdef __CHAR_UNSIGNED__
+ return 1;
+#else
+ return 0;
+#endif
+}
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 140e1eb300d17..bdadad0b4e7ab 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3675,6 +3675,8 @@ extern int ext4_check_blockref(const char *, unsigned int,
extern int ext4_sb_block_valid(struct super_block *sb, struct inode *inode,
ext4_fsblk_t start_blk, unsigned int count);
+/* char.c */
+int ext4_is_char_unsigned(void);
/* extents.c */
struct ext4_ext_path;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 260c1b3e3ef2c..2bd6d1b15d041 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5189,16 +5189,16 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (i & EXT2_FLAGS_UNSIGNED_HASH)
sbi->s_hash_unsigned = 3;
else if ((i & EXT2_FLAGS_SIGNED_HASH) == 0) {
-#ifdef __CHAR_UNSIGNED__
- if (!sb_rdonly(sb))
- es->s_flags |=
- cpu_to_le32(EXT2_FLAGS_UNSIGNED_HASH);
- sbi->s_hash_unsigned = 3;
-#else
- if (!sb_rdonly(sb))
- es->s_flags |=
- cpu_to_le32(EXT2_FLAGS_SIGNED_HASH);
-#endif
+ if (ext4_is_char_unsigned()) {
+ if (!sb_rdonly(sb))
+ es->s_flags |=
+ cpu_to_le32(EXT2_FLAGS_UNSIGNED_HASH);
+ sbi->s_hash_unsigned = 3;
+ } else {
+ if (!sb_rdonly(sb))
+ es->s_flags |=
+ cpu_to_le32(EXT2_FLAGS_SIGNED_HASH);
+ }
}
}
base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4
--
2.39.0
