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

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 10/9/26 11:18 AM, Heming Zhao wrote:
> 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@vger.kernel.org # 6.2+
>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> I agree with Sashiko review comment, the ocfs2_check_xattr_bucket_collision()
> also requires the same fix.
> 

I am looking into it.
It seems we have to use the stored hash for an entry that already exists.
I'll send v2 with a third patch to address this comments.

Thanks,
Joseph