On Thu, Dec 29, 2022 at 12:49 AM kernel test robot
<oliver.sang@xxxxxxxxx> wrote:
>
> generic/454 _check_generic_filesystem: filesystem on /dev/sda4 is inconsistent
The commentary on that test is:
Create xattrs with multiple keys that all appear the same
(in unicode, anyway) but point to different values. In theory all
Linux filesystems should allow this (filenames are a sequence of
arbitrary bytes) even if the user implications are horrifying.
and looking at the script it seems to indeed just do setfattr and
getfattr with some unusual data (ie high bit set).
Adding Ted, since this is apparently all on ext4. I guess it could be
the vfs layer too, but it really doesn't tend to look very much at the
xattr data, so.. Adding Christian Brauner anyway, since he's been
working in this area for other reasons.
Ted, Christian - I cut down the report mercilessly. It's not really
all that interesting, apart from the basic information of "xfstest
generic/454 started failing consistently on ext4 at commit
3bc753c06dd0 ('kbuild: treat char as always unsigned')".
If you think you need more, see
https://lore.kernel.org/all/202212291509.704a11c9-oliver.sang@xxxxxxxxx/
Also, I'm surprised this hasn't been an issue earlier - 'char' has
always been unsigned on arm (among other architectures), so if this
test started failing now on x86-64 due to -funsigned-char, it has
presumably been failing on arm the whole time.
I assume it's something that compares a 'char *name' by value, but the
ones I looked at (eg xattr_find_entry() used strlen()/memcmp() which
should be all good).
Oh, I think I see one potential problem in ext4:
ext4_xattr_hash_entry() is hot garbage. Lookie here:
while (name_len--) {
hash = (hash << NAME_HASH_SHIFT) ^
(hash >> (8*sizeof(hash) - NAME_HASH_SHIFT)) ^
*name++;
}
so that hash will now depend on the sign of that 'char *name' pointer.
If that hash ever has any long-term meaning (ie saved on disk or
exposed some other way), that would be problematic.
Of course, if it's just an ephemeral thing only used on that one
machine, then it isn't a problem - having different hashes on
different machines (and different boots) is a non-issue. But
considering that it then does
return cpu_to_le32(hash);
at the end does seem to imply to me that it all really *tries* to be
architecture-neutral, and has just failed horrendously.
Because that does look like code that might get confused by the sign of a char.
There might be other cases, I only looked very quickly for these kinds
of problems.
Linus
