[PATCH v2 0/3] ocfs2: deal with legacy signed name hash values

[PATCH v2 0/3] ocfs2: deal with legacy signed name hash values

[Joseph Qi]

Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") turned on
-funsigned-char globally, which changed the result of two naked 'char'
loads in ocfs2 hashing code:

	str2hashbuf()            val = msg[i] + (val << 8);
	ocfs2_xattr_name_hash()  hash = (hash << 5) ^ (hash >> 27) ^ *name++;

A name byte >= 0x80 used to sign-extend and now zero-extends.  Both
results are written to disk, the first into the dx leaf when a directory
entry is indexed and the second into xe_name_hash when an xattr is
stored, so anything written by a pre-6.2 kernel is looked up under a
different hash and is no longer found.  readdir and listxattr still list
those names because neither of them hashes, which makes this look like
the filesystem losing entries rather than like a lookup bug.  Anderson
Ferneda reported it that way for directories with non-ASCII names [1].

Patches 1 and 3 do what ext4 did in commit f3bbac32475b2 ("ext4: deal
with legacy signed xattr name hash values"): look up with the current
unsigned hash, retry with the legacy signed one on a miss, and always
store new entries under the unsigned hash.  The retry is skipped when the
two hashes come out equal, so an ASCII name does not walk the index
twice.  Both hash functions now spell out the signedness instead of
leaving it to -funsigned-char, as commit 854f0912f813 ("ext4: make xattr
char unsignedness in hash explicit") did, so a backport to a kernel
without the flag still computes both variants correctly.

Patch 2 is a separate fix, placed before patch 3 so that every commit in
the series is correct on its own.  ocfs2_check_xattr_bucket_collision()
recomputes the hash of the name to decide whether splitting a full bucket
can make room for the entry being set.  That is right for a new entry and
wrong for one that already exists, since an update leaves xe_name_hash
alone.  Against a bucket holding legacy hashes the comparison does not
match, so the split goes ahead and cannot help: a bucket whose entries
all share one hash has no divide position.  How that ends depends on
where the unsigned hash sorts.  Below the legacy ones, the set fails with
-ENOSPC after growing the tree for nothing.  Above them, it lands in the
empty bucket the split just appended and stores a second copy of the
entry there while the original stays behind, so listxattr reports the
name twice.  Using the stored hash closes both.

Nothing retires a legacy hash at runtime.  Deleting or updating an entry
does not rewrite it, and the only code that rehashes existing dirents is
ocfs2_expand_inline_dir() on the inline to extent conversion, so the
retry stays for as long as the legacy entries do.  Rebuilding the index
offline is what removes it for good.

Changes since v1:
  - add patch 2, so that ocfs2_check_xattr_bucket_collision() uses the
    stored hash of an existing entry rather than recomputing it, to
    address Sashiko's comments on v1 patch 2/2;
  - v1 patch 2/2 is now patch 3, with no code change;
  - patch 1 is unchanged and picks up Heming's Reviewed-by.

[1] https://lore.kernel.org/ocfs2-devel/CP5P284MB2780AC2C3C2AB2CF2B6CB7AFBE942@CP5P284MB2780.BRAP284.PROD.OUTLOOK.COM/

Joseph Qi (3):
  ocfs2: deal with legacy signed dir index name hash values
  ocfs2: use the stored hash when checking xattr bucket collision
  ocfs2: deal with legacy signed xattr name hash values

 fs/ocfs2/dir.c   |  76 +++++++++++++++++++++++++-----
 fs/ocfs2/xattr.c | 120 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 164 insertions(+), 32 deletions(-)

-- 
2.39.3

[PATCH v2 1/3] ocfs2: deal with legacy signed dir index name hash values

[Joseph Qi]

Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") set
-funsigned-char globally, which changed the result of the naked 'char'
load in str2hashbuf():

	val = msg[i] + (val << 8);

A name byte >= 0x80 used to sign-extend and now zero-extends, so
ocfs2_dx_dir_name_hash() computes a different hash pair for every name
containing one.  The pair is written into the dx leaf when the entry is
created, so an index built by an older kernel no longer matches and
ocfs2_dx_dir_search() returns -ENOENT for a name that readdir still
lists.

Search with the current unsigned hash and, on a miss, retry with the
legacy signed one, as ext4 does in commit f3bbac32475b2 ("ext4: deal
with legacy signed xattr name hash values").  New entries are always
indexed under the unsigned hash.  Skip the retry when the two hashes
are equal, so that a miss on an ASCII name does not walk the index
twice.

The retry costs a second walk of the dx tree, so a negative lookup of a
name containing a byte >= 0x80 does twice the work.  That is bounded and
worth it, and directories whose dx root is still inline pay nothing
extra, since there the retry only rescans the root block that is already
loaded.  Nothing retires it at runtime: a legacy entry keeps its legacy
hash, because neither deleting nor updating an entry rewrites the dx
hash, and the only code that rehashes existing dirents is
ocfs2_expand_inline_dir() on the inline to extent conversion.
Rebuilding the index offline is what removes the cost for good.

Also spell out the signedness in str2hashbuf() instead of leaving the
current hash to -funsigned-char, as commit 854f0912f813 ("ext4: make
xattr char unsignedness in hash explicit") did, so that both variants
stay correct if this is backported to a kernel without the flag.

Reported-by: Anderson Ferneda <anderson.ferneda@braza.com.br>
Link: https://lore.kernel.org/ocfs2-devel/CP5P284MB2780AC2C3C2AB2CF2B6CB7AFBE942@CP5P284MB2780.BRAP284.PROD.OUTLOOK.COM/
Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
Cc: <stable@vger.kernel.org> # 6.2+
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
---
 fs/ocfs2/dir.c | 76 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 11 deletions(-)

diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
index 55c4a305a282..3f159a20a248 100644
--- a/fs/ocfs2/dir.c
+++ b/fs/ocfs2/dir.c
@@ -221,7 +221,8 @@ static void TEA_transform(__u32 buf[4], __u32 const in[])
 	buf[1] += b1;
 }
 
-static void str2hashbuf(const char *msg, int len, __u32 *buf, int num)
+static void str2hashbuf(const char *msg, int len, __u32 *buf, int num,
+			bool legacy_signed)
 {
 	__u32	pad, val;
 	int	i;
@@ -235,7 +236,10 @@ static void str2hashbuf(const char *msg, int len, __u32 *buf, int num)
 	for (i = 0; i < len; i++) {
 		if ((i % 4) == 0)
 			val = pad;
-		val = msg[i] + (val << 8);
+		if (legacy_signed)
+			val = (signed char)msg[i] + (val << 8);
+		else
+			val = (unsigned char)msg[i] + (val << 8);
 		if ((i % 4) == 3) {
 			*buf++ = val;
 			val = pad;
@@ -248,8 +252,9 @@ static void str2hashbuf(const char *msg, int len, __u32 *buf, int num)
 		*buf++ = pad;
 }
 
-static void ocfs2_dx_dir_name_hash(struct inode *dir, const char *name, int len,
-				   struct ocfs2_dx_hinfo *hinfo)
+static void __ocfs2_dx_dir_name_hash(struct inode *dir, const char *name,
+				     int len, struct ocfs2_dx_hinfo *hinfo,
+				     bool legacy_signed)
 {
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
 	const char	*p;
@@ -279,7 +284,7 @@ static void ocfs2_dx_dir_name_hash(struct inode *dir, const char *name, int len,
 
 	p = name;
 	while (len > 0) {
-		str2hashbuf(p, len, in, 4);
+		str2hashbuf(p, len, in, 4, legacy_signed);
 		TEA_transform(buf, in);
 		len -= 16;
 		p += 16;
@@ -290,6 +295,19 @@ static void ocfs2_dx_dir_name_hash(struct inode *dir, const char *name, int len,
 	hinfo->minor_hash = buf[1];
 }
 
+static void ocfs2_dx_dir_name_hash(struct inode *dir, const char *name, int len,
+				   struct ocfs2_dx_hinfo *hinfo)
+{
+	__ocfs2_dx_dir_name_hash(dir, name, len, hinfo, false);
+}
+
+static void ocfs2_dx_dir_name_hash_signed(struct inode *dir, const char *name,
+					  int len,
+					  struct ocfs2_dx_hinfo *hinfo)
+{
+	__ocfs2_dx_dir_name_hash(dir, name, len, hinfo, true);
+}
+
 /*
  * bh passed here can be an inode block or a dir data block, depending
  * on the inode inline data flag.
@@ -1021,10 +1039,10 @@ static int ocfs2_dx_dir_lookup(struct inode *inode,
 	return ret;
 }
 
-static int ocfs2_dx_dir_search(const char *name, int namelen,
-			       struct inode *dir,
-			       struct ocfs2_dx_root_block *dx_root,
-			       struct ocfs2_dir_lookup_result *res)
+static int __ocfs2_dx_dir_search(const char *name, int namelen,
+				 struct inode *dir,
+				 struct ocfs2_dx_root_block *dx_root,
+				 struct ocfs2_dir_lookup_result *res)
 {
 	int ret, i, found;
 	u64 phys;
@@ -1037,8 +1055,6 @@ static int ocfs2_dx_dir_search(const char *name, int namelen,
 	struct ocfs2_extent_list *dr_el;
 	struct ocfs2_dx_entry_list *entry_list;
 
-	ocfs2_dx_dir_name_hash(dir, name, namelen, &res->dl_hinfo);
-
 	if (ocfs2_dx_root_inline(dx_root)) {
 		entry_list = &dx_root->dr_entries;
 		goto search;
@@ -1135,6 +1151,44 @@ static int ocfs2_dx_dir_search(const char *name, int namelen,
 	return ret;
 }
 
+static int ocfs2_dx_dir_search(const char *name, int namelen,
+			       struct inode *dir,
+			       struct ocfs2_dx_root_block *dx_root,
+			       struct ocfs2_dir_lookup_result *res)
+{
+	struct ocfs2_dx_hinfo legacy;
+	int ret;
+
+	ocfs2_dx_dir_name_hash(dir, name, namelen, &res->dl_hinfo);
+
+	ret = __ocfs2_dx_dir_search(name, namelen, dir, dx_root, res);
+	if (ret != -ENOENT)
+		return ret;
+
+	/*
+	 * Nothing under the current hash.  The entry may have been indexed by
+	 * an older kernel, which sign-extended the name bytes when hashing.
+	 * New entries are always indexed under the unsigned hash, so only fall
+	 * back to the legacy signed one when it can actually differ: an ASCII
+	 * name hashes the same either way, and a genuine miss on one should
+	 * not have to walk the index twice.
+	 */
+	ocfs2_dx_dir_name_hash_signed(dir, name, namelen, &legacy);
+	if (legacy.major_hash == res->dl_hinfo.major_hash &&
+	    legacy.minor_hash == res->dl_hinfo.minor_hash)
+		return ret;
+
+	res->dl_hinfo = legacy;
+
+	ret = __ocfs2_dx_dir_search(name, namelen, dir, dx_root, res);
+	if (ret)
+		return ret;
+
+	pr_warn_once("ocfs2: directory index with signed name hash\n");
+
+	return 0;
+}
+
 static int ocfs2_find_entry_dx(const char *name, int namelen,
 			       struct inode *dir,
 			       struct ocfs2_dir_lookup_result *lookup)
-- 
2.39.3

[PATCH v2 2/3] ocfs2: use the stored hash when checking xattr bucket collision

[Joseph Qi]

ocfs2_check_xattr_bucket_collision() decides whether splitting a full
bucket can make room for the entry being set by recomputing the hash of
the name:

	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));

	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
		return 0;

For a new entry that is the hash it will be stored under, so comparing it
is correct.  An existing entry keeps the hash it already has: an update
never rewrites xe_name_hash, since ocfs2_xa_add_entry() is the only
writer of that field for a real entry and ocfs2_xa_prepare_entry() calls
it only when loc->xl_entry is NULL.

An entry stored by a kernel that sign-extended the name bytes when
hashing can still be updated, because ocfs2_xattr_find_entry() searches
a non-indexed xattr block by memcmp on the name and never looks at
xe_name_hash.  Growing it past the space left in the block converts the
block into a tree, ocfs2_cp_xattr_block_to_bucket() fills the bucket in
stored hash order, and the update runs out of room in the bucket too.
The collision check then compares the unsigned hash against the legacy
hashes in the bucket and reports no collision.

ocfs2_xattr_set_entry_index_block() goes on to allocate a bucket that
ocfs2_divide_xattr_bucket() cannot fill: a bucket whose entries all share
one hash has no divide position, so all it does is append an empty bucket
with a sentinel hash one above the last entry's.

The re-search that follows depends on where the unsigned hash sorts.
Below the legacy ones, it comes back to the full bucket and the set fails
with -ENOSPC, having grown the tree for nothing.  Above them, it lands on
the new empty bucket, which ocfs2_xattr_bucket_find() handles explicitly
and ocfs2_find_xe_in_bucket() scans zero times, so the set stores a second
copy of the entry under the unsigned hash and returns success.  The
original stays in the full bucket, listxattr reports the name twice and
getxattr returns the new copy.

Pass the hash the entry is stored under instead: the stored one for an
existing entry, the unsigned one for a new entry.  A tree whose entries
are all stored under the unsigned hash sees no change, since there the
two are the same value.

Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
Cc: <stable@vger.kernel.org> # 6.2+
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index a428fe908116..c5a39a7d43d0 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -5896,16 +5896,14 @@ static int ocfs2_rm_xattr_cluster(struct inode *inode,
 
 /*
  * check whether the xattr bucket is filled up with the same hash value.
- * If we want to insert the xattr with the same hash, return -ENOSPC.
- * If we want to insert a xattr with different hash value, go ahead
- * and ocfs2_divide_xattr_bucket will handle this.
+ * If the entry being set carries that same hash, return -ENOSPC, since
+ * ocfs2_divide_xattr_bucket() has no divide position to work with.
+ * Otherwise go ahead and ocfs2_divide_xattr_bucket() will handle this.
  */
-static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
-					      struct ocfs2_xattr_bucket *bucket,
-					      const char *name)
+static int ocfs2_check_xattr_bucket_collision(struct ocfs2_xattr_bucket *bucket,
+					      u32 name_hash)
 {
 	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
-	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
 
 	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
 		return 0;
@@ -5974,6 +5972,7 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
 					     struct ocfs2_xattr_search *xs,
 					     struct ocfs2_xattr_set_ctxt *ctxt)
 {
+	u32 name_hash;
 	int ret;
 
 	trace_ocfs2_xattr_set_entry_index_block(xi->xi_name);
@@ -5993,10 +5992,18 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
 	 * the maximum number of collisions we will allow for then is
 	 * one bucket's worth, so check it here whether we need to
 	 * add a new bucket for the insert.
+	 *
+	 * An existing entry keeps the hash it was stored under, and that is
+	 * the hash a split has to work with.  A new entry is stored under the
+	 * unsigned one, which is what ocfs2_xa_add_entry() will write.
 	 */
-	ret = ocfs2_check_xattr_bucket_collision(inode,
-						 xs->bucket,
-						 xi->xi_name);
+	if (xs->not_found)
+		name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
+						  xi->xi_name_len);
+	else
+		name_hash = le32_to_cpu(xs->here->xe_name_hash);
+
+	ret = ocfs2_check_xattr_bucket_collision(xs->bucket, name_hash);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
-- 
2.39.3

[PATCH v2 3/3] ocfs2: deal with legacy signed xattr name hash values

[Joseph Qi]

Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") set
-funsigned-char globally, which changed the result of the naked 'char'
load in ocfs2_xattr_name_hash():

	hash = (hash << OCFS2_HASH_SHIFT) ^
	       (hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT)) ^
	       *name++;

A name byte >= 0x80 used to sign-extend and now zero-extends, so the
hash no longer matches the xe_name_hash an older kernel stored.  An
indexed xattr tree is searched by that hash alone, and both the bucket
binary search and the entry scan within it stop as soon as the wanted
hash falls below an entry's, so the entry is never reached: getxattr,
setxattr and removexattr return -ENODATA for a name that listxattr
still lists.

Search with the current unsigned hash and, on a miss, retry with the
legacy signed one, as ext4 does in commit f3bbac32475b2 ("ext4: deal
with legacy signed xattr name hash values").  New entries are always
stored under the unsigned hash.  Skip the retry when the two hashes are
equal, so that a miss on an ASCII name does not walk the tree twice.

A miss is not empty handed: ocfs2_xattr_bucket_find() leaves xs->bucket
holding the bucket a new entry would go into.  So after a double miss
drop it and search once more with the unsigned hash, otherwise a new
entry would be placed by its legacy hash and stored under its unsigned
one, breaking the ordering the search relies on.

Only indexed trees are affected; inline xattrs and non-indexed xattr
blocks compare names with memcmp and never look at the hash.

Also spell out the signedness instead of leaving the current hash to
-funsigned-char, as commit 854f0912f813 ("ext4: make xattr char
unsignedness in hash explicit") did, so that both variants stay correct
if this is backported to a kernel without the flag.

Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
Cc: <stable@vger.kernel.org> # 6.2+
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 93 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 82 insertions(+), 11 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index c5a39a7d43d0..e92bcf401490 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -601,9 +601,10 @@ static inline const char *ocfs2_xattr_prefix(int name_index)
 	return handler ? xattr_prefix(handler) : NULL;
 }
 
-static u32 ocfs2_xattr_name_hash(struct inode *inode,
-				 const char *name,
-				 int name_len)
+static u32 __ocfs2_xattr_name_hash(struct inode *inode,
+				   const char *name,
+				   int name_len,
+				   bool legacy_signed)
 {
 	/* Get hash value of uuid from super block */
 	u32 hash = OCFS2_SB(inode->i_sb)->uuid_hash;
@@ -612,13 +613,30 @@ static u32 ocfs2_xattr_name_hash(struct inode *inode,
 	/* hash extended attribute name */
 	for (i = 0; i < name_len; i++) {
 		hash = (hash << OCFS2_HASH_SHIFT) ^
-		       (hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT)) ^
-		       *name++;
+		       (hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT));
+		if (legacy_signed)
+			hash ^= (signed char)name[i];
+		else
+			hash ^= (unsigned char)name[i];
 	}
 
 	return hash;
 }
 
+static u32 ocfs2_xattr_name_hash(struct inode *inode,
+				 const char *name,
+				 int name_len)
+{
+	return __ocfs2_xattr_name_hash(inode, name, name_len, false);
+}
+
+static u32 ocfs2_xattr_name_hash_signed(struct inode *inode,
+					const char *name,
+					int name_len)
+{
+	return __ocfs2_xattr_name_hash(inode, name, name_len, true);
+}
+
 static int ocfs2_xattr_entry_real_size(int name_len, size_t value_len)
 {
 	return namevalue_size(name_len, value_len) +
@@ -4304,11 +4322,12 @@ static int ocfs2_xattr_bucket_find(struct inode *inode,
 	return ret;
 }
 
-static int ocfs2_xattr_index_block_find(struct inode *inode,
-					struct buffer_head *root_bh,
-					int name_index,
-					const char *name,
-					struct ocfs2_xattr_search *xs)
+static int __ocfs2_xattr_index_block_find(struct inode *inode,
+					  struct buffer_head *root_bh,
+					  int name_index,
+					  const char *name,
+					  u32 name_hash,
+					  struct ocfs2_xattr_search *xs)
 {
 	int ret;
 	struct ocfs2_xattr_block *xb =
@@ -4317,7 +4336,6 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
 	struct ocfs2_extent_list *el = &xb_root->xt_list;
 	u64 p_blkno = 0;
 	u32 first_hash, num_clusters = 0;
-	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
 
 	if (le16_to_cpu(el->l_next_free_rec) == 0)
 		return -ENODATA;
@@ -4348,6 +4366,59 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
 	return ret;
 }
 
+static int ocfs2_xattr_index_block_find(struct inode *inode,
+					struct buffer_head *root_bh,
+					int name_index,
+					const char *name,
+					struct ocfs2_xattr_search *xs)
+{
+	u32 name_hash, legacy_hash;
+	int name_len = strlen(name);
+	int ret;
+
+	name_hash = ocfs2_xattr_name_hash(inode, name, name_len);
+
+	ret = __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
+					     name_hash, xs);
+	if (ret != -ENODATA)
+		return ret;
+
+	/*
+	 * Nothing under the current hash.  The entry may have been stored by
+	 * an older kernel, which sign-extended the name bytes when hashing.
+	 * Skip the retry when the two hashes are equal, so that a name made
+	 * only of ASCII does not have to walk the tree twice.
+	 */
+	legacy_hash = ocfs2_xattr_name_hash_signed(inode, name, name_len);
+	if (legacy_hash == name_hash)
+		return ret;
+
+	/*
+	 * A miss still leaves xs->bucket holding the bucket a new entry would
+	 * be inserted into, so drop it before searching again.
+	 */
+	ocfs2_xattr_bucket_relse(xs->bucket);
+
+	ret = __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
+					     legacy_hash, xs);
+	if (!ret) {
+		pr_warn_once("ocfs2: xattr tree with signed name hash\n");
+		return ret;
+	}
+	if (ret != -ENODATA)
+		return ret;
+
+	/*
+	 * Not under either hash.  Restore the unsigned placement, since that
+	 * is where a new entry is stored: leaving the bucket where the legacy
+	 * hash put it would break the ordering the search relies on.
+	 */
+	ocfs2_xattr_bucket_relse(xs->bucket);
+
+	return __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
+					      name_hash, xs);
+}
+
 static int ocfs2_iterate_xattr_buckets(struct inode *inode,
 				       u64 blkno,
 				       u32 clusters,
-- 
2.39.3

Re: [PATCH v2 2/3] ocfs2: use the stored hash when checking xattr bucket collision

[Joseph Qi]

On 10/9/26 4:44 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] fs/ocfs2: xattr data corruption due to stale loc.xl_entry after bucket defrag
> --
> 
> commit 6247b6061c7a43f1fbd44574fe532663a6b670bc
> Author: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
>     ocfs2: use the stored hash when checking xattr bucket collision
> 
> This patch changes ocfs2_check_xattr_bucket_collision() to use the stored hash
> for existing entries when checking for bucket collisions during splits. It
> fixes an issue where unsigned hashes compared against legacy buckets reported
> no collision, leading to duplicate entries.
> 
>> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
>> index a428fe908116e..c5a39a7d43d07 100644
>> --- a/fs/ocfs2/xattr.c
>> +++ b/fs/ocfs2/xattr.c
> 
> [ ... ]
> 
>> @@ -5993,10 +5992,18 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>>  	 * the maximum number of collisions we will allow for then is
>>  	 * one bucket's worth, so check it here whether we need to
>>  	 * add a new bucket for the insert.
>> +	 *
>> +	 * An existing entry keeps the hash it was stored under, and that is
>> +	 * the hash a split has to work with.  A new entry is stored under the
>> +	 * unsigned one, which is what ocfs2_xa_add_entry() will write.
>>  	 */
>> -	ret = ocfs2_check_xattr_bucket_collision(inode,
>> -						 xs->bucket,
>> -						 xi->xi_name);
>> +	if (xs->not_found)
>> +		name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
>> +						  xi->xi_name_len);
>> +	else
>> +		name_hash = le32_to_cpu(xs->here->xe_name_hash);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> [Severity: High]
> If ocfs2_xattr_set_entry_bucket() fails with -ENOSPC after defragmentation,
> can this access a stale xs->here pointer?
> 
> In ocfs2_xattr_set_entry_bucket(), loc.xl_entry is initialized to point
> directly to the target array element (xs->here):
> 
> 	ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
> 				       xs->not_found ? NULL : xs->here);
> 
> If ocfs2_xa_set() fails with -ENOSPC, it calls ocfs2_defrag_xattr_bucket(),
> which sorts the entries array by xe_name_hash:
> 
> 	/* sort the entries by their name_hash. */
> 	sort(entries, le16_to_cpu(xh->xh_count),
> 	     sizeof(struct ocfs2_xattr_entry),
> 	     cmp_xe, NULL);
> 
> Since sort() is unstable and hashes only cover the name suffix, identical-hash
> entries can be swapped. If they are swapped, loc.xl_entry and xs->here now
> point to the wrong xattr entry.
> 
> The second ocfs2_xa_set() call then reuses the stale loc.xl_entry pointer
> without re-evaluating its position in the sorted array:
> 
> 	ret = ocfs2_xa_set(&loc, xi, ctxt);
> 
> Can this overwrite the wrong xattr's data on disk, causing data corruption?
> 
> If the second ocfs2_xa_set() also fails with -ENOSPC, it returns to
> ocfs2_xattr_set_entry_index_block() where the patch now relies on the stale
> xs->here pointer to read the name_hash.
> 

