Re: [PATCH v17 4/7] ext4: Reuse generic_ci_match for ci comparisons

[Eugen Hristev]

On 6/4/24 22:17, Gabriel Krisman Bertazi wrote:
> Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> writes:
> 
>> From: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
>>
>> Instead of reimplementing ext4_match_ci, use the new libfs helper.
>>
>> It also adds a comment explaining why fname->cf_name.name must be
>> checked prior to the encryption hash optimization, because that tripped
>> me before.
>>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>> Reviewed-by: Eric Biggers <ebiggers@xxxxxxxxxx>
>> ---
>>  fs/ext4/namei.c | 91 +++++++++++++++----------------------------------
>>  1 file changed, 27 insertions(+), 64 deletions(-)
>>
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index ec4c9bfc1057..20668741a23c 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -1390,58 +1390,6 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
>>  }
>>  
>>  #if IS_ENABLED(CONFIG_UNICODE)
>> -/*
>> - * Test whether a case-insensitive directory entry matches the filename
>> - * being searched for.  If quick is set, assume the name being looked up
>> - * is already in the casefolded form.
>> - *
>> - * Returns: 0 if the directory entry matches, more than 0 if it
>> - * doesn't match or less than zero on error.
>> - */
>> -static int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
>> -			   u8 *de_name, size_t de_name_len, bool quick)
>> -{
>> -	const struct super_block *sb = parent->i_sb;
>> -	const struct unicode_map *um = sb->s_encoding;
>> -	struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
>> -	struct qstr entry = QSTR_INIT(de_name, de_name_len);
>> -	int ret;
>> -
>> -	if (IS_ENCRYPTED(parent)) {
>> -		const struct fscrypt_str encrypted_name =
>> -				FSTR_INIT(de_name, de_name_len);
>> -
>> -		decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
>> -		if (!decrypted_name.name)
>> -			return -ENOMEM;
>> -		ret = fscrypt_fname_disk_to_usr(parent, 0, 0, &encrypted_name,
>> -						&decrypted_name);
>> -		if (ret < 0)
>> -			goto out;
>> -		entry.name = decrypted_name.name;
>> -		entry.len = decrypted_name.len;
>> -	}
>> -
>> -	if (quick)
>> -		ret = utf8_strncasecmp_folded(um, name, &entry);
>> -	else
>> -		ret = utf8_strncasecmp(um, name, &entry);
>> -	if (ret < 0) {
>> -		/* Handle invalid character sequence as either an error
>> -		 * or as an opaque byte sequence.
>> -		 */
>> -		if (sb_has_strict_encoding(sb))
>> -			ret = -EINVAL;
>> -		else if (name->len != entry.len)
>> -			ret = 1;
>> -		else
>> -			ret = !!memcmp(name->name, entry.name, entry.len);
>> -	}
>> -out:
>> -	kfree(decrypted_name.name);
>> -	return ret;
>> -}
>> -
>>  int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
>>  				  struct ext4_filename *name)
>>  {
>> @@ -1503,20 +1451,35 @@ static bool ext4_match(struct inode *parent,
>>  #if IS_ENABLED(CONFIG_UNICODE)
>>  	if (IS_CASEFOLDED(parent) &&
>>  	    (!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
>> -		if (fname->cf_name.name) {
>> -			if (IS_ENCRYPTED(parent)) {
>> -				if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
>> -					fname->hinfo.minor_hash !=
>> -						EXT4_DIRENT_MINOR_HASH(de)) {
>> +		int ret;
>>  
>> -					return false;
>> -				}
>> -			}
>> -			return !ext4_ci_compare(parent, &fname->cf_name,
>> -						de->name, de->name_len, true);
>> +		/*
>> +		 * Just checking IS_ENCRYPTED(parent) below is not
>> +		 * sufficient to decide whether one can use the hash for
>> +		 * skipping the string comparison, because the key might
>> +		 * have been added right after
>> +		 * ext4_fname_setup_ci_filename().  In this case, a hash
>> +		 * mismatch will be a false negative.  Therefore, make
>> +		 * sure cf_name was properly initialized before
>> +		 * considering the calculated hash.
>> +		 */
>> +		if (IS_ENCRYPTED(parent) && fname->cf_name.name &&
>> +		    (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
>> +		     fname->hinfo.minor_hash != EXT4_DIRENT_MINOR_HASH(de)))
>> +			return false;
>> +
>> +		ret = generic_ci_match(parent, fname->usr_fname,
>> +				       &fname->cf_name, de->name,
>> +				       de->name_len);
>> +		if (ret < 0) {
>> +			/*
>> +			 * Treat comparison errors as not a match.  The
>> +			 * only case where it happens is on a disk
>> +			 * corruption or ENOMEM.
>> +			 */
>> +			return false;
>>  		}
>> -		return !ext4_ci_compare(parent, fname->usr_fname, de->name,
>> -						de->name_len, false);
> 
> With the changes to patch 3 in this iteration, This could become:
> 
> /*
>  * Treat comparison errors as not a match.  The
>  * only case where it happens is disk corruption
>  * or ENOMEM.
>  */
> return ext4_ci_compare(parent, fname->usr_fname, de->name,
> 		       de->name_len, false) > 0;
> 

Do you mean

return generic_ci_match(parent, fname->usr_fname,
		       &fname->cf_name, de->name,
		       de->name_len) > 0;

?

Because ext4_ci_compare was obsoleted with this series.

Thanks,
Eugen