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

[Heming Zhao <heming.zhao@suse.com>]

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
>