Skip to contentSkip to conversation

Clean up the case-insensitive lookup path

PATCH Superseded Gabriel Krisman Bertazi 3 participants 21 messages May 11, 2022 – May 18, 2022

Subsystems (3)
VFS ext4 file system F2FS file system

v4 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Gabriel Krisman Bertazi

author
#0 lore raw

v4 cover Clean up the case-insensitive lookup path

The case-insensitive implementations in f2fs and ext4 have quite a bit of duplicated code. This series simplifies the ext4 version, with the goal of extracting ext4_ci_compare into a helper library that can be used by both filesystems. It also reduces the clutter from many codeguards for CONFIG_UNICODE; as requested by Linus, they are part of the codeflow now.

While there, I noticed we can leverage the utf8 functions to detect encoded names that are corrupted in the filesystem. Therefore, it also adds an ext4 error on that scenario, to mark the filesystem as corrupted.

This series survived passes of xfstests -g quick.

Gabriel Krisman Bertazi (10):
ext4: Match the f2fs ci_compare implementation
ext4: Simplify the handling of cached insensitive names
f2fs: Simplify the handling of cached insensitive names
ext4: Implement ci comparison using unicode_name
ext4: Simplify hash check on ext4_match
ext4: Log error when lookup of encoded dentry fails
ext4: Move ext4_match_ci into libfs
f2fs: Reuse generic_ci_match for ci comparisons
ext4: Move CONFIG_UNICODE defguards into the code flow
f2fs: Move CONFIG_UNICODE defguards into the code flow

 fs/ext4/ext4.h     |  41 +++++++--------
 fs/ext4/namei.c    | 126 ++++++++++++++-------------------------------
 fs/ext4/super.c    |   4 +-
 fs/f2fs/dir.c      | 103 ++++++++++++------------------------
 fs/f2fs/f2fs.h     |   3 +-
 fs/f2fs/namei.c    |  12 ++---
 fs/f2fs/recovery.c |   5 +-
 fs/f2fs/super.c    |  22 ++++----
 fs/libfs.c         |  61 ++++++++++++++++++++++
 include/linux/fs.h |   8 +++
 10 files changed, 185 insertions(+), 200 deletions(-)
Signature · 1 line
2.36.1
author
#1 lore raw

PATCH v4 1/10 ext4: Match the f2fs ci_compare implementation

ext4_ci_compare originally follows utf8_*_strcmp, which means return zero on match. This means that every usage of that in ext4 negates the return.

Turn it into a predicate function, let it follow the kernel convention and return true on match, which means it's now the same as its f2fs counterpart and can be extracted into generic code.

This change also makes it more obvious that we are ignoring error handling in ext4_match, which can occur since casefolding support (bad utf8 name due to disk corruption on strict mode causes -EINVAL) and casefold+encryption (-ENOMEM). For now, keep the behavior. It is handled by the following patches.

While we are there, change the comment to the kernel-doc style.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
changes since v3:
- move variable into code guard (lkp)
changes since v1:
- rename to match f2fs naming (Eric)

---
 fs/ext4/namei.c | 66 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 22 deletions(-)
fs/ext4/namei.c +44 -22
index 767b4bfe39c3..59eb3ecfdea7 100644
@@ -1318,22 +1318,29 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
 }
 
 #if IS_ENABLED(CONFIG_UNICODE)
-/*
+/**
+ * ext4_match_ci() - Match (case-insensitive) a name with a dirent.
+ * @parent: Inode of the parent of the dentry.
+ * @name: name under lookup.
+ * @de_name: Dirent name.
+ * @de_name_len: dirent name length.
+ * @quick: whether @name is already casefolded.
+ *
  * 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.
+ * being searched.  If quick is set, 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.
+ * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
+ * < 0 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)
+static int ext4_match_ci(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;
+	int ret, match = false;
 
 	if (IS_ENCRYPTED(parent)) {
 		const struct fscrypt_str encrypted_name =

@@ -1354,20 +1361,22 @@ static int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
 		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 (!ret)
+		match = true;
+	else if (ret < 0 && !sb_has_strict_encoding(sb)) {
+		/*
+		 * In non-strict mode, fallback to a byte comparison if
+		 * the names have invalid characters.
 		 */
-		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);
+		ret = 0;
+		match = ((name->len == entry.len) &&
+			 !memcmp(name->name, entry.name, entry.len));
 	}
+
 out:
 	kfree(decrypted_name.name);
-	return ret;
+	return (ret >= 0) ? match : ret;
 }
 
 int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,

@@ -1431,6 +1440,8 @@ static bool ext4_match(struct inode *parent,
 #if IS_ENABLED(CONFIG_UNICODE)
 	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent) &&
 	    (!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
+		int ret;
+
 		if (fname->cf_name.name) {
 			struct qstr cf = {.name = fname->cf_name.name,
 					  .len = fname->cf_name.len};

@@ -1442,11 +1453,22 @@ static bool ext4_match(struct inode *parent,
 					return false;
 				}
 			}
-			return !ext4_ci_compare(parent, &cf, de->name,
-							de->name_len, true);
+			ret = ext4_match_ci(parent, &cf, de->name,
+					    de->name_len, true);
+		} else {
+			ret = ext4_match_ci(parent, fname->usr_fname,
+					    de->name, de->name_len, false);
+		}
+
+		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
Signature · 1 line
2.36.1
author
#2 lore raw

PATCH v4 2/10 ext4: Simplify the handling of cached insensitive names

In reply to Gabriel Krisman Bertazi

Keeping it as qstr avoids the unnecessary conversion in ext4_match

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v1:
- Simplify hunk (eric)

---
 fs/ext4/ext4.h  |  2 +-
 fs/ext4/namei.c | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)
fs/ext4/ext4.h +1 -1
index a743b1e3b89e..93a28fcb2e22 100644
@@ -2490,7 +2490,7 @@ struct ext4_filename {
 	struct fscrypt_str crypto_buf;
 #endif
 #if IS_ENABLED(CONFIG_UNICODE)
-	struct fscrypt_str cf_name;
+	struct qstr cf_name;
 #endif
 };
fs/ext4/namei.c +11 -11
index 59eb3ecfdea7..84fdb23f09b8 100644
@@ -1382,7 +1382,8 @@ static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
 int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
 				  struct ext4_filename *name)
 {
-	struct fscrypt_str *cf_name = &name->cf_name;
+	struct qstr *cf_name = &name->cf_name;
+	unsigned char *buf;
 	struct dx_hash_info *hinfo = &name->hinfo;
 	int len;
 

@@ -1392,18 +1393,18 @@ int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
 		return 0;
 	}
 
-	cf_name->name = kmalloc(EXT4_NAME_LEN, GFP_NOFS);
-	if (!cf_name->name)
+	buf = kmalloc(EXT4_NAME_LEN, GFP_NOFS);
+	if (!buf)
 		return -ENOMEM;
 