It seems unrelated to my patch.

Since the array is already sorted on xe_name_hash when defrag runs,
re-sorting on the same key can only permute within a run of equal
hashes. The pointer may denote a different entry, but not a different
hash.

The underlying bug is real and independent of my series: after defrag
permutes an equal-hash run, that second ocfs2_xa_set(&loc, ...) operates
on a different entry. I'd like this to be addressed in a separate
thread.

Thanks,
Joseph

Re: [PATCH v2 3/3] ocfs2: deal with legacy signed xattr name hash values

[Joseph Qi]

On 10/9/26 4:45 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] fs/ocfs2: NULL pointer dereference in ocfs2_xattr_set_entry_bucket() on corrupted index
> --
> 
> commit d720400774af2e4eeb97fd62736805be315e728c
> Author: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> ocfs2: deal with legacy signed xattr name hash values
> 
> This commit introduces retry logic to xattr tree searches to fall back to a
> legacy signed hash if an entry is missed using the unsigned hash. This fixes
> a regression where names containing bytes >= 0x80 became unreachable when
> char signedness was changed in kbuild.
> 
>> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
>> index c5a39a7d43d07..e92bcf401490d 100644
>> --- a/fs/ocfs2/xattr.c
>> +++ b/fs/ocfs2/xattr.c
> 
> [ ... ]
> 
>> @@ -4304,11 +4322,12 @@ static int ocfs2_xattr_bucket_find(struct inode *inode,
>>  	return ret;
>>  }
>>  
>> -static int ocfs2_xattr_index_block_find(struct inode *inode,
>> -					struct buffer_head *root_bh,
>> -					int name_index,
>> -					const char *name,
>> -					struct ocfs2_xattr_search *xs)
>> +static int __ocfs2_xattr_index_block_find(struct inode *inode,
>> +					  struct buffer_head *root_bh,
>> +					  int name_index,
>> +					  const char *name,
>> +					  u32 name_hash,
>> +					  struct ocfs2_xattr_search *xs)
>>  {
>>  	int ret;
>>  	struct ocfs2_xattr_block *xb =
>> @@ -4317,7 +4336,6 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
>>  	struct ocfs2_extent_list *el = &xb_root->xt_list;
>>  	u64 p_blkno = 0;
>>  	u32 first_hash, num_clusters = 0;
>> -	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
>>  
>>  	if (le16_to_cpu(el->l_next_free_rec) == 0)
>>  		return -ENODATA;
> 
> [ ... ]
> 
>> @@ -4348,6 +4366,59 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
>>  	return ret;
>>  }
>>  
>> +static int ocfs2_xattr_index_block_find(struct inode *inode,
>> +					struct buffer_head *root_bh,
>> +					int name_index,
>> +					const char *name,
>> +					struct ocfs2_xattr_search *xs)
>> +{
>> +	u32 name_hash, legacy_hash;
>> +	int name_len = strlen(name);
>> +	int ret;
>> +
>> +	name_hash = ocfs2_xattr_name_hash(inode, name, name_len);
>> +
>> +	ret = __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
>> +					     name_hash, xs);
>> +	if (ret != -ENODATA)
>> +		return ret;
>> +
>> +	/*
>> +	 * Nothing under the current hash.  The entry may have been stored by
>> +	 * an older kernel, which sign-extended the name bytes when hashing.
>> +	 * Skip the retry when the two hashes are equal, so that a name made
>> +	 * only of ASCII does not have to walk the tree twice.
>> +	 */
>> +	legacy_hash = ocfs2_xattr_name_hash_signed(inode, name, name_len);
>> +	if (legacy_hash == name_hash)
>> +		return ret;
>> +
>> +	/*
>> +	 * A miss still leaves xs->bucket holding the bucket a new entry would
>> +	 * be inserted into, so drop it before searching again.
>> +	 */
>> +	ocfs2_xattr_bucket_relse(xs->bucket);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> [Severity: High]
> Does this new retry logic need to validate that xs->bucket was actually
> populated before relying on it?
> 
> The patch builds its retry logic under the explicit assumption that a miss
> invariably leaves xs->bucket populated. However, if the filesystem is
> corrupted such that the external xattr block is indexed but its extent list
> is empty (el->l_next_free_rec == 0), __ocfs2_xattr_index_block_find()
> returns -ENODATA immediately without calling ocfs2_xattr_bucket_find().
> 
> This early return leaves xs->bucket->bu_bhs unpopulated.
> 
> Later, during a setxattr() operation, ocfs2_xattr_block_set() delegates
> to ocfs2_xattr_set_entry_index_block() and then
> ocfs2_xattr_set_entry_bucket(). The call to ocfs2_init_xattr_bucket_xa_loc()
> expands the bucket_xh() macro, which accesses bucket->bu_bhs[0]->b_data.
> Since bu_bhs[0] is unpopulated, this triggers a NULL pointer dereference.
> 
> Can we add validation to handle cases where __ocfs2_xattr_index_block_find()
> returns -ENODATA but doesn't actually populate the bucket?
> 

