From: Gabriel Krisman Bertazi <krisman@collabora.com>
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@collabora.com>
Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
Reviewed-by: Eric Biggers <ebiggers@google.com>
---
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);
+ return ret;
}
#endif
--
2.34.1Eugen Hristev <eugen.hristev@collabora.com> writes:
> From: Gabriel Krisman Bertazi <krisman@collabora.com>
>
> 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@collabora.com>
> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
> Reviewed-by: Eric Biggers <ebiggers@google.com>
> ---
> 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;
--
Gabriel Krisman Bertazi
On 6/4/24 22:17, Gabriel Krisman Bertazi wrote:
> Eugen Hristev <eugen.hristev@collabora.com> writes:
>
>> From: Gabriel Krisman Bertazi <krisman@collabora.com>
>>
>> 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@collabora.com>
>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>> Reviewed-by: Eric Biggers <ebiggers@google.com>
>> ---
>> 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
© 2016 - 2025 Red Hat, Inc.