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

[Heming Zhao]

On Thu, Oct 08, 2026 at 08:27:43PM +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@xxxxxxxxxxxxxxx # 6.2+
> Signed-off-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>

I agree with Sashiko review comment, the ocfs2_check_xattr_bucket_collision()
also requires the same fix.

Thanks,
Heming
> ---
>  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 a428fe908116..0834883ae971 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
>