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

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

On Thu, Oct 08, 2026 at 08:27:42PM +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 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>

LGTM.
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
>