It's a pre-exsiting issue.
It seems we have to enhance the validation in ocfs2_validate_xattr_block().

Thanks,
Joseph

Re: [PATCH v2 2/3] ocfs2: use the stored hash when checking xattr bucket collision

[Heming Zhao]

On Fri, Oct 09, 2026 at 04:30:01PM +0800, Joseph Qi wrote:
> ocfs2_check_xattr_bucket_collision() decides whether splitting a full
> bucket can make room for the entry being set by recomputing the hash of
> the name:
> 
> 	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
> 
> 	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
> 		return 0;
> 
> For a new entry that is the hash it will be stored under, so comparing it
> is correct.  An existing entry keeps the hash it already has: an update
> never rewrites xe_name_hash, since ocfs2_xa_add_entry() is the only
> writer of that field for a real entry and ocfs2_xa_prepare_entry() calls
> it only when loc->xl_entry is NULL.
> 
> An entry stored by a kernel that sign-extended the name bytes when
> hashing can still be updated, because ocfs2_xattr_find_entry() searches
> a non-indexed xattr block by memcmp on the name and never looks at
> xe_name_hash.  Growing it past the space left in the block converts the
> block into a tree, ocfs2_cp_xattr_block_to_bucket() fills the bucket in
> stored hash order, and the update runs out of room in the bucket too.
> The collision check then compares the unsigned hash against the legacy
> hashes in the bucket and reports no collision.
> 
> ocfs2_xattr_set_entry_index_block() goes on to allocate a bucket that
> ocfs2_divide_xattr_bucket() cannot fill: a bucket whose entries all share
> one hash has no divide position, so all it does is append an empty bucket
> with a sentinel hash one above the last entry's.
> 
> The re-search that follows depends on where the unsigned hash sorts.
> Below the legacy ones, it comes back to the full bucket and the set fails
> with -ENOSPC, having grown the tree for nothing.  Above them, it lands on
> the new empty bucket, which ocfs2_xattr_bucket_find() handles explicitly
> and ocfs2_find_xe_in_bucket() scans zero times, so the set stores a second
> copy of the entry under the unsigned hash and returns success.  The
> original stays in the full bucket, listxattr reports the name twice and
> getxattr returns the new copy.
> 
> Pass the hash the entry is stored under instead: the stored one for an
> existing entry, the unsigned one for a new entry.  A tree whose entries
> are all stored under the unsigned hash sees no change, since there the
> two are the same value.
> 
> Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
> Cc: <stable@vger.kernel.org> # 6.2+
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

LGTM.
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/xattr.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index a428fe908116..c5a39a7d43d0 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -5896,16 +5896,14 @@ static int ocfs2_rm_xattr_cluster(struct inode *inode,
>  
>  /*
>   * check whether the xattr bucket is filled up with the same hash value.
> - * If we want to insert the xattr with the same hash, return -ENOSPC.
> - * If we want to insert a xattr with different hash value, go ahead
> - * and ocfs2_divide_xattr_bucket will handle this.
> + * If the entry being set carries that same hash, return -ENOSPC, since
> + * ocfs2_divide_xattr_bucket() has no divide position to work with.
> + * Otherwise go ahead and ocfs2_divide_xattr_bucket() will handle this.
>   */
> -static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
> -					      struct ocfs2_xattr_bucket *bucket,
> -					      const char *name)
> +static int ocfs2_check_xattr_bucket_collision(struct ocfs2_xattr_bucket *bucket,
> +					      u32 name_hash)
>  {
>  	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
> -	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
>  
>  	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
>  		return 0;
> @@ -5974,6 +5972,7 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>  					     struct ocfs2_xattr_search *xs,
>  					     struct ocfs2_xattr_set_ctxt *ctxt)
>  {
> +	u32 name_hash;
>  	int ret;
>  
>  	trace_ocfs2_xattr_set_entry_index_block(xi->xi_name);
> @@ -5993,10 +5992,18 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>  	 * the maximum number of collisions we will allow for then is
>  	 * one bucket's worth, so check it here whether we need to
>  	 * add a new bucket for the insert.
> +	 *
> +	 * An existing entry keeps the hash it was stored under, and that is
> +	 * the hash a split has to work with.  A new entry is stored under the
> +	 * unsigned one, which is what ocfs2_xa_add_entry() will write.
>  	 */
> -	ret = ocfs2_check_xattr_bucket_collision(inode,
> -						 xs->bucket,
> -						 xi->xi_name);
> +	if (xs->not_found)
> +		name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
> +						  xi->xi_name_len);
> +	else
> +		name_hash = le32_to_cpu(xs->here->xe_name_hash);
> +
> +	ret = ocfs2_check_xattr_bucket_collision(xs->bucket, name_hash);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out;
> -- 
> 2.39.3
> 

Re: [PATCH v2 2/3] ocfs2: use the stored hash when checking xattr bucket collision

[Heming Zhao]

On Fri, Oct 09, 2026 at 04:30:01PM +0800, Joseph Qi wrote:
> ocfs2_check_xattr_bucket_collision() decides whether splitting a full
> bucket can make room for the entry being set by recomputing the hash of
> the name:
> 
> 	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
> 
> 	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
> 		return 0;
> 
> For a new entry that is the hash it will be stored under, so comparing it
> is correct.  An existing entry keeps the hash it already has: an update
> never rewrites xe_name_hash, since ocfs2_xa_add_entry() is the only
> writer of that field for a real entry and ocfs2_xa_prepare_entry() calls
> it only when loc->xl_entry is NULL.
> 
> An entry stored by a kernel that sign-extended the name bytes when
> hashing can still be updated, because ocfs2_xattr_find_entry() searches
> a non-indexed xattr block by memcmp on the name and never looks at
> xe_name_hash.  Growing it past the space left in the block converts the
> block into a tree, ocfs2_cp_xattr_block_to_bucket() fills the bucket in
> stored hash order, and the update runs out of room in the bucket too.
> The collision check then compares the unsigned hash against the legacy
> hashes in the bucket and reports no collision.
> 
> ocfs2_xattr_set_entry_index_block() goes on to allocate a bucket that
> ocfs2_divide_xattr_bucket() cannot fill: a bucket whose entries all share
> one hash has no divide position, so all it does is append an empty bucket
> with a sentinel hash one above the last entry's.
> 
> The re-search that follows depends on where the unsigned hash sorts.
> Below the legacy ones, it comes back to the full bucket and the set fails
> with -ENOSPC, having grown the tree for nothing.  Above them, it lands on
> the new empty bucket, which ocfs2_xattr_bucket_find() handles explicitly
> and ocfs2_find_xe_in_bucket() scans zero times, so the set stores a second
> copy of the entry under the unsigned hash and returns success.  The
> original stays in the full bucket, listxattr reports the name twice and
> getxattr returns the new copy.
> 
> Pass the hash the entry is stored under instead: the stored one for an
> existing entry, the unsigned one for a new entry.  A tree whose entries
> are all stored under the unsigned hash sees no change, since there the
> two are the same value.
> 
> Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
> Cc: <stable@vger.kernel.org> # 6.2+
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

LGTM.
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/xattr.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index a428fe908116..c5a39a7d43d0 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -5896,16 +5896,14 @@ static int ocfs2_rm_xattr_cluster(struct inode *inode,
>  
>  /*
>   * check whether the xattr bucket is filled up with the same hash value.
> - * If we want to insert the xattr with the same hash, return -ENOSPC.
> - * If we want to insert a xattr with different hash value, go ahead
> - * and ocfs2_divide_xattr_bucket will handle this.
> + * If the entry being set carries that same hash, return -ENOSPC, since
> + * ocfs2_divide_xattr_bucket() has no divide position to work with.
> + * Otherwise go ahead and ocfs2_divide_xattr_bucket() will handle this.
>   */
> -static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
> -					      struct ocfs2_xattr_bucket *bucket,
> -					      const char *name)
> +static int ocfs2_check_xattr_bucket_collision(struct ocfs2_xattr_bucket *bucket,
> +					      u32 name_hash)
>  {
>  	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
> -	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
>  
>  	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
>  		return 0;
> @@ -5974,6 +5972,7 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>  					     struct ocfs2_xattr_search *xs,
>  					     struct ocfs2_xattr_set_ctxt *ctxt)
>  {
> +	u32 name_hash;
>  	int ret;
>  
>  	trace_ocfs2_xattr_set_entry_index_block(xi->xi_name);
> @@ -5993,10 +5992,18 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>  	 * the maximum number of collisions we will allow for then is
>  	 * one bucket's worth, so check it here whether we need to
>  	 * add a new bucket for the insert.
> +	 *
> +	 * An existing entry keeps the hash it was stored under, and that is
> +	 * the hash a split has to work with.  A new entry is stored under the
> +	 * unsigned one, which is what ocfs2_xa_add_entry() will write.
>  	 */
> -	ret = ocfs2_check_xattr_bucket_collision(inode,
> -						 xs->bucket,
> -						 xi->xi_name);
> +	if (xs->not_found)
> +		name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
> +						  xi->xi_name_len);
> +	else
> +		name_hash = le32_to_cpu(xs->here->xe_name_hash);
> +
> +	ret = ocfs2_check_xattr_bucket_collision(xs->bucket, name_hash);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out;
> -- 
> 2.39.3
> 

Re: [PATCH v2 3/3] ocfs2: deal with legacy signed xattr name hash values

[Heming Zhao]

On Fri, Oct 09, 2026 at 04:30:02PM +0800, Joseph Qi wrote:
> Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") set
> -funsigned-char globally, which changed the result of the naked 'char'
> load in ocfs2_xattr_name_hash():
> 
> 	hash = (hash << OCFS2_HASH_SHIFT) ^
> 	       (hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT)) ^
> 	       *name++;
> 
> A name byte >= 0x80 used to sign-extend and now zero-extends, so the
> hash no longer matches the xe_name_hash an older kernel stored.  An
> indexed xattr tree is searched by that hash alone, and both the bucket
> binary search and the entry scan within it stop as soon as the wanted
> hash falls below an entry's, so the entry is never reached: getxattr,
> setxattr and removexattr return -ENODATA for a name that listxattr
> still lists.
> 
> Search with the current unsigned hash and, on a miss, retry with the
> legacy signed one, as ext4 does in commit f3bbac32475b2 ("ext4: deal
> with legacy signed xattr name hash values").  New entries are always
> stored under the unsigned hash.  Skip the retry when the two hashes are
> equal, so that a miss on an ASCII name does not walk the tree twice.
> 
> A miss is not empty handed: ocfs2_xattr_bucket_find() leaves xs->bucket
> holding the bucket a new entry would go into.  So after a double miss
> drop it and search once more with the unsigned hash, otherwise a new
> entry would be placed by its legacy hash and stored under its unsigned
> one, breaking the ordering the search relies on.
> 
> Only indexed trees are affected; inline xattrs and non-indexed xattr
> blocks compare names with memcmp and never look at the hash.
> 
> Also spell out the signedness instead of leaving the current hash to
> -funsigned-char, as commit 854f0912f813 ("ext4: make xattr char
> unsignedness in hash explicit") did, so that both variants stay correct
> if this is backported to a kernel without the flag.
> 
> Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
> Cc: <stable@vger.kernel.org> # 6.2+
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

LGTM.
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/xattr.c | 93 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 82 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index c5a39a7d43d0..e92bcf401490 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -601,9 +601,10 @@ static inline const char *ocfs2_xattr_prefix(int name_index)
>  	return handler ? xattr_prefix(handler) : NULL;
>  }
>  
> -static u32 ocfs2_xattr_name_hash(struct inode *inode,
> -				 const char *name,
> -				 int name_len)
> +static u32 __ocfs2_xattr_name_hash(struct inode *inode,
> +				   const char *name,
> +				   int name_len,
> +				   bool legacy_signed)
>  {
>  	/* Get hash value of uuid from super block */
>  	u32 hash = OCFS2_SB(inode->i_sb)->uuid_hash;
> @@ -612,13 +613,30 @@ static u32 ocfs2_xattr_name_hash(struct inode *inode,
>  	/* hash extended attribute name */
>  	for (i = 0; i < name_len; i++) {
>  		hash = (hash << OCFS2_HASH_SHIFT) ^
> -		       (hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT)) ^
> -		       *name++;
> +		       (hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT));
> +		if (legacy_signed)
> +			hash ^= (signed char)name[i];
> +		else
> +			hash ^= (unsigned char)name[i];
>  	}
>  
>  	return hash;
>  }
>  
> +static u32 ocfs2_xattr_name_hash(struct inode *inode,
> +				 const char *name,
> +				 int name_len)
> +{
> +	return __ocfs2_xattr_name_hash(inode, name, name_len, false);
> +}
> +
> +static u32 ocfs2_xattr_name_hash_signed(struct inode *inode,
> +					const char *name,
> +					int name_len)
> +{
> +	return __ocfs2_xattr_name_hash(inode, name, name_len, true);
> +}
> +
>  static int ocfs2_xattr_entry_real_size(int name_len, size_t value_len)
>  {
>  	return namevalue_size(name_len, value_len) +
> @@ -4304,11 +4322,12 @@ static int ocfs2_xattr_bucket_find(struct inode *inode,
>  	return ret;
>  }
>  
> -static int ocfs2_xattr_index_block_find(struct inode *inode,
> -					struct buffer_head *root_bh,
> -					int name_index,
> -					const char *name,
> -					struct ocfs2_xattr_search *xs)
> +static int __ocfs2_xattr_index_block_find(struct inode *inode,
> +					  struct buffer_head *root_bh,
> +					  int name_index,
> +					  const char *name,
> +					  u32 name_hash,
> +					  struct ocfs2_xattr_search *xs)
>  {
>  	int ret;
>  	struct ocfs2_xattr_block *xb =
> @@ -4317,7 +4336,6 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
>  	struct ocfs2_extent_list *el = &xb_root->xt_list;
>  	u64 p_blkno = 0;
>  	u32 first_hash, num_clusters = 0;
> -	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
>  
>  	if (le16_to_cpu(el->l_next_free_rec) == 0)
>  		return -ENODATA;
> @@ -4348,6 +4366,59 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
>  	return ret;
>  }
>  
> +static int ocfs2_xattr_index_block_find(struct inode *inode,
> +					struct buffer_head *root_bh,
> +					int name_index,
> +					const char *name,
> +					struct ocfs2_xattr_search *xs)
> +{
> +	u32 name_hash, legacy_hash;
> +	int name_len = strlen(name);
> +	int ret;
> +
> +	name_hash = ocfs2_xattr_name_hash(inode, name, name_len);
> +
> +	ret = __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
> +					     name_hash, xs);
> +	if (ret != -ENODATA)
> +		return ret;
> +
> +	/*
> +	 * Nothing under the current hash.  The entry may have been stored by
> +	 * an older kernel, which sign-extended the name bytes when hashing.
> +	 * Skip the retry when the two hashes are equal, so that a name made
> +	 * only of ASCII does not have to walk the tree twice.
> +	 */
> +	legacy_hash = ocfs2_xattr_name_hash_signed(inode, name, name_len);
> +	if (legacy_hash == name_hash)
> +		return ret;
> +
> +	/*
> +	 * A miss still leaves xs->bucket holding the bucket a new entry would
> +	 * be inserted into, so drop it before searching again.
> +	 */
> +	ocfs2_xattr_bucket_relse(xs->bucket);
> +
> +	ret = __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
> +					     legacy_hash, xs);
> +	if (!ret) {
> +		pr_warn_once("ocfs2: xattr tree with signed name hash\n");
> +		return ret;
> +	}
> +	if (ret != -ENODATA)
> +		return ret;
> +
> +	/*
> +	 * Not under either hash.  Restore the unsigned placement, since that
> +	 * is where a new entry is stored: leaving the bucket where the legacy
> +	 * hash put it would break the ordering the search relies on.
> +	 */
> +	ocfs2_xattr_bucket_relse(xs->bucket);
> +
> +	return __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
> +					      name_hash, xs);
> +}
> +
>  static int ocfs2_iterate_xattr_buckets(struct inode *inode,
>  				       u64 blkno,
>  				       u32 clusters,
> -- 
> 2.39.3
>