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@xxxxxxxxxx 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@xxxxxxxxxxxxxxxxx>
> 
>     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