On Tue, Jan 17, 2023 at 7:56 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> [Added some Cc's, and updated subject to reflect what this is really about]
Hmm.
I really hate this.
> On Tue, Jan 17, 2023 at 05:10:55PM -0700, Andreas Dilger 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:
So clearly ext4 is completely buggy in this respect, but this is
exactly what would happen if you just mount a disk that was written to
on (old, pre-funsigned-char) x86, and then mount it on, say, an arm
machine that has always been unsigned-char.
That was always supposed to work.
So switching to a new kernel really should just be *exactly* the same
as moving the disk to a different machine.
And that's still "supposed to just work".
And this whole "let's get it wrong, and make x86 act as if 'char' is
signed, even when it isn't" seems entirely the wrong way around.
So the bug here is that __ext4_fill_super() seems to not look at the
actual on-disk thing, but instead do
> > static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> > {
...
> > else if ((i & EXT2_FLAGS_SIGNED_HASH) == 0) {
> > #ifdef __CHAR_UNSIGNED__
but at the same time this code is *exactly* the code that is trying to
deal with "oh, you moved the disk from a signed architecture to an
unsigned one".
So the above code is literally what should fix up that movement -
taking the *actual* new signedness (or lack thereof, in this case)
into account.
Now, it apparently doesn't work very well, and I suspect the reason it
doesn't work is that the xattr code doesn't actually test these
EXT2_FLAGS_SIGNED_HASH bits (and the s_hash_unsigned field value that
goes along with it).
But we should *fix* that.
Instead, the patch is self-admittedly very ugly:
> Below is one very ugly solution. It seems to work [..]
but I really don't think it works. It just perpetuates the bug that
you can't move a filesystem from one architecture to another.
So I really think that the solution is either
(a) just admit that ext4 was buggy, and say "char is now unsigned",
and know that generic/454 will fail when you switch from a buggy
kernel to a new one that no longer has this signedness bug.
(b) fix ext4_xattr_hash_entry() to actually see "oh, this filesystem
was created with signed chars, and so we'll use that algorithm even
though our chars are always unsigned".
Honestly, the only actual case of breakage I have heard of is that
test, so I was hoping that (a) is simply the acceptable and simplest
solution. It basically says "nobody really cares, we're now always
unsigned, real people didn't use non-ASCII xattr names".
Anyway, here's a TOTALLY UNTESTED patch to do (b). Maybe it's entirely
broken, but I think you can see what I'm aiming for.
Comments?
Linus
fs/ext4/xattr.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 7decaaf27e82..69a1b8c6a2ec 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -81,6 +81,8 @@ ext4_xattr_block_cache_find(struct inode *, struct ext4_xattr_header *,
struct mb_cache_entry **);
static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
size_t value_count);
+static __le32 ext4_xattr_hash_entry_signed(char *name, size_t name_len, __le32 *value,
+ size_t value_count);
static void ext4_xattr_rehash(struct ext4_xattr_header *);
static const struct xattr_handler * const ext4_xattr_handler_map[] = {
@@ -470,8 +472,21 @@ ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
tmp_data = cpu_to_le32(hash);
e_hash = ext4_xattr_hash_entry(entry->e_name, entry->e_name_len,
&tmp_data, 1);
- if (e_hash != entry->e_hash)
- return -EFSCORRUPTED;
+ /* All good? */
+ if (e_hash == entry->e_hash)
+ return 0;
+
+ /*
+ * Not good. Maybe the entry hash was calculated
+ * using the buggy signed char version?
+ */
+ e_hash = ext4_xattr_hash_entry_signed(entry->e_name, entry->e_name_len,
+ &tmp_data, 1);
+ if (e_hash == entry->e_hash)
+ return 0;
+
+ /* Still no match - bad */
+ return -EFSCORRUPTED;
}
return 0;
}
@@ -3091,6 +3106,28 @@ static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
return cpu_to_le32(hash);
}
+/*
+ * ext4_xattr_hash_entry_signed()
+ *
+ * Compute the hash of an extended attribute incorrectly.
+ */
+static __le32 ext4_xattr_hash_entry_signed(char *name, size_t name_len, __le32 *value, size_t value_count)
+{
+ __u32 hash = 0;
+
+ while (name_len--) {
+ hash = (hash << NAME_HASH_SHIFT) ^
+ (hash >> (8*sizeof(hash) - NAME_HASH_SHIFT)) ^
+ (signed char)*name++;
+ }
+ while (value_count--) {
+ hash = (hash << VALUE_HASH_SHIFT) ^
+ (hash >> (8*sizeof(hash) - VALUE_HASH_SHIFT)) ^
+ le32_to_cpu(*value++);
+ }
+ return cpu_to_le32(hash);
+}
+
#undef NAME_HASH_SHIFT
#undef VALUE_HASH_SHIFT