-	len = utf8_casefold(dir->i_sb->s_encoding,
-			    iname, cf_name->name,
-			    EXT4_NAME_LEN);
+	len = utf8_casefold(dir->i_sb->s_encoding, iname, buf, EXT4_NAME_LEN);
 	if (len <= 0) {
-		kfree(cf_name->name);
-		cf_name->name = NULL;
+		kfree(buf);
+		buf = NULL;
 	}
+	cf_name->name = buf;
 	cf_name->len = (unsigned) len;
+
 	if (!IS_ENCRYPTED(dir))
 		return 0;
 

@@ -1443,8 +1444,6 @@ static bool ext4_match(struct inode *parent,
 		int ret;
 
 		if (fname->cf_name.name) {
-			struct qstr cf = {.name = fname->cf_name.name,
-					  .len = fname->cf_name.len};
 			if (IS_ENCRYPTED(parent)) {
 				if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
 					fname->hinfo.minor_hash !=

@@ -1453,7 +1452,8 @@ static bool ext4_match(struct inode *parent,
 					return false;
 				}
 			}
-			ret = ext4_match_ci(parent, &cf, de->name,
+
+			ret = ext4_match_ci(parent, &fname->cf_name, de->name,
 					    de->name_len, true);
 		} else {
 			ret = ext4_match_ci(parent, fname->usr_fname,
Signature · 1 line
2.36.1
author
#3 lore raw

PATCH v4 3/10 f2fs: Simplify the handling of cached insensitive names

In reply to Gabriel Krisman Bertazi

Keeping it as qstr avoids the unnecessary conversion in f2fs_match

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/f2fs/dir.c      | 52 ++++++++++++++++++++++++++++------------------
 fs/f2fs/f2fs.h     |  3 ++-
 fs/f2fs/recovery.c |  5 +----
 3 files changed, 35 insertions(+), 25 deletions(-)
fs/f2fs/dir.c +32 -20
index 166f08623362..c2a02003c5b9 100644
@@ -81,28 +81,47 @@ int f2fs_init_casefolded_name(const struct inode *dir,
 {
 #if IS_ENABLED(CONFIG_UNICODE)
 	struct super_block *sb = dir->i_sb;
+	unsigned char *buf;
+	int len;
 
 	if (IS_CASEFOLDED(dir)) {
-		fname->cf_name.name = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
+		buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
 					GFP_NOFS, false, F2FS_SB(sb));
-		if (!fname->cf_name.name)
-			return -ENOMEM;
-		fname->cf_name.len = utf8_casefold(sb->s_encoding,
-						   fname->usr_fname,
-						   fname->cf_name.name,
-						   F2FS_NAME_LEN);
-		if ((int)fname->cf_name.len <= 0) {
-			kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
+		if (!buf) {
 			fname->cf_name.name = NULL;
+			return -ENOMEM;
+		}
+
+		len = utf8_casefold(sb->s_encoding, fname->usr_fname,
+				    buf, F2FS_NAME_LEN);
+
+		if (len <= 0) {
+			kmem_cache_free(f2fs_cf_name_slab, buf);
+			buf = NULL;
 			if (sb_has_strict_encoding(sb))
 				return -EINVAL;
 			/* fall back to treating name as opaque byte sequence */
 		}
+		fname->cf_name.name = buf;
+		fname->cf_name.len = (unsigned int) len;
 	}
 #endif
 	return 0;
 }
 
+void f2fs_free_casefolded_name(struct f2fs_filename *fname)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	unsigned char *buf = (unsigned char *)fname->cf_name.name;
+
+	if (buf) {
+		kmem_cache_free(f2fs_cf_name_slab, buf);
+		fname->cf_name.name = NULL;
+	}
+
+#endif
+}
+
 static int __f2fs_setup_filename(const struct inode *dir,
 				 const struct fscrypt_name *crypt_name,
 				 struct f2fs_filename *fname)

@@ -174,12 +193,7 @@ void f2fs_free_filename(struct f2fs_filename *fname)
 	kfree(fname->crypto_buf.name);
 	fname->crypto_buf.name = NULL;
 #endif
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (fname->cf_name.name) {
-		kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
-		fname->cf_name.name = NULL;
-	}
-#endif
+	f2fs_free_casefolded_name(fname);
 }
 
 static unsigned long dir_block_index(unsigned int level,

@@ -267,11 +281,9 @@ static inline int f2fs_match_name(const struct inode *dir,
 	struct fscrypt_name f;
 
 #if IS_ENABLED(CONFIG_UNICODE)
-	if (fname->cf_name.name) {
-		struct qstr cf = FSTR_TO_QSTR(&fname->cf_name);
-
-		return f2fs_match_ci_name(dir, &cf, de_name, de_name_len);
-	}
+	if (fname->cf_name.name)
+		return f2fs_match_ci_name(dir, &fname->cf_name,
+					  de_name, de_name_len);
 #endif
 	f.usr_fname = fname->usr_fname;
 	f.disk_name = fname->disk_name;
fs/f2fs/f2fs.h +2 -1
index 68b44015514f..4b6df34fc8b0 100644
@@ -497,7 +497,7 @@ struct f2fs_filename {
 	 * NULL.  In all these cases we fall back to treating the name as an
 	 * opaque byte sequence.
 	 */
-	struct fscrypt_str cf_name;
+	struct qstr cf_name;
 #endif
 };
 

@@ -3345,6 +3345,7 @@ struct dentry *f2fs_get_parent(struct dentry *child);
 unsigned char f2fs_get_de_type(struct f2fs_dir_entry *de);
 int f2fs_init_casefolded_name(const struct inode *dir,
 			      struct f2fs_filename *fname);
+void f2fs_free_casefolded_name(struct f2fs_filename *fname);
 int f2fs_setup_filename(struct inode *dir, const struct qstr *iname,
 			int lookup, struct f2fs_filename *fname);
 int f2fs_prepare_lookup(struct inode *dir, struct dentry *dentry,
fs/f2fs/recovery.c +1 -4
index 79773d322c47..3c3a8abf6953 100644
@@ -149,11 +149,8 @@ static int init_recovered_filename(const struct inode *dir,
 		if (err)
 			return err;
 		f2fs_hash_filename(dir, fname);
-#if IS_ENABLED(CONFIG_UNICODE)
 		/* Case-sensitive match is fine for recovery */
-		kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
-		fname->cf_name.name = NULL;
-#endif
+		f2fs_free_casefolded_name(fname);
 	} else {
 		f2fs_hash_filename(dir, fname);
 	}
Signature · 1 line
2.36.1
#4 lore raw

Re: [PATCH v4 03/10] f2fs: Simplify the handling of cached insensitive names

Quoted 41 lines · Gabriel Krisman Bertazi

Keeping it as qstr avoids the unnecessary conversion in f2fs_match

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/f2fs/dir.c      | 52 ++++++++++++++++++++++++++++------------------
 fs/f2fs/f2fs.h     |  3 ++-
 fs/f2fs/recovery.c |  5 +----
 3 files changed, 35 insertions(+), 25 deletions(-)
fs/f2fs/dir.c +11 -9
index 166f08623362..c2a02003c5b9 100644
@@ -81,28 +81,47 @@ int f2fs_init_casefolded_name(const struct inode *dir,
 {
 #if IS_ENABLED(CONFIG_UNICODE)
 	struct super_block *sb = dir->i_sb;
+	unsigned char *buf;
+	int len;
 
 	if (IS_CASEFOLDED(dir)) {
-		fname->cf_name.name = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
+		buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
 					GFP_NOFS, false, F2FS_SB(sb));
-		if (!fname->cf_name.name)
-			return -ENOMEM;
-		fname->cf_name.len = utf8_casefold(sb->s_encoding,
-						   fname->usr_fname,
-						   fname->cf_name.name,
-						   F2FS_NAME_LEN);
-		if ((int)fname->cf_name.len <= 0) {
-			kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
+		if (!buf) {
 			fname->cf_name.name = NULL;
+			return -ENOMEM;
+		}
+
+		len = utf8_casefold(sb->s_encoding, fname->usr_fname,
+				    buf, F2FS_NAME_LEN);
+
+		if (len <= 0) {

fs/f2fs/dir.c int f2fs_init_casefolded_name(const struct inode *dir, · line 108

Quoted 10 lines
fs/f2fs/dir.c +4 -0
+			kmem_cache_free(f2fs_cf_name_slab, buf);
+			buf = NULL;
 			if (sb_has_strict_encoding(sb))
 				return -EINVAL;
 			/* fall back to treating name as opaque byte sequence */
 		}
+		fname->cf_name.name = buf;
+		fname->cf_name.len = (unsigned int) len;
 	}
 #endif

There's some inconsistent behavior above; now sometimes fname->cf_name.name is set to NULL on failure and sometime it's not. Also now fname->cf_name.len can be set to a negative value.

Since struct f2fs_filename is always zero-initialized, how about only setting the fname->cf_name fields if we actually have a valid value to assign? I.e.

		buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
					GFP_NOFS, false, F2FS_SB(sb));
		if (!buf)
			return -ENOMEM;

		len = utf8_casefold(sb->s_encoding, fname->usr_fname,
				    buf, F2FS_NAME_LEN);
		if (len <= 0) {
			kmem_cache_free(f2fs_cf_name_slab, buf);
			if (sb_has_strict_encoding(sb))
				return -EINVAL;
			/* fall back to treating name as opaque byte sequence */
			return 0;
		}
		fname->cf_name.name = buf;
		fname->cf_name.len = len;

fs/f2fs/dir.c int f2fs_init_casefolded_name(const struct inode *dir, · line 123

Quoted 12 lines
fs/f2fs/dir.c +12 -0
+void f2fs_free_casefolded_name(struct f2fs_filename *fname)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	unsigned char *buf = (unsigned char *)fname->cf_name.name;
+
+	if (buf) {
+		kmem_cache_free(f2fs_cf_name_slab, buf);
+		fname->cf_name.name = NULL;
+	}
+
+#endif
+}

Kernel code usually uses static inline stubs for the !CONFIG_$FOO case in cases like this, as that causes the function calls to be compiled away to nothing when they're unneeded.

- Eric

author
#5 lore raw

PATCH v4 4/10 ext4: Implement ci comparison using unicode_name

In reply to Gabriel Krisman Bertazi

By using a new type here, we can hide most of the caching casefold logic from ext4. The condition in ext4_match is now quite redundant, but this is addressed in the next patch.

This doesn't use ext4_filename to keep it generic, since the function will be moved to libfs to be shared with f2fs.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v1:
- Instead of (ab)using fscrypt_name, create a new type (ebiggers).

---
 fs/ext4/namei.c    | 32 +++++++++++++++-----------------
 include/linux/fs.h |  5 +++++
 2 files changed, 20 insertions(+), 17 deletions(-)
fs/ext4/namei.c +15 -17
index 84fdb23f09b8..5296ced2e43e 100644
@@ -1321,20 +1321,19 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
 /**
  * ext4_match_ci() - Match (case-insensitive) a name with a dirent.
  * @parent: Inode of the parent of the dentry.
- * @name: name under lookup.
+ * @uname: name under lookup.
  * @de_name: Dirent name.
  * @de_name_len: dirent name length.
- * @quick: whether @name is already casefolded.
  *
  * Test whether a case-insensitive directory entry matches the filename
- * being searched.  If quick is set, the @name being looked up is
- * already in the casefolded form.
+ * being searched.
  *
  * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
  * < 0 on error.
  */
-static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
-			 u8 *de_name, size_t de_name_len, bool quick)
+static int ext4_match_ci(const struct inode *parent,
+			 const struct unicode_name *uname,
+			 u8 *de_name, size_t de_name_len)
 {
 	const struct super_block *sb = parent->i_sb;
 	const struct unicode_map *um = sb->s_encoding;

@@ -1357,10 +1356,10 @@ static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
 		entry.len = decrypted_name.len;
 	}
 
-	if (quick)
-		ret = utf8_strncasecmp_folded(um, name, &entry);
+	if (uname->folded_name->name)
+		ret = utf8_strncasecmp_folded(um, uname->folded_name, &entry);
 	else
-		ret = utf8_strncasecmp(um, name, &entry);
+		ret = utf8_strncasecmp(um, uname->usr_name, &entry);
 
 	if (!ret)
 		match = true;

@@ -1370,8 +1369,8 @@ static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
 		 * the names have invalid characters.
 		 */
 		ret = 0;
-		match = ((name->len == entry.len) &&
-			 !memcmp(name->name, entry.name, entry.len));
+		match = ((uname->usr_name->len == entry.len) &&
+			 !memcmp(uname->usr_name->name, entry.name, entry.len));
 	}
 
 out:

@@ -1441,6 +1440,10 @@ static bool ext4_match(struct inode *parent,
 #if IS_ENABLED(CONFIG_UNICODE)
 	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent) &&
 	    (!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
+		struct unicode_name u = {
+			.folded_name = &fname->cf_name,
+			.usr_name = fname->usr_fname
+		};
 		int ret;
 
 		if (fname->cf_name.name) {

@@ -1452,14 +1455,9 @@ static bool ext4_match(struct inode *parent,
 					return false;
 				}
 			}
-
-			ret = ext4_match_ci(parent, &fname->cf_name, de->name,
-					    de->name_len, true);
-		} else {
-			ret = ext4_match_ci(parent, fname->usr_fname,
-					    de->name, de->name_len, false);
 		}
 
+		ret = ext4_match_ci(parent, &u, de->name, de->name_len);
 		if (ret < 0) {
 			/*
 			 * Treat comparison errors as not a match.  The
include/linux/fs.h +5 -0
index e2d892b201b0..3f76a18a5f40 100644
@@ -3358,6 +3358,11 @@ extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
 
 extern int generic_check_addressable(unsigned, u64);
 
+struct unicode_name {
+	const struct qstr *folded_name;
+	const struct qstr *usr_name;
+};
+
 extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
 
 #ifdef CONFIG_MIGRATION
Signature · 1 line
2.36.1
#6 lore raw

Re: [PATCH v4 04/10] ext4: Implement ci comparison using unicode_name

Quoted 105 lines · Gabriel Krisman Bertazi

By using a new type here, we can hide most of the caching casefold logic from ext4. The condition in ext4_match is now quite redundant, but this is addressed in the next patch.

This doesn't use ext4_filename to keep it generic, since the function will be moved to libfs to be shared with f2fs.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v1:
- Instead of (ab)using fscrypt_name, create a new type (ebiggers).

---
 fs/ext4/namei.c    | 32 +++++++++++++++-----------------
 include/linux/fs.h |  5 +++++
 2 files changed, 20 insertions(+), 17 deletions(-)
fs/ext4/namei.c +15 -17
index 84fdb23f09b8..5296ced2e43e 100644
@@ -1321,20 +1321,19 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
 /**
  * ext4_match_ci() - Match (case-insensitive) a name with a dirent.
  * @parent: Inode of the parent of the dentry.
- * @name: name under lookup.
+ * @uname: name under lookup.
  * @de_name: Dirent name.
  * @de_name_len: dirent name length.
- * @quick: whether @name is already casefolded.
  *
  * Test whether a case-insensitive directory entry matches the filename
- * being searched.  If quick is set, the @name being looked up is
- * already in the casefolded form.
+ * being searched.
  *
  * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
  * < 0 on error.
  */
-static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
-			 u8 *de_name, size_t de_name_len, bool quick)
+static int ext4_match_ci(const struct inode *parent,
+			 const struct unicode_name *uname,
+			 u8 *de_name, size_t de_name_len)
 {
 	const struct super_block *sb = parent->i_sb;
 	const struct unicode_map *um = sb->s_encoding;

@@ -1357,10 +1356,10 @@ static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
 		entry.len = decrypted_name.len;
 	}
 
-	if (quick)
-		ret = utf8_strncasecmp_folded(um, name, &entry);
+	if (uname->folded_name->name)
+		ret = utf8_strncasecmp_folded(um, uname->folded_name, &entry);
 	else
-		ret = utf8_strncasecmp(um, name, &entry);
+		ret = utf8_strncasecmp(um, uname->usr_name, &entry);
 
 	if (!ret)
 		match = true;

@@ -1370,8 +1369,8 @@ static int ext4_match_ci(const struct inode *parent, const struct qstr *name,
 		 * the names have invalid characters.
 		 */
 		ret = 0;
-		match = ((name->len == entry.len) &&
-			 !memcmp(name->name, entry.name, entry.len));
+		match = ((uname->usr_name->len == entry.len) &&
+			 !memcmp(uname->usr_name->name, entry.name, entry.len));
 	}
 
 out:

@@ -1441,6 +1440,10 @@ static bool ext4_match(struct inode *parent,
 #if IS_ENABLED(CONFIG_UNICODE)
 	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent) &&
 	    (!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
+		struct unicode_name u = {
+			.folded_name = &fname->cf_name,
+			.usr_name = fname->usr_fname
+		};
 		int ret;
 
 		if (fname->cf_name.name) {

@@ -1452,14 +1455,9 @@ static bool ext4_match(struct inode *parent,
 					return false;
 				}
 			}
-
-			ret = ext4_match_ci(parent, &fname->cf_name, de->name,
-					    de->name_len, true);
-		} else {
-			ret = ext4_match_ci(parent, fname->usr_fname,
-					    de->name, de->name_len, false);
 		}
 
+		ret = ext4_match_ci(parent, &u, de->name, de->name_len);
 		if (ret < 0) {
 			/*
 			 * Treat comparison errors as not a match.  The
include/linux/fs.h +0 -0
index e2d892b201b0..3f76a18a5f40 100644
@@ -3358,6 +3358,11 @@ extern int generic_file_fsync(struct file *, loff_t, loff_t, int);

include/linux/fs.h extern int generic_file_fsync(struct file *, loff_t, loff_t, int); · line 3368

Quoted 10 lines
include/linux/fs.h +5 -0
 extern int generic_check_addressable(unsigned, u64);
 
+struct unicode_name {
+	const struct qstr *folded_name;
+	const struct qstr *usr_name;
+};
+
 extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
 
 #ifdef CONFIG_MIGRATION

I don't really see the point of this. The only times struct unicode_name gets used are when one is initialized on the stack for a single call to generic_ci_match(). So the end result is just that the function prototype is:

int generic_ci_match(const struct inode *parent,

		     const struct unicode_name *uname,
		     const u8 *de_name, size_t de_name_len);

... instead of:

int generic_ci_match(const struct inode *parent, const struct qstr *usr_fname,

		     const struct qstr *folded_name,
		     const u8 *de_name, size_t de_name_len);

So the only effect is to consolidate two parameters into one. I don't think it's worth it, given that the struct is being created on-demand.

Also note that filenames are not necessarily valid Unicode, so "unicode_name" is a bit misleading.

- Eric

author
#7 lore raw

PATCH v4 5/10 ext4: Simplify hash check on ext4_match

In reply to Gabriel Krisman Bertazi

The existence of fname->cf_name.name requires s_encoding & IS_CASEFOLDED, therefore this can be simplified.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/ext4/namei.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
fs/ext4/namei.c +7 -13
index 5296ced2e43e..cebbcabf0ff0 100644
@@ -1438,25 +1438,19 @@ static bool ext4_match(struct inode *parent,
 #endif
 
 #if IS_ENABLED(CONFIG_UNICODE)
-	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent) &&
-	    (!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
+	if (IS_ENCRYPTED(parent) && fname->cf_name.name) {
+		if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
+		    fname->hinfo.minor_hash != EXT4_DIRENT_MINOR_HASH(de))
+			return false;
+	}
+
+	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent)) {
 		struct unicode_name u = {
 			.folded_name = &fname->cf_name,
 			.usr_name = fname->usr_fname
 		};
 		int ret;
 
-		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)) {
-
-					return false;
-				}
-			}
-		}
-
 		ret = ext4_match_ci(parent, &u, de->name, de->name_len);
 		if (ret < 0) {
 			/*
Signature · 1 line
2.36.1
#8 lore raw

Re: [PATCH v4 05/10] ext4: Simplify hash check on ext4_match

Quoted 32 lines · Gabriel Krisman Bertazi

The existence of fname->cf_name.name requires s_encoding & IS_CASEFOLDED, therefore this can be simplified.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/ext4/namei.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
fs/ext4/namei.c +7 -3
index 5296ced2e43e..cebbcabf0ff0 100644
@@ -1438,25 +1438,19 @@ static bool ext4_match(struct inode *parent,
 #endif
 
 #if IS_ENABLED(CONFIG_UNICODE)
-	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent) &&
-	    (!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
+	if (IS_ENCRYPTED(parent) && fname->cf_name.name) {
+		if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
+		    fname->hinfo.minor_hash != EXT4_DIRENT_MINOR_HASH(de))
+			return false;
+	}
+
+	if (parent->i_sb->s_encoding && IS_CASEFOLDED(parent)) {
 		struct unicode_name u = {
 			.folded_name = &fname->cf_name,
 			.usr_name = fname->usr_fname
 		};
 		int ret;
 
-		if (fname->cf_name.name) {

fs/ext4/namei.c static bool ext4_match(struct inode *parent, · line 1459

Quoted 10 lines
fs/ext4/namei.c +0 -10
-			if (IS_ENCRYPTED(parent)) {
-				if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
-					fname->hinfo.minor_hash !=
-						EXT4_DIRENT_MINOR_HASH(de)) {
-
-					return false;
-				}
-			}
-		}
-

I don't think it's correct to delete the check for the encryption key here. If lookup is by no-key name, then fscrypt_match_name() must be used, not generic_ci_match(). And unlike f2fs, ext4 doesn't keep track of whether the whole lookup is by no-key name; ext4 relies on this fscrypt_has_encryption_key() check at the last minute when doing each individual comparison. (Which is not great, but that's how it works now.)

- Eric

author
#9 lore raw

PATCH v4 6/10 ext4: Log error when lookup of encoded dentry fails

In reply to Gabriel Krisman Bertazi

If the volume is in strict mode, ext4_ci_compare can report a broken encoding name. This will not trigger on a bad lookup, which is caught earlier, only if the actual disk name is bad.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---

Changes since v1:
- reword error message "file in directory" -> "filename" (Eric)

---
 fs/ext4/namei.c | 3 +++
 1 file changed, 3 insertions(+)
fs/ext4/namei.c +3 -0
index cebbcabf0ff0..708811525411 100644
@@ -1458,6 +1458,9 @@ static bool ext4_match(struct inode *parent,
 			 * only case where it happens is on a disk
 			 * corruption or ENOMEM.
 			 */
+			if (ret == -EINVAL)
+				EXT4_ERROR_INODE(parent,
+						 "Bad encoded filename");
 			return false;
 		}
 		return ret;
Signature · 1 line
2.36.1
#10 lore raw

Re: [PATCH v4 06/10] ext4: Log error when lookup of encoded dentry fails

Quoted 19 lines · Gabriel Krisman Bertazi

If the volume is in strict mode, ext4_ci_compare can report a broken encoding name. This will not trigger on a bad lookup, which is caught earlier, only if the actual disk name is bad.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---

Changes since v1:
- reword error message "file in directory" -> "filename" (Eric)

---
 fs/ext4/namei.c | 3 +++
 1 file changed, 3 insertions(+)
fs/ext4/namei.c +0 -0
index cebbcabf0ff0..708811525411 100644

fs/ext4/namei.c static bool ext4_match(struct inode *parent, · line 1463

Quoted 6 lines
fs/ext4/namei.c +3 -0
@@ -1458,6 +1458,9 @@ static bool ext4_match(struct inode *parent,
 			 * only case where it happens is on a disk
 			 * corruption or ENOMEM.
 			 */
+			if (ret == -EINVAL)
+				EXT4_ERROR_INODE(parent,
+						 "Bad encoded filename");

This message is still quite vague; perhaps it should be more specific about what a "bad" filename is? Maybe something like: "Directory contains filename that is not valid UTF-8" (or whatever the encoding being enforced is).

- Eric

author
#11 lore raw

PATCH v4 7/10 ext4: Move ext4_match_ci into libfs

In reply to Gabriel Krisman Bertazi

Matching case-insensitive names is a generic operation and can be shared with f2fs. Move it next to the rest of the shared casefold fs code.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/ext4/namei.c    | 62 +---------------------------------------------
 fs/libfs.c         | 61 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  3 +++
 3 files changed, 65 insertions(+), 61 deletions(-)
fs/ext4/namei.c +1 -61
index 708811525411..16fd0df5f8a8 100644
@@ -1318,66 +1318,6 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
 }
 
 #if IS_ENABLED(CONFIG_UNICODE)
-/**
- * ext4_match_ci() - Match (case-insensitive) a name with a dirent.
- * @parent: Inode of the parent of the dentry.
- * @uname: name under lookup.
- * @de_name: Dirent name.
- * @de_name_len: dirent name length.
- *
- * Test whether a case-insensitive directory entry matches the filename
- * being searched.
- *
- * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
- * < 0 on error.
- */
-static int ext4_match_ci(const struct inode *parent,
-			 const struct unicode_name *uname,
-			 u8 *de_name, size_t de_name_len)
-{
-	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, match = false;
-
-	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 (uname->folded_name->name)
-		ret = utf8_strncasecmp_folded(um, uname->folded_name, &entry);
-	else
-		ret = utf8_strncasecmp(um, uname->usr_name, &entry);
-
-	if (!ret)
-		match = true;
-	else if (ret < 0 && !sb_has_strict_encoding(sb)) {
-		/*
-		 * In non-strict mode, fallback to a byte comparison if
-		 * the names have invalid characters.
-		 */
-		ret = 0;
-		match = ((uname->usr_name->len == entry.len) &&
-			 !memcmp(uname->usr_name->name, entry.name, entry.len));
-	}
-
-out:
-	kfree(decrypted_name.name);
-	return (ret >= 0) ? match : ret;
-}
-
 int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
 				  struct ext4_filename *name)
 {

@@ -1451,7 +1391,7 @@ static bool ext4_match(struct inode *parent,
 		};
 		int ret;
 
-		ret = ext4_match_ci(parent, &u, de->name, de->name_len);
+		ret = generic_ci_match(parent, &u, de->name, de->name_len);
 		if (ret < 0) {
 			/*
 			 * Treat comparison errors as not a match.  The
fs/libfs.c +61 -0

67 lines not on this page · raw message

include/linux/fs.h +3 -0
index 3f76a18a5f40..6a750b8704c9 100644
@@ -3364,6 +3364,9 @@ struct unicode_name {
 };
 
 extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
+extern int generic_ci_match(const struct inode *parent,
+			    const struct unicode_name *uname, u8 *de_name,
+			    size_t de_name_len);
 
 #ifdef CONFIG_MIGRATION
 extern int buffer_migrate_page(struct address_space *,
Signature · 1 line
2.36.1
#12 lore raw

Re: [PATCH v4 07/10] ext4: Move ext4_match_ci into libfs

Quoted 9 lines · Gabriel Krisman Bertazi

Matching case-insensitive names is a generic operation and can be shared with f2fs. Move it next to the rest of the shared casefold fs code.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/ext4/namei.c    | 62 +---------------------------------------------
 fs/libfs.c         | 61 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  3 +++
 3 files changed, 65 insertions(+), 61 deletions(-)

It might be a good idea to split this into two patches, one for the libfs part and one for the ext4 part. That would make sorting out the dependencies of this series easier in case it doesn't all go in in one cycle.

Quoted 6 lines
fs/libfs.c +6 -0
+/**
+ * generic_ci_match() - Match (case-insensitive) a name with a dirent.
+ * @parent: Inode of the parent of the dentry.
+ * @uname: name under lookup.
+ * @de_name: Dirent name.
+ * @de_name_len: dirent name length.

fs/libfs.c static const struct dentry_operations generic_ci_dentry_ops = { · line 1484

Quoted 10 lines
fs/libfs.c +10 -0
+ *
+ * Test whether a case-insensitive directory entry matches the filename
+ * being searched.
+ *
+ * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
+ * < 0 on error.
+ */
+int generic_ci_match(const struct inode *parent,
+		     const struct unicode_name *uname,
+		     u8 *de_name, size_t de_name_len)

de_name should be const, like it is in the f2fs version. It does get cast away temporarily when it is stored in a fscrypt_str, but it never gets modified (and must not be) so const is appropriate.

- Eric

author
#13 lore raw

PATCH v4 8/10 f2fs: Reuse generic_ci_match for ci comparisons

In reply to Gabriel Krisman Bertazi

Now that ci_match is part of libfs, make f2fs reuse it instead of having a different implementation.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v3:
- fix unused variable iff !CONFIG_UNICODE (lkp)

---
 fs/f2fs/dir.c | 63 +++++++--------------------------------------------
 1 file changed, 8 insertions(+), 55 deletions(-)
fs/f2fs/dir.c +8 -55
index c2a02003c5b9..e284d6f5d0bf 100644
@@ -222,58 +222,6 @@ static struct f2fs_dir_entry *find_in_block(struct inode *dir,
 	return f2fs_find_target_dentry(&d, fname, max_slots);
 }
 
-#if IS_ENABLED(CONFIG_UNICODE)
-/*
- * Test whether a case-insensitive directory entry matches the filename
- * being searched for.
- *
- * Returns 1 for a match, 0 for no match, and -errno on an error.
- */
-static int f2fs_match_ci_name(const struct inode *dir, const struct qstr *name,
-			       const u8 *de_name, u32 de_name_len)
-{
-	const struct super_block *sb = dir->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 res;
-
-	if (IS_ENCRYPTED(dir)) {
-		const struct fscrypt_str encrypted_name =
-			FSTR_INIT((u8 *)de_name, de_name_len);
-
-		if (WARN_ON_ONCE(!fscrypt_has_encryption_key(dir)))
-			return -EINVAL;
-
-		decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
-		if (!decrypted_name.name)
-			return -ENOMEM;
-		res = fscrypt_fname_disk_to_usr(dir, 0, 0, &encrypted_name,
-						&decrypted_name);
-		if (res < 0)
-			goto out;
-		entry.name = decrypted_name.name;
-		entry.len = decrypted_name.len;
-	}
-
-	res = utf8_strncasecmp_folded(um, name, &entry);
-	/*
-	 * In strict mode, ignore invalid names.  In non-strict mode,
-	 * fall back to treating them as opaque byte sequences.
-	 */
-	if (res < 0 && !sb_has_strict_encoding(sb)) {
-		res = name->len == entry.len &&
-				memcmp(name->name, entry.name, name->len) == 0;
-	} else {
-		/* utf8_strncasecmp_folded returns 0 on match */
-		res = (res == 0);
-	}
-out:
-	kfree(decrypted_name.name);
-	return res;
-}
-#endif /* CONFIG_UNICODE */
-
 static inline int f2fs_match_name(const struct inode *dir,
 				   const struct f2fs_filename *fname,
 				   const u8 *de_name, u32 de_name_len)

@@ -281,9 +229,14 @@ static inline int f2fs_match_name(const struct inode *dir,
 	struct fscrypt_name f;
 
 #if IS_ENABLED(CONFIG_UNICODE)
-	if (fname->cf_name.name)
-		return f2fs_match_ci_name(dir, &fname->cf_name,
-					  de_name, de_name_len);
+	if (fname->cf_name.name) {
+		struct unicode_name u = {
+			.folded_name = &fname->cf_name,
+			.usr_name = fname->usr_fname,
+		};
+
+		return generic_ci_match(dir, &u, (u8 *) de_name, de_name_len);
+	}
 #endif
 	f.usr_fname = fname->usr_fname;
 	f.disk_name = fname->disk_name;
Signature · 8 lines
2.36.1



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
author
#14 lore raw

PATCH v4 9/10 ext4: Move CONFIG_UNICODE defguards into the code flow

In reply to Gabriel Krisman Bertazi

Instead of a bunch of ifdefs, make the unicode built checks part of the code flow where possible, as requested by Torvalds.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/ext4/ext4.h  | 39 +++++++++++++++++++--------------------
 fs/ext4/namei.c | 15 ++++++---------
 fs/ext4/super.c |  4 +---
 3 files changed, 26 insertions(+), 32 deletions(-)
fs/ext4/ext4.h +19 -20
index 93a28fcb2e22..e3c55a8e23bd 100644
@@ -2725,11 +2725,17 @@ extern unsigned ext4_free_clusters_after_init(struct super_block *sb,
 					      struct ext4_group_desc *gdp);
 ext4_fsblk_t ext4_inode_to_goal_block(struct inode *);
 
-#if IS_ENABLED(CONFIG_UNICODE)
 extern int ext4_fname_setup_ci_filename(struct inode *dir,
-					 const struct qstr *iname,
-					 struct ext4_filename *fname);
+					const struct qstr *iname,
+					struct ext4_filename *fname);
+
+static inline void ext4_fname_free_ci_filename(struct ext4_filename *fname)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	kfree(fname->cf_name.name);
+	fname->cf_name.name = NULL;
 #endif
+}
 
 #ifdef CONFIG_FS_ENCRYPTION
 static inline void ext4_fname_from_fscrypt_name(struct ext4_filename *dst,

@@ -2758,9 +2764,9 @@ static inline int ext4_fname_setup_filename(struct inode *dir,
 
 	ext4_fname_from_fscrypt_name(fname, &name);
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, iname, fname);
-#endif
+	if (IS_ENABLED(CONFIG_UNICODE))
+		err = ext4_fname_setup_ci_filename(dir, iname, fname);
+
 	return err;
 }
 

@@ -2777,9 +2783,9 @@ static inline int ext4_fname_prepare_lookup(struct inode *dir,
 
 	ext4_fname_from_fscrypt_name(fname, &name);
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
-#endif
+	if (IS_ENABLED(CONFIG_UNICODE))
+		err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
+
 	return err;
 }
 

@@ -2794,10 +2800,7 @@ static inline void ext4_fname_free_filename(struct ext4_filename *fname)
 	fname->usr_fname = NULL;
 	fname->disk_name.name = NULL;
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	kfree(fname->cf_name.name);
-	fname->cf_name.name = NULL;
-#endif
+	ext4_fname_free_ci_filename(fname);
 }
 #else /* !CONFIG_FS_ENCRYPTION */
 static inline int ext4_fname_setup_filename(struct inode *dir,

@@ -2810,9 +2813,8 @@ static inline int ext4_fname_setup_filename(struct inode *dir,
 	fname->disk_name.name = (unsigned char *) iname->name;
 	fname->disk_name.len = iname->len;
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, iname, fname);
-#endif
+	if (IS_ENABLED(CONFIG_UNICODE))
+		err = ext4_fname_setup_ci_filename(dir, iname, fname);
 
 	return err;
 }

@@ -2826,10 +2828,7 @@ static inline int ext4_fname_prepare_lookup(struct inode *dir,
 
 static inline void ext4_fname_free_filename(struct ext4_filename *fname)
 {
-#if IS_ENABLED(CONFIG_UNICODE)
-	kfree(fname->cf_name.name);
-	fname->cf_name.name = NULL;
-#endif
+	ext4_fname_free_ci_filename(fname);
 }
 #endif /* !CONFIG_FS_ENCRYPTION */
fs/ext4/namei.c +6 -9
index 16fd0df5f8a8..0892f9ee15cf 100644
@@ -1757,8 +1757,7 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 		}
 	}
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (!inode && IS_CASEFOLDED(dir)) {
+	if (IS_ENABLED(CONFIG_UNICODE) && !inode && IS_CASEFOLDED(dir)) {
 		/* Eventually we want to call d_add_ci(dentry, NULL)
 		 * for negative dentries in the encoding case as
 		 * well.  For now, prevent the negative dentry

@@ -1766,7 +1765,7 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 		 */
 		return NULL;
 	}
-#endif
+
 	return d_splice_alias(inode, dentry);
 }
 

@@ -3083,16 +3082,14 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
 	ext4_fc_track_unlink(handle, dentry);
 	retval = ext4_mark_inode_dirty(handle, dir);
 
-#if IS_ENABLED(CONFIG_UNICODE)
 	/* VFS negative dentries are incompatible with Encoding and
 	 * Case-insensitiveness. Eventually we'll want avoid
 	 * invalidating the dentries here, alongside with returning the
 	 * negative dentries at ext4_lookup(), when it is better
 	 * supported by the VFS for the CI case.
 	 */
-	if (IS_CASEFOLDED(dir))
+	if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
 		d_invalidate(dentry);
-#endif
 
 end_rmdir:
 	brelse(bh);

@@ -3188,16 +3185,16 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 	retval = __ext4_unlink(handle, dir, &dentry->d_name, d_inode(dentry));
 	if (!retval)
 		ext4_fc_track_unlink(handle, dentry);
-#if IS_ENABLED(CONFIG_UNICODE)
+
 	/* VFS negative dentries are incompatible with Encoding and
 	 * Case-insensitiveness. Eventually we'll want avoid
 	 * invalidating the dentries here, alongside with returning the
 	 * negative dentries at ext4_lookup(), when it is  better
 	 * supported by the VFS for the CI case.
 	 */
-	if (IS_CASEFOLDED(dir))
+	if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
 		d_invalidate(dentry);
-#endif
+
 	if (handle)
 		ext4_journal_stop(handle);
fs/ext4/super.c +1 -3
index 1847b46af808..fa0004459dd6 100644
@@ -3645,14 +3645,12 @@ int ext4_feature_set_ok(struct super_block *sb, int readonly)
 		return 0;
 	}
 
-#if !IS_ENABLED(CONFIG_UNICODE)
-	if (ext4_has_feature_casefold(sb)) {
+	if (!IS_ENABLED(CONFIG_UNICODE) && ext4_has_feature_casefold(sb)) {
 		ext4_msg(sb, KERN_ERR,
 			 "Filesystem with casefold feature cannot be "
 			 "mounted without CONFIG_UNICODE");
 		return 0;
 	}
-#endif
 
 	if (readonly)
 		return 1;
Signature · 8 lines
2.36.1



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
#15 lore raw

Re: [PATCH v4 09/10] ext4: Move CONFIG_UNICODE defguards into the code flow

Quoted 15 lines · Gabriel Krisman Bertazi

Instead of a bunch of ifdefs, make the unicode built checks part of the code flow where possible, as requested by Torvalds.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/ext4/ext4.h  | 39 +++++++++++++++++++--------------------
 fs/ext4/namei.c | 15 ++++++---------
 fs/ext4/super.c |  4 +---
 3 files changed, 26 insertions(+), 32 deletions(-)
fs/ext4/ext4.h +0 -0
index 93a28fcb2e22..e3c55a8e23bd 100644

fs/ext4/ext4.h extern unsigned ext4_free_clusters_after_init(struct super_block *sb, · line 2730

Quoted 9 lines
fs/ext4/ext4.h +2 -3
@@ -2725,11 +2725,17 @@ extern unsigned ext4_free_clusters_after_init(struct super_block *sb,
 					      struct ext4_group_desc *gdp);
 ext4_fsblk_t ext4_inode_to_goal_block(struct inode *);
 
-#if IS_ENABLED(CONFIG_UNICODE)
 extern int ext4_fname_setup_ci_filename(struct inode *dir,
-					 const struct qstr *iname,
-					 struct ext4_filename *fname);
+					const struct qstr *iname,
+					struct ext4_filename *fname);

I think this function should just have a !CONFIG_UNICODE stub that does nothing, so that the callers can just call it unconditionally and not have to gate their call on CONFIG_UNICODE themselves.

- Eric

author
#16 lore raw

PATCH v4 10/10 f2fs: Move CONFIG_UNICODE defguards into the code flow

In reply to Gabriel Krisman Bertazi

Instead of a bunch of ifdefs, make the unicode built checks part of the code flow where possible, as requested by Torvalds.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>
---
 fs/f2fs/namei.c | 12 ++++++------
 fs/f2fs/super.c | 22 ++++++++++++----------
 2 files changed, 18 insertions(+), 16 deletions(-)
fs/f2fs/namei.c +6 -6
index 5f213f05556d..843e4102347d 100644
@@ -561,8 +561,8 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 		goto out_iput;
 	}
 out_splice:
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (!inode && IS_CASEFOLDED(dir)) {
+
+	if (IS_ENABLED(CONFIG_UNICODE) && !inode && IS_CASEFOLDED(dir)) {
 		/* Eventually we want to call d_add_ci(dentry, NULL)
 		 * for negative dentries in the encoding case as
 		 * well.  For now, prevent the negative dentry

@@ -571,7 +571,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 		trace_f2fs_lookup_end(dir, dentry, ino, err);
 		return NULL;
 	}
-#endif
+
 	new = d_splice_alias(inode, dentry);
 	err = PTR_ERR_OR_ZERO(new);
 	trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err);

@@ -622,16 +622,16 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 		goto fail;
 	}
 	f2fs_delete_entry(de, page, dir, inode);
-#if IS_ENABLED(CONFIG_UNICODE)
+
 	/* VFS negative dentries are incompatible with Encoding and
 	 * Case-insensitiveness. Eventually we'll want avoid
 	 * invalidating the dentries here, alongside with returning the
 	 * negative dentries at f2fs_lookup(), when it is better
 	 * supported by the VFS for the CI case.
 	 */
-	if (IS_CASEFOLDED(dir))
+	if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
 		d_invalidate(dentry);
-#endif
+
 	f2fs_unlock_op(sbi);
 
 	if (IS_DIRSYNC(dir))
fs/f2fs/super.c +12 -10
index baefd398ec1a..c336760ff743 100644
@@ -278,12 +278,13 @@ f2fs_sb_read_encoding(const struct f2fs_super_block *sb)
 
 	return NULL;
 }
+#endif
 
 struct kmem_cache *f2fs_cf_name_slab;
 static int __init f2fs_create_casefold_cache(void)
 {
 	f2fs_cf_name_slab = f2fs_kmem_cache_create("f2fs_casefolded_name",
-							F2FS_NAME_LEN);
+						   F2FS_NAME_LEN);
 	if (!f2fs_cf_name_slab)
 		return -ENOMEM;
 	return 0;

@@ -293,10 +294,6 @@ static void f2fs_destroy_casefold_cache(void)
 {
 	kmem_cache_destroy(f2fs_cf_name_slab);
 }
-#else
-static int __init f2fs_create_casefold_cache(void) { return 0; }
-static void f2fs_destroy_casefold_cache(void) { }
-#endif
 
 static inline void limit_reserve_root(struct f2fs_sb_info *sbi)
 {

@@ -1259,13 +1256,13 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 		return -EINVAL;
 	}
 #endif
-#if !IS_ENABLED(CONFIG_UNICODE)
-	if (f2fs_sb_has_casefold(sbi)) {
+
+	if (!IS_ENABLED(CONFIG_UNICODE) && f2fs_sb_has_casefold(sbi)) {
 		f2fs_err(sbi,
 			"Filesystem with casefold feature cannot be mounted without CONFIG_UNICODE");
 		return -EINVAL;
 	}
-#endif
+
 	/*
 	 * The BLKZONED feature indicates that the drive was formatted with
 	 * zone alignment optimization. This is optional for host-aware

@@ -4611,7 +4608,10 @@ static int __init init_f2fs_fs(void)
 	err = f2fs_init_compress_cache();
 	if (err)
 		goto free_compress_mempool;
-	err = f2fs_create_casefold_cache();
+
+	if (IS_ENABLED(CONFIG_UNICODE))
+		err = f2fs_create_casefold_cache();
+
 	if (err)
 		goto free_compress_cache;
 	return 0;

@@ -4654,7 +4654,9 @@ static int __init init_f2fs_fs(void)
 
 static void __exit exit_f2fs_fs(void)
 {
-	f2fs_destroy_casefold_cache();
+	if (IS_ENABLED(CONFIG_UNICODE))
+		f2fs_destroy_casefold_cache();
+
 	f2fs_destroy_compress_cache();
 	f2fs_destroy_compress_mempool();
 	f2fs_destroy_bioset();
Signature · 8 lines
2.36.1



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
#17 lore raw

Re: [PATCH v4 10/10] f2fs: Move CONFIG_UNICODE defguards into the code flow

Quoted 8 lines · Gabriel Krisman Bertazi
fs/f2fs/super.c +0 -4
@@ -293,10 +294,6 @@ static void f2fs_destroy_casefold_cache(void)
 {
 	kmem_cache_destroy(f2fs_cf_name_slab);
 }
-#else
-static int __init f2fs_create_casefold_cache(void) { return 0; }
-static void f2fs_destroy_casefold_cache(void) { }
-#endif

[...]

Quoted 13 lines
fs/f2fs/super.c +4 -1
@@ -4611,7 +4608,10 @@ static int __init init_f2fs_fs(void)
 	err = f2fs_init_compress_cache();
 	if (err)
 		goto free_compress_mempool;
-	err = f2fs_create_casefold_cache();
+
+	if (IS_ENABLED(CONFIG_UNICODE))
+		err = f2fs_create_casefold_cache();
+
 	if (err)
 		goto free_compress_cache;
 	return 0;

fs/f2fs/super.c static int __init init_f2fs_fs(void) · line 4659

Quoted 7 lines
fs/f2fs/super.c +3 -1
@@ -4654,7 +4654,9 @@ static int __init init_f2fs_fs(void)
 
 static void __exit exit_f2fs_fs(void)
 {
-	f2fs_destroy_casefold_cache();
+	if (IS_ENABLED(CONFIG_UNICODE))
+		f2fs_destroy_casefold_cache();
+

I don't think the above two changes are actually an improvement. It's cleaner to use stub functions to keep the callers simpler, as the original code did.

- Eric

maintainer
#18 lore raw
In reply to Gabriel Krisman Bertazi
Quoted 11 lines · Gabriel Krisman Bertazi

The case-insensitive implementations in f2fs and ext4 have quite a bit of duplicated code. This series simplifies the ext4 version, with the goal of extracting ext4_ci_compare into a helper library that can be used by both filesystems. It also reduces the clutter from many codeguards for CONFIG_UNICODE; as requested by Linus, they are part of the codeflow now.

While there, I noticed we can leverage the utf8 functions to detect encoded names that are corrupted in the filesystem. Therefore, it also adds an ext4 error on that scenario, to mark the filesystem as corrupted.

Gabriel, are you planning on doing another version of this patch series?

It looks like the first two patches for ext4 are not controversial, so I could take those, while some of the other patches have questions which Eric has raised.

Thanks,

- Ted

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author
#19 lore raw
Quoted 13 lines · Theodore Ts'o
Quoted 11 lines · Gabriel Krisman Bertazi

The case-insensitive implementations in f2fs and ext4 have quite a bit of duplicated code. This series simplifies the ext4 version, with the goal of extracting ext4_ci_compare into a helper library that can be used by both filesystems. It also reduces the clutter from many codeguards for CONFIG_UNICODE; as requested by Linus, they are part of the codeflow now.

While there, I noticed we can leverage the utf8 functions to detect encoded names that are corrupted in the filesystem. Therefore, it also adds an ext4 error on that scenario, to mark the filesystem as corrupted.

Quoted 5 lines

Gabriel, are you planning on doing another version of this patch series?
It looks like the first two patches for ext4 are not controversial, so I could take those, while some of the other patches have questions which Eric has raised.

Hi Ted,

I'll be reworking the series to apply Eric's comments and I might render patch 1 unnecessary. I'd be happy to send a v5 for the whole thing instead of applying the first two now.

Thanks,

Signature · 1 line
Gabriel Krisman Bertazi