linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 0/4] Make UTF-8 encoding loadable
@ 2021-04-23 20:51 Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 1/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Shreeya Patel @ 2021-04-23 20:51 UTC (permalink / raw)
  To: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0
  Cc: linux-ext4, linux-kernel, linux-f2fs-devel, linux-fsdevel,
	kernel, andre.almeida

utf8data.h_shipped has a large database table which is an auto-generated
decodification trie for the unicode normalization functions and it is not
necessary to carry this large table in the kernel unless it is required by
the filesystem during boot time.

Goal is to make UTF-8 encoding loadable by converting it into a module
and adding a unicode subsystem layer between the filesystems and the
utf8 module.
This layer will then load the module whenever any filesystem that
needs unicode is mounted or utf8 can also be built into the kernel incase
it is required by the filesystem during boot time.

Currently, only UTF-8 encoding is supported but if any other encodings
are supported in future then the layer file would be responsible for
loading the desired encoding module.

1st patch in the series resolves the warning reported by kernel test
robot by using strscpy instead of strncpy.

Unicode is the subsystem and utf8 is a charachter encoding for the
subsystem, hence 2nd and 3rd patches in the series are renaming functions
and file name to unicode for better understanding the difference between
UTF-8 module and unicode layer.

Last patch in the series adds the layer and utf8 module and also uses
static calls which gives performance benefit when compared to indirect
calls using function pointers.

---
Changes in v8
  - Improve the commit message of patch 1 to decribe about how
    overly-long strings should be handled.
  - Improve the commit messages in patches 2/3/4 to better understand
    the use of built-in option.
  - Improve the help text in Kconfig for avoiding contradictory
    statements.
  - Make spinlock definition static.
  - Use int instead of bool to avoid gcc warning.
  - Add a comment for decribing why we are using
    try_then_request_module() instead of request_module()

Changes in v7
  - Update the help text in Kconfig
  - Handle the unicode_load_static_call function failure by decrementing
    the reference.
  - Correct the code for handling built-in utf8 option as well.
  - Correct the synchronization for accessing utf8mod.
  - Make changes to unicode_unload() for handling the situation where
    utf8mod != NULL and um == NULL.

Changes in v6
  - Add spinlock to protect utf8mod and avoid NULL pointer
    dereference.
  - Change the static call function names for being consistent with
    kernel coding style.
  - Merge the unicode_load_module function with unicode_load as it is
    not really needed to have a separate function.
  - Use try_then_module_get instead of module_get to avoid loading the
    module even when it is already loaded.
  - Improve the commit message.

Changes in v5
  - Remove patch which adds NULL check in ext4/super.c and f2fs/super.c
    before calling unicode_unload().
  - Rename global variables and default static call functions for better
    understanding
  - Make only config UNICODE_UTF8 visible and config UNICODE to be always
    enabled provided UNICODE_UTF8 is enabled.  
  - Improve the documentation for Kconfig
  - Improve the commit message.
 
Changes in v4
  - Return error from the static calls instead of doing nothing and
    succeeding even without loading the module.
  - Remove the complete usage of utf8_ops and use static calls at all
    places.
  - Restore the static calls to default values when module is unloaded.
  - Decrement the reference of module after calling the unload function.
  - Remove spinlock as there will be no race conditions after removing
    utf8_ops.

Changes in v3
  - Add a patch which checks if utf8 is loaded before calling utf8_unload()
    in ext4 and f2fs filesystems
  - Return error if strscpy() returns value < 0
  - Correct the conditions to prevent NULL pointer dereference while
    accessing functions via utf8_ops variable.
  - Add spinlock to avoid race conditions.
  - Use static_call() for preventing speculative execution attacks.

Changes in v2
  - Remove the duplicate file from the last patch.
  - Make the wrapper functions inline.
  - Remove msleep and use try_module_get() and module_put()
    for ensuring that module is loaded correctly and also
    doesn't get unloaded while in use.
  - Resolve the warning reported by kernel test robot.
  - Resolve all the checkpatch.pl warnings.

Shreeya Patel (4):
  fs: unicode: Use strscpy() instead of strncpy()
  fs: unicode: Rename function names from utf8 to unicode
  fs: unicode: Rename utf8-core file to unicode-core
  fs: unicode: Add utf8 module and a unicode layer

 fs/ext4/hash.c                             |   2 +-
 fs/ext4/namei.c                            |  12 +-
 fs/ext4/super.c                            |   6 +-
 fs/f2fs/dir.c                              |  12 +-
 fs/f2fs/super.c                            |   6 +-
 fs/libfs.c                                 |   6 +-
 fs/unicode/Kconfig                         |  26 ++-
 fs/unicode/Makefile                        |   5 +-
 fs/unicode/unicode-core.c                  | 175 +++++++++++++++++++++
 fs/unicode/{utf8-core.c => unicode-utf8.c} |  98 +++++++-----
 fs/unicode/utf8-selftest.c                 |   8 +-
 include/linux/unicode.h                    | 100 ++++++++++--
 12 files changed, 374 insertions(+), 82 deletions(-)
 create mode 100644 fs/unicode/unicode-core.c
 rename fs/unicode/{utf8-core.c => unicode-utf8.c} (57%)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v8 1/4] fs: unicode: Use strscpy() instead of strncpy()
  2021-04-23 20:51 [PATCH v8 0/4] Make UTF-8 encoding loadable Shreeya Patel
@ 2021-04-23 20:51 ` Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 2/4] fs: unicode: Rename function names from utf8 to unicode Shreeya Patel
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Shreeya Patel @ 2021-04-23 20:51 UTC (permalink / raw)
  To: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0
  Cc: linux-ext4, linux-kernel, linux-f2fs-devel, linux-fsdevel,
	kernel, andre.almeida, kernel test robot

Following warning was reported by Kernel Test Robot.

In function 'utf8_parse_version',
inlined from 'utf8_load' at fs/unicode/utf8mod.c:195:7:
>> fs/unicode/utf8mod.c:175:2: warning: 'strncpy' specified bound 12 equals
destination size [-Wstringop-truncation]
175 |  strncpy(version_string, version, sizeof(version_string));
    |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The -Wstringop-truncation warning highlights the unintended
uses of the strncpy function that truncate the terminating NULL
character if source string is longer than the destination size.

strscpy() returns -E2BIG error code in case the source string doesn't
fit into the destination. Hence, use strscpy() and return an error for
overly-long strings instead of creating a non-null-terminated string
with strncpy().

Fixes: 9d53690f0d4e5 (unicode: implement higher level API for string handling)
Acked-by: Gabriel Krisman Bertazi <krisman@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Reported-by: kernel test robot <lkp@intel.com>
---
Changes in v8
  - Improve the commit message to decribe about how overly-long strings
    are handled.

 fs/unicode/utf8-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/unicode/utf8-core.c b/fs/unicode/utf8-core.c
index dc25823bfed9..f9e6a2718aba 100644
--- a/fs/unicode/utf8-core.c
+++ b/fs/unicode/utf8-core.c
@@ -179,8 +179,10 @@ static int utf8_parse_version(const char *version, unsigned int *maj,
 		{1, "%d.%d.%d"},
 		{0, NULL}
 	};
+	int ret = strscpy(version_string, version, sizeof(version_string));
 
-	strncpy(version_string, version, sizeof(version_string));
+	if (ret < 0)
+		return ret;
 
 	if (match_token(version_string, token, args) != 1)
 		return -EINVAL;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v8 2/4] fs: unicode: Rename function names from utf8 to unicode
  2021-04-23 20:51 [PATCH v8 0/4] Make UTF-8 encoding loadable Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 1/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
@ 2021-04-23 20:51 ` Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 3/4] fs: unicode: Rename utf8-core file to unicode-core Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
  3 siblings, 0 replies; 15+ messages in thread
From: Shreeya Patel @ 2021-04-23 20:51 UTC (permalink / raw)
  To: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0
  Cc: linux-ext4, linux-kernel, linux-f2fs-devel, linux-fsdevel,
	kernel, andre.almeida

utf8data.h_shipped has a large database table which is an auto-generated
decodification trie for the unicode normalization functions and it is not
necessary to carry this large table in the kernel unless it is required by
the filesystem during boot time.

Goal is to make UTF-8 encoding loadable by converting it into a module
and adding a unicode subsystem layer between the filesystems and the
utf8 module.
This layer will then load the module whenever any filesystem that
needs unicode is mounted or can also be built into the kernel incase
it is required by the filesystem during boot time.

Currently, only UTF-8 encoding is supported but if any other encodings
are supported in future then the layer file would be responsible for
loading the desired encoding module.

utf8-core will be converted into this layer file in the future patches.
Rename the function names from utf8 to unicode which will denote the
functions as the unicode subsystem layer functions and this will also be
the first step towards the transformation of utf8-core file into the
unicode subsystem layer file.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
Changes in v8
  - Improve the commit message to better decribe about the built-in
    option usage.

 fs/ext4/hash.c             |  2 +-
 fs/ext4/namei.c            | 12 ++++----
 fs/ext4/super.c            |  6 ++--
 fs/f2fs/dir.c              | 12 ++++----
 fs/f2fs/super.c            |  6 ++--
 fs/libfs.c                 |  6 ++--
 fs/unicode/utf8-core.c     | 57 +++++++++++++++++++-------------------
 fs/unicode/utf8-selftest.c |  8 +++---
 include/linux/unicode.h    | 32 ++++++++++-----------
 9 files changed, 70 insertions(+), 71 deletions(-)

diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
index a92eb79de0cc..8890a76abe86 100644
--- a/fs/ext4/hash.c
+++ b/fs/ext4/hash.c
@@ -285,7 +285,7 @@ int ext4fs_dirhash(const struct inode *dir, const char *name, int len,
 		if (!buff)
 			return -ENOMEM;
 
-		dlen = utf8_casefold(um, &qstr, buff, PATH_MAX);
+		dlen = unicode_casefold(um, &qstr, buff, PATH_MAX);
 		if (dlen < 0) {
 			kfree(buff);
 			goto opaque_seq;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 686bf982c84e..dde5ce795416 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1290,9 +1290,9 @@ int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
 	int ret;
 
 	if (quick)
-		ret = utf8_strncasecmp_folded(um, name, entry);
+		ret = unicode_strncasecmp_folded(um, name, entry);
 	else
-		ret = utf8_strncasecmp(um, name, entry);
+		ret = unicode_strncasecmp(um, name, entry);
 
 	if (ret < 0) {
 		/* Handle invalid character sequence as either an error
@@ -1324,9 +1324,9 @@ void ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
 	if (!cf_name->name)
 		return;
 
-	len = utf8_casefold(dir->i_sb->s_encoding,
-			    iname, cf_name->name,
-			    EXT4_NAME_LEN);
+	len = unicode_casefold(dir->i_sb->s_encoding,
+			       iname, cf_name->name,
+			       EXT4_NAME_LEN);
 	if (len <= 0) {
 		kfree(cf_name->name);
 		cf_name->name = NULL;
@@ -2201,7 +2201,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 
 #ifdef CONFIG_UNICODE
 	if (sb_has_strict_encoding(sb) && IS_CASEFOLDED(dir) &&
-	    sb->s_encoding && utf8_validate(sb->s_encoding, &dentry->d_name))
+	    sb->s_encoding && unicode_validate(sb->s_encoding, &dentry->d_name))
 		return -EINVAL;
 #endif
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ad34a37278cd..2fb845752c90 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1259,7 +1259,7 @@ static void ext4_put_super(struct super_block *sb)
 	fs_put_dax(sbi->s_daxdev);
 	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	unicode_unload(sb->s_encoding);
 #endif
 	kfree(sbi);
 }
@@ -4304,7 +4304,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 			goto failed_mount;
 		}
 
-		encoding = utf8_load(encoding_info->version);
+		encoding = unicode_load(encoding_info->version);
 		if (IS_ERR(encoding)) {
 			ext4_msg(sb, KERN_ERR,
 				 "can't mount with superblock charset: %s-%s "
@@ -5165,7 +5165,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		crypto_free_shash(sbi->s_chksum_driver);
 
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	unicode_unload(sb->s_encoding);
 #endif
 
 #ifdef CONFIG_QUOTA
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index e6270a867be1..f160f9dd667d 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -84,10 +84,10 @@ int f2fs_init_casefolded_name(const struct inode *dir,
 						   GFP_NOFS);
 		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);
+		fname->cf_name.len = unicode_casefold(sb->s_encoding,
+						      fname->usr_fname,
+						      fname->cf_name.name,
+						      F2FS_NAME_LEN);
 		if ((int)fname->cf_name.len <= 0) {
 			kfree(fname->cf_name.name);
 			fname->cf_name.name = NULL;
@@ -237,7 +237,7 @@ static int f2fs_match_ci_name(const struct inode *dir, const struct qstr *name,
 		entry.len = decrypted_name.len;
 	}
 
-	res = utf8_strncasecmp_folded(um, name, &entry);
+	res = unicode_strncasecmp_folded(um, name, &entry);
 	/*
 	 * In strict mode, ignore invalid names.  In non-strict mode,
 	 * fall back to treating them as opaque byte sequences.
@@ -246,7 +246,7 @@ static int f2fs_match_ci_name(const struct inode *dir, const struct qstr *name,
 		res = name->len == entry.len &&
 				memcmp(name->name, entry.name, name->len) == 0;
 	} else {
-		/* utf8_strncasecmp_folded returns 0 on match */
+		/* unicode_strncasecmp_folded returns 0 on match */
 		res = (res == 0);
 	}
 out:
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7069793752f1..b4a92e763e27 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1430,7 +1430,7 @@ static void f2fs_put_super(struct super_block *sb)
 	for (i = 0; i < NR_PAGE_TYPE; i++)
 		kvfree(sbi->write_io[i]);
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	unicode_unload(sb->s_encoding);
 #endif
 	kfree(sbi);
 }
@@ -3560,7 +3560,7 @@ static int f2fs_setup_casefold(struct f2fs_sb_info *sbi)
 			return -EINVAL;
 		}
 
-		encoding = utf8_load(encoding_info->version);
+		encoding = unicode_load(encoding_info->version);
 		if (IS_ERR(encoding)) {
 			f2fs_err(sbi,
 				 "can't mount with superblock charset: %s-%s "
@@ -4073,7 +4073,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		kvfree(sbi->write_io[i]);
 
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	unicode_unload(sb->s_encoding);
 	sb->s_encoding = NULL;
 #endif
 free_options:
diff --git a/fs/libfs.c b/fs/libfs.c
index e2de5401abca..766556165bb5 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1404,7 +1404,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
 	 * If the dentry name is stored in-line, then it may be concurrently
 	 * modified by a rename.  If this happens, the VFS will eventually retry
 	 * the lookup, so it doesn't matter what ->d_compare() returns.
-	 * However, it's unsafe to call utf8_strncasecmp() with an unstable
+	 * However, it's unsafe to call unicode_strncasecmp() with an unstable
 	 * string.  Therefore, we have to copy the name into a temporary buffer.
 	 */
 	if (len <= DNAME_INLINE_LEN - 1) {
@@ -1414,7 +1414,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
 		/* prevent compiler from optimizing out the temporary buffer */
 		barrier();
 	}
-	ret = utf8_strncasecmp(um, name, &qstr);
+	ret = unicode_strncasecmp(um, name, &qstr);
 	if (ret >= 0)
 		return ret;
 
@@ -1443,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
 	if (!dir || !needs_casefold(dir))
 		return 0;
 
-	ret = utf8_casefold_hash(um, dentry, str);
+	ret = unicode_casefold_hash(um, dentry, str);
 	if (ret < 0 && sb_has_strict_encoding(sb))
 		return -EINVAL;
 	return 0;
diff --git a/fs/unicode/utf8-core.c b/fs/unicode/utf8-core.c
index f9e6a2718aba..730dbaedf593 100644
--- a/fs/unicode/utf8-core.c
+++ b/fs/unicode/utf8-core.c
@@ -10,7 +10,7 @@
 
 #include "utf8n.h"
 
-int utf8_validate(const struct unicode_map *um, const struct qstr *str)
+int unicode_validate(const struct unicode_map *um, const struct qstr *str)
 {
 	const struct utf8data *data = utf8nfdi(um->version);
 
@@ -18,10 +18,10 @@ int utf8_validate(const struct unicode_map *um, const struct qstr *str)
 		return -1;
 	return 0;
 }
-EXPORT_SYMBOL(utf8_validate);
+EXPORT_SYMBOL(unicode_validate);
 
-int utf8_strncmp(const struct unicode_map *um,
-		 const struct qstr *s1, const struct qstr *s2)
+int unicode_strncmp(const struct unicode_map *um,
+		    const struct qstr *s1, const struct qstr *s2)
 {
 	const struct utf8data *data = utf8nfdi(um->version);
 	struct utf8cursor cur1, cur2;
@@ -45,10 +45,10 @@ int utf8_strncmp(const struct unicode_map *um,
 
 	return 0;
 }
-EXPORT_SYMBOL(utf8_strncmp);
+EXPORT_SYMBOL(unicode_strncmp);
 
-int utf8_strncasecmp(const struct unicode_map *um,
-		     const struct qstr *s1, const struct qstr *s2)
+int unicode_strncasecmp(const struct unicode_map *um,
+			const struct qstr *s1, const struct qstr *s2)
 {
 	const struct utf8data *data = utf8nfdicf(um->version);
 	struct utf8cursor cur1, cur2;
@@ -72,14 +72,14 @@ int utf8_strncasecmp(const struct unicode_map *um,
 
 	return 0;
 }
-EXPORT_SYMBOL(utf8_strncasecmp);
+EXPORT_SYMBOL(unicode_strncasecmp);
 
 /* String cf is expected to be a valid UTF-8 casefolded
  * string.
  */
-int utf8_strncasecmp_folded(const struct unicode_map *um,
-			    const struct qstr *cf,
-			    const struct qstr *s1)
+int unicode_strncasecmp_folded(const struct unicode_map *um,
+			       const struct qstr *cf,
+			       const struct qstr *s1)
 {
 	const struct utf8data *data = utf8nfdicf(um->version);
 	struct utf8cursor cur1;
@@ -100,10 +100,10 @@ int utf8_strncasecmp_folded(const struct unicode_map *um,
 
 	return 0;
 }
-EXPORT_SYMBOL(utf8_strncasecmp_folded);
+EXPORT_SYMBOL(unicode_strncasecmp_folded);
 
-int utf8_casefold(const struct unicode_map *um, const struct qstr *str,
-		  unsigned char *dest, size_t dlen)
+int unicode_casefold(const struct unicode_map *um, const struct qstr *str,
+		     unsigned char *dest, size_t dlen)
 {
 	const struct utf8data *data = utf8nfdicf(um->version);
 	struct utf8cursor cur;
@@ -123,10 +123,10 @@ int utf8_casefold(const struct unicode_map *um, const struct qstr *str,
 	}
 	return -EINVAL;
 }
-EXPORT_SYMBOL(utf8_casefold);
+EXPORT_SYMBOL(unicode_casefold);
 
-int utf8_casefold_hash(const struct unicode_map *um, const void *salt,
-		       struct qstr *str)
+int unicode_casefold_hash(const struct unicode_map *um, const void *salt,
+			  struct qstr *str)
 {
 	const struct utf8data *data = utf8nfdicf(um->version);
 	struct utf8cursor cur;
@@ -144,10 +144,10 @@ int utf8_casefold_hash(const struct unicode_map *um, const void *salt,
 	str->hash = end_name_hash(hash);
 	return 0;
 }
-EXPORT_SYMBOL(utf8_casefold_hash);
+EXPORT_SYMBOL(unicode_casefold_hash);
 
-int utf8_normalize(const struct unicode_map *um, const struct qstr *str,
-		   unsigned char *dest, size_t dlen)
+int unicode_normalize(const struct unicode_map *um, const struct qstr *str,
+		      unsigned char *dest, size_t dlen)
 {
 	const struct utf8data *data = utf8nfdi(um->version);
 	struct utf8cursor cur;
@@ -167,11 +167,10 @@ int utf8_normalize(const struct unicode_map *um, const struct qstr *str,
 	}
 	return -EINVAL;
 }
+EXPORT_SYMBOL(unicode_normalize);
 
-EXPORT_SYMBOL(utf8_normalize);
-
-static int utf8_parse_version(const char *version, unsigned int *maj,
-			      unsigned int *min, unsigned int *rev)
+static int unicode_parse_version(const char *version, unsigned int *maj,
+				 unsigned int *min, unsigned int *rev)
 {
 	substring_t args[3];
 	char version_string[12];
@@ -194,7 +193,7 @@ static int utf8_parse_version(const char *version, unsigned int *maj,
 	return 0;
 }
 
-struct unicode_map *utf8_load(const char *version)
+struct unicode_map *unicode_load(const char *version)
 {
 	struct unicode_map *um = NULL;
 	int unicode_version;
@@ -202,7 +201,7 @@ struct unicode_map *utf8_load(const char *version)
 	if (version) {
 		unsigned int maj, min, rev;
 
-		if (utf8_parse_version(version, &maj, &min, &rev) < 0)
+		if (unicode_parse_version(version, &maj, &min, &rev) < 0)
 			return ERR_PTR(-EINVAL);
 
 		if (!utf8version_is_supported(maj, min, rev))
@@ -227,12 +226,12 @@ struct unicode_map *utf8_load(const char *version)
 
 	return um;
 }
-EXPORT_SYMBOL(utf8_load);
+EXPORT_SYMBOL(unicode_load);
 
-void utf8_unload(struct unicode_map *um)
+void unicode_unload(struct unicode_map *um)
 {
 	kfree(um);
 }
-EXPORT_SYMBOL(utf8_unload);
+EXPORT_SYMBOL(unicode_unload);
 
 MODULE_LICENSE("GPL v2");
diff --git a/fs/unicode/utf8-selftest.c b/fs/unicode/utf8-selftest.c
index 6fe8af7edccb..796c1ed922ea 100644
--- a/fs/unicode/utf8-selftest.c
+++ b/fs/unicode/utf8-selftest.c
@@ -235,7 +235,7 @@ static void check_utf8_nfdicf(void)
 static void check_utf8_comparisons(void)
 {
 	int i;
-	struct unicode_map *table = utf8_load("12.1.0");
+	struct unicode_map *table = unicode_load("12.1.0");
 
 	if (IS_ERR(table)) {
 		pr_err("%s: Unable to load utf8 %d.%d.%d. Skipping.\n",
@@ -249,7 +249,7 @@ static void check_utf8_comparisons(void)
 		const struct qstr s2 = {.name = nfdi_test_data[i].dec,
 					.len = sizeof(nfdi_test_data[i].dec)};
 
-		test_f(!utf8_strncmp(table, &s1, &s2),
+		test_f(!unicode_strncmp(table, &s1, &s2),
 		       "%s %s comparison mismatch\n", s1.name, s2.name);
 	}
 
@@ -259,11 +259,11 @@ static void check_utf8_comparisons(void)
 		const struct qstr s2 = {.name = nfdicf_test_data[i].ncf,
 					.len = sizeof(nfdicf_test_data[i].ncf)};
 
-		test_f(!utf8_strncasecmp(table, &s1, &s2),
+		test_f(!unicode_strncasecmp(table, &s1, &s2),
 		       "%s %s comparison mismatch\n", s1.name, s2.name);
 	}
 
-	utf8_unload(table);
+	unicode_unload(table);
 }
 
 static void check_supported_versions(void)
diff --git a/include/linux/unicode.h b/include/linux/unicode.h
index 74484d44c755..de23f9ee720b 100644
--- a/include/linux/unicode.h
+++ b/include/linux/unicode.h
@@ -10,27 +10,27 @@ struct unicode_map {
 	int version;
 };
 
-int utf8_validate(const struct unicode_map *um, const struct qstr *str);
+int unicode_validate(const struct unicode_map *um, const struct qstr *str);
 
-int utf8_strncmp(const struct unicode_map *um,
-		 const struct qstr *s1, const struct qstr *s2);
+int unicode_strncmp(const struct unicode_map *um,
+		    const struct qstr *s1, const struct qstr *s2);
 
-int utf8_strncasecmp(const struct unicode_map *um,
-		 const struct qstr *s1, const struct qstr *s2);
-int utf8_strncasecmp_folded(const struct unicode_map *um,
-			    const struct qstr *cf,
-			    const struct qstr *s1);
+int unicode_strncasecmp(const struct unicode_map *um,
+			const struct qstr *s1, const struct qstr *s2);
+int unicode_strncasecmp_folded(const struct unicode_map *um,
+			       const struct qstr *cf,
+			       const struct qstr *s1);
 
-int utf8_normalize(const struct unicode_map *um, const struct qstr *str,
-		   unsigned char *dest, size_t dlen);
+int unicode_normalize(const struct unicode_map *um, const struct qstr *str,
+		      unsigned char *dest, size_t dlen);
 
-int utf8_casefold(const struct unicode_map *um, const struct qstr *str,
-		  unsigned char *dest, size_t dlen);
+int unicode_casefold(const struct unicode_map *um, const struct qstr *str,
+		     unsigned char *dest, size_t dlen);
 
-int utf8_casefold_hash(const struct unicode_map *um, const void *salt,
-		       struct qstr *str);
+int unicode_casefold_hash(const struct unicode_map *um, const void *salt,
+			  struct qstr *str);
 
-struct unicode_map *utf8_load(const char *version);
-void utf8_unload(struct unicode_map *um);
+struct unicode_map *unicode_load(const char *version);
+void unicode_unload(struct unicode_map *um);
 
 #endif /* _LINUX_UNICODE_H */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v8 3/4] fs: unicode: Rename utf8-core file to unicode-core
  2021-04-23 20:51 [PATCH v8 0/4] Make UTF-8 encoding loadable Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 1/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 2/4] fs: unicode: Rename function names from utf8 to unicode Shreeya Patel
@ 2021-04-23 20:51 ` Shreeya Patel
  2021-04-23 20:51 ` [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
  3 siblings, 0 replies; 15+ messages in thread
From: Shreeya Patel @ 2021-04-23 20:51 UTC (permalink / raw)
  To: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0
  Cc: linux-ext4, linux-kernel, linux-f2fs-devel, linux-fsdevel,
	kernel, andre.almeida

utf8data.h_shipped has a large database table which is an auto-generated
decodification trie for the unicode normalization functions and it is not
necessary to carry this large table in the kernel unless it is required by
the filesystem during boot time.

Goal is to make UTF-8 encoding loadable by converting it into a module
and adding a unicode subsystem layer between the filesystems and the
utf8 module.
This layer will then load the module whenever any filesystem that
needs unicode is mounted or utf8 can also be built into the kernel incase
it is required by the filesystem during boot time.

Currently, only UTF-8 encoding is supported but if any other encodings
are supported in future then the layer file would be responsible for
loading the desired encoding module.

Rename the file name from utf8-core to unicode-core for transformation of
utf8-core file into the unicode subsystem layer file and also for better
understanding.
Implementation for unicode-core file to act as layer will be added in the
future patches.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
Changes in v8
  - Improve the commit message to better describe about the built-in
    option usage.

 fs/unicode/Makefile                        | 2 +-
 fs/unicode/{utf8-core.c => unicode-core.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename fs/unicode/{utf8-core.c => unicode-core.c} (100%)

diff --git a/fs/unicode/Makefile b/fs/unicode/Makefile
index b88aecc86550..fbf9a629ed0d 100644
--- a/fs/unicode/Makefile
+++ b/fs/unicode/Makefile
@@ -3,7 +3,7 @@
 obj-$(CONFIG_UNICODE) += unicode.o
 obj-$(CONFIG_UNICODE_NORMALIZATION_SELFTEST) += utf8-selftest.o
 
-unicode-y := utf8-norm.o utf8-core.o
+unicode-y := utf8-norm.o unicode-core.o
 
 $(obj)/utf8-norm.o: $(obj)/utf8data.h
 
diff --git a/fs/unicode/utf8-core.c b/fs/unicode/unicode-core.c
similarity index 100%
rename from fs/unicode/utf8-core.c
rename to fs/unicode/unicode-core.c
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-23 20:51 [PATCH v8 0/4] Make UTF-8 encoding loadable Shreeya Patel
                   ` (2 preceding siblings ...)
  2021-04-23 20:51 ` [PATCH v8 3/4] fs: unicode: Rename utf8-core file to unicode-core Shreeya Patel
@ 2021-04-23 20:51 ` Shreeya Patel
  2021-04-27  6:29   ` Christoph Hellwig
  3 siblings, 1 reply; 15+ messages in thread
From: Shreeya Patel @ 2021-04-23 20:51 UTC (permalink / raw)
  To: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0
  Cc: linux-ext4, linux-kernel, linux-f2fs-devel, linux-fsdevel,
	kernel, andre.almeida

utf8data.h_shipped has a large database table which is an auto-generated
decodification trie for the unicode normalization functions.
We can avoid carrying this large table in the kernel unless it is required
by the filesystem during boot process.

Hence, make UTF-8 encoding loadable by converting it into a module and
also add built-in UTF-8 support option for compiling it into the
kernel whenever required by the filesystem.

Modify the file called unicode-core which will act as a layer for
unicode subsystem. It will be responsible for loading the UTF-8 module
and accessing it's functions.

Currently, only UTF-8 encoding is supported but if any other encodings
are supported in future then the layer file would be responsible for
loading the desired encoding module.

Also, indirect calls using function pointers are slow, use static calls to
avoid overhead caused in case of repeated indirect calls. Static calls
improves the performance by directly calling the functions as opposed to
indirect calls.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
Changes in v8
  - Improve the commit messages to better understand the use of built-in option.
  - Improve the help text in Kconfig for avoiding contradictory statements.
  - Make spinlock definition static.
  - Use int instead of bool to avoid gcc warning.
  - Add a comment for decribing why we are using try_then_request_module()
    instead of request_module()

Changes in v7
  - Update the help text in Kconfig
  - Handle the unicode_load_static_call function failure by decrementing
    the reference.
  - Correct the code for handling built-in utf8 option as well.
  - Correct the synchronization for accessing utf8mod.
  - Make changes to unicode_unload() for handling the situation where
    utf8mod != NULL and um == NULL.

Changes in v6
  - Add spinlock to protect utf8mod and avoid NULL pointer
    dereference.
  - Change the static call function names for being consistent with
    kernel coding style.
  - Merge the unicode_load_module function with unicode_load as it is
    not really needed to have a separate function.
  - Use try_then_module_get instead of module_get to avoid loading the
    module even when it is already loaded.
  - Improve the commit message.

Changes in v5
  - Rename global variables and default static call functions for better
    understanding
  - Make only config UNICODE_UTF8 visible and config UNICODE to be always
    enabled provided UNICODE_UTF8 is enabled.  
  - Improve the documentation for Kconfig
  - Improve the commit message.
 
Changes in v4
  - Return error from the static calls instead of doing nothing and
    succeeding even without loading the module.
  - Remove the complete usage of utf8_ops and use static calls at all
    places.
  - Restore the static calls to default values when module is unloaded.
  - Decrement the reference of module after calling the unload function.
  - Remove spinlock as there will be no race conditions after removing
    utf8_ops.

Changes in v3
  - Add a patch which checks if utf8 is loaded before calling utf8_unload()
    in ext4 and f2fs filesystems
  - Return error if strscpy() returns value < 0
  - Correct the conditions to prevent NULL pointer dereference while
    accessing functions via utf8_ops variable.
  - Add spinlock to avoid race conditions.
  - Use static_call() for preventing speculative execution attacks.

Changes in v2
  - Remove the duplicate file from the last patch.
  - Make the wrapper functions inline.
  - Remove msleep and use try_module_get() and module_put()
    for ensuring that module is loaded correctly and also
    doesn't get unloaded while in use.
  - Resolve the warning reported by kernel test robot.
  - Resolve all the checkpatch.pl warnings.

 fs/unicode/Kconfig        |  26 +++-
 fs/unicode/Makefile       |   5 +-
 fs/unicode/unicode-core.c | 310 +++++++++++++++-----------------------
 fs/unicode/unicode-utf8.c | 264 ++++++++++++++++++++++++++++++++
 include/linux/unicode.h   |  96 ++++++++++--
 5 files changed, 496 insertions(+), 205 deletions(-)
 create mode 100644 fs/unicode/unicode-utf8.c

diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig
index 2c27b9a5cd6c..250b3671a0e2 100644
--- a/fs/unicode/Kconfig
+++ b/fs/unicode/Kconfig
@@ -2,13 +2,31 @@
 #
 # UTF-8 normalization
 #
+# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8
+# is enabled. This config option adds the unicode subsystem layer which loads
+# the UTF-8 module whenever any filesystem needs it.
 config UNICODE
-	bool "UTF-8 normalization and casefolding support"
+	bool
+
+config UNICODE_UTF8
+	tristate "UTF-8 support for native Case-Insensitive filesystems"
+	select UNICODE
 	help
-	  Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding
-	  support.
+	  Say M here to enable UTF-8 NFD normalization and NFD+CF casefolding
+	  support as a loadable module or say Y for building it into the kernel.
+	  It is currently supported by EXT4 and F2FS filesystems.
+
+	  utf8data.h_shipped has a large database table which is an
+	  auto-generated decodification trie for the unicode normalization
+	  functions. Enabling UNICODE_UTF8 as M will allow you to avoid
+	  carrying this large table into the kernel and module will only be
+	  loaded whenever required by any filesystem.
+	  Please note, in this case utf8 module will only be available after
+	  booting into the compiled kernel. If your filesystem requires it to
+	  have utf8 during boot time then you should have it built into the
+	  kernel by saying Y here to avoid boot failure.
 
 config UNICODE_NORMALIZATION_SELFTEST
 	tristate "Test UTF-8 normalization support"
-	depends on UNICODE
+	depends on UNICODE_UTF8
 	default n
diff --git a/fs/unicode/Makefile b/fs/unicode/Makefile
index fbf9a629ed0d..49d50083e6ee 100644
--- a/fs/unicode/Makefile
+++ b/fs/unicode/Makefile
@@ -1,11 +1,14 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_UNICODE) += unicode.o
+obj-$(CONFIG_UNICODE_UTF8) += utf8.o
 obj-$(CONFIG_UNICODE_NORMALIZATION_SELFTEST) += utf8-selftest.o
 
-unicode-y := utf8-norm.o unicode-core.o
+unicode-y := unicode-core.o
+utf8-y := unicode-utf8.o utf8-norm.o
 
 $(obj)/utf8-norm.o: $(obj)/utf8data.h
+$(obj)/unicode-utf8.o: $(obj)/utf8-norm.o
 
 # In the normal build, the checked-in utf8data.h is just shipped.
 #
diff --git a/fs/unicode/unicode-core.c b/fs/unicode/unicode-core.c
index 730dbaedf593..bf442446ed89 100644
--- a/fs/unicode/unicode-core.c
+++ b/fs/unicode/unicode-core.c
@@ -1,228 +1,145 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/module.h>
 #include <linux/kernel.h>
-#include <linux/string.h>
 #include <linux/slab.h>
-#include <linux/parser.h>
 #include <linux/errno.h>
 #include <linux/unicode.h>
-#include <linux/stringhash.h>
+#include <linux/spinlock.h>
 
-#include "utf8n.h"
+static DEFINE_SPINLOCK(utf8mod_lock);
 
-int unicode_validate(const struct unicode_map *um, const struct qstr *str)
-{
-	const struct utf8data *data = utf8nfdi(um->version);
+static struct module *utf8mod;
+static bool utf8mod_loaded;
 
-	if (utf8nlen(data, str->name, str->len) < 0)
-		return -1;
-	return 0;
+int unicode_validate_default(const struct unicode_map *um,
+			     const struct qstr *str)
+{
+	WARN_ON(1);
+	return -EIO;
 }
-EXPORT_SYMBOL(unicode_validate);
+EXPORT_SYMBOL(unicode_validate_default);
 
-int unicode_strncmp(const struct unicode_map *um,
-		    const struct qstr *s1, const struct qstr *s2)
+int unicode_strncmp_default(const struct unicode_map *um,
+			    const struct qstr *s1,
+			    const struct qstr *s2)
 {
-	const struct utf8data *data = utf8nfdi(um->version);
-	struct utf8cursor cur1, cur2;
-	int c1, c2;
-
-	if (utf8ncursor(&cur1, data, s1->name, s1->len) < 0)
-		return -EINVAL;
-
-	if (utf8ncursor(&cur2, data, s2->name, s2->len) < 0)
-		return -EINVAL;
-
-	do {
-		c1 = utf8byte(&cur1);
-		c2 = utf8byte(&cur2);
-
-		if (c1 < 0 || c2 < 0)
-			return -EINVAL;
-		if (c1 != c2)
-			return 1;
-	} while (c1);
-
-	return 0;
+	WARN_ON(1);
+	return -EIO;
 }
-EXPORT_SYMBOL(unicode_strncmp);
+EXPORT_SYMBOL(unicode_strncmp_default);
 
-int unicode_strncasecmp(const struct unicode_map *um,
-			const struct qstr *s1, const struct qstr *s2)
+int unicode_strncasecmp_default(const struct unicode_map *um,
+				const struct qstr *s1,
+				const struct qstr *s2)
 {
-	const struct utf8data *data = utf8nfdicf(um->version);
-	struct utf8cursor cur1, cur2;
-	int c1, c2;
-
-	if (utf8ncursor(&cur1, data, s1->name, s1->len) < 0)
-		return -EINVAL;
-
-	if (utf8ncursor(&cur2, data, s2->name, s2->len) < 0)
-		return -EINVAL;
-
-	do {
-		c1 = utf8byte(&cur1);
-		c2 = utf8byte(&cur2);
-
-		if (c1 < 0 || c2 < 0)
-			return -EINVAL;
-		if (c1 != c2)
-			return 1;
-	} while (c1);
-
-	return 0;
+	WARN_ON(1);
+	return -EIO;
 }
-EXPORT_SYMBOL(unicode_strncasecmp);
-
-/* String cf is expected to be a valid UTF-8 casefolded
- * string.
- */
-int unicode_strncasecmp_folded(const struct unicode_map *um,
-			       const struct qstr *cf,
-			       const struct qstr *s1)
+EXPORT_SYMBOL(unicode_strncasecmp_default);
+
+int unicode_strncasecmp_folded_default(const struct unicode_map *um,
+				       const struct qstr *cf,
+				       const struct qstr *s1)
 {
-	const struct utf8data *data = utf8nfdicf(um->version);
-	struct utf8cursor cur1;
-	int c1, c2;
-	int i = 0;
-
-	if (utf8ncursor(&cur1, data, s1->name, s1->len) < 0)
-		return -EINVAL;
-
-	do {
-		c1 = utf8byte(&cur1);
-		c2 = cf->name[i++];
-		if (c1 < 0)
-			return -EINVAL;
-		if (c1 != c2)
-			return 1;
-	} while (c1);
-
-	return 0;
+	WARN_ON(1);
+	return -EIO;
 }
-EXPORT_SYMBOL(unicode_strncasecmp_folded);
+EXPORT_SYMBOL(unicode_strncasecmp_folded_default);
 
-int unicode_casefold(const struct unicode_map *um, const struct qstr *str,
-		     unsigned char *dest, size_t dlen)
+int unicode_normalize_default(const struct unicode_map *um,
+			      const struct qstr *str,
+			      unsigned char *dest, size_t dlen)
 {
-	const struct utf8data *data = utf8nfdicf(um->version);
-	struct utf8cursor cur;
-	size_t nlen = 0;
-
-	if (utf8ncursor(&cur, data, str->name, str->len) < 0)
-		return -EINVAL;
-
-	for (nlen = 0; nlen < dlen; nlen++) {
-		int c = utf8byte(&cur);
+	WARN_ON(1);
+	return -EIO;
+}
+EXPORT_SYMBOL(unicode_normalize_default);
 
-		dest[nlen] = c;
-		if (!c)
-			return nlen;
-		if (c == -1)
-			break;
-	}
-	return -EINVAL;
+int unicode_casefold_default(const struct unicode_map *um,
+			     const struct qstr *str,
+			     unsigned char *dest, size_t dlen)
+{
+	WARN_ON(1);
+	return -EIO;
 }
-EXPORT_SYMBOL(unicode_casefold);
+EXPORT_SYMBOL(unicode_casefold_default);
 
-int unicode_casefold_hash(const struct unicode_map *um, const void *salt,
-			  struct qstr *str)
+int unicode_casefold_hash_default(const struct unicode_map *um,
+				  const void *salt, struct qstr *str)
 {
-	const struct utf8data *data = utf8nfdicf(um->version);
-	struct utf8cursor cur;
-	int c;
-	unsigned long hash = init_name_hash(salt);
-
-	if (utf8ncursor(&cur, data, str->name, str->len) < 0)
-		return -EINVAL;
-
-	while ((c = utf8byte(&cur))) {
-		if (c < 0)
-			return -EINVAL;
-		hash = partial_name_hash((unsigned char)c, hash);
-	}
-	str->hash = end_name_hash(hash);
-	return 0;
+	WARN_ON(1);
+	return -EIO;
 }
-EXPORT_SYMBOL(unicode_casefold_hash);
+EXPORT_SYMBOL(unicode_casefold_hash_default);
 
-int unicode_normalize(const struct unicode_map *um, const struct qstr *str,
-		      unsigned char *dest, size_t dlen)
+struct unicode_map *unicode_load_default(const char *version)
 {
-	const struct utf8data *data = utf8nfdi(um->version);
-	struct utf8cursor cur;
-	ssize_t nlen = 0;
+	WARN_ON(1);
+	return ERR_PTR(-EIO);
+}
+EXPORT_SYMBOL(unicode_load_default);
 
-	if (utf8ncursor(&cur, data, str->name, str->len) < 0)
-		return -EINVAL;
+DEFINE_STATIC_CALL(unicode_validate_static_call, unicode_validate_default);
+EXPORT_STATIC_CALL(unicode_validate_static_call);
 
-	for (nlen = 0; nlen < dlen; nlen++) {
-		int c = utf8byte(&cur);
+DEFINE_STATIC_CALL(unicode_strncmp_static_call, unicode_strncmp_default);
+EXPORT_STATIC_CALL(unicode_strncmp_static_call);
 
-		dest[nlen] = c;
-		if (!c)
-			return nlen;
-		if (c == -1)
-			break;
-	}
-	return -EINVAL;
-}
-EXPORT_SYMBOL(unicode_normalize);
+DEFINE_STATIC_CALL(unicode_strncasecmp_static_call,
+		   unicode_strncasecmp_default);
+EXPORT_STATIC_CALL(unicode_strncasecmp_static_call);
 
-static int unicode_parse_version(const char *version, unsigned int *maj,
-				 unsigned int *min, unsigned int *rev)
-{
-	substring_t args[3];
-	char version_string[12];
-	static const struct match_token token[] = {
-		{1, "%d.%d.%d"},
-		{0, NULL}
-	};
-	int ret = strscpy(version_string, version, sizeof(version_string));
+DEFINE_STATIC_CALL(unicode_strncasecmp_folded_static_call,
+		   unicode_strncasecmp_folded_default);
+EXPORT_STATIC_CALL(unicode_strncasecmp_folded_static_call);
 
-	if (ret < 0)
-		return ret;
+DEFINE_STATIC_CALL(unicode_normalize_static_call, unicode_normalize_default);
+EXPORT_STATIC_CALL(unicode_normalize_static_call);
 
-	if (match_token(version_string, token, args) != 1)
-		return -EINVAL;
+DEFINE_STATIC_CALL(unicode_casefold_static_call, unicode_casefold_default);
+EXPORT_STATIC_CALL(unicode_casefold_static_call);
 
-	if (match_int(&args[0], maj) || match_int(&args[1], min) ||
-	    match_int(&args[2], rev))
-		return -EINVAL;
+DEFINE_STATIC_CALL(unicode_casefold_hash_static_call,
+		   unicode_casefold_hash_default);
+EXPORT_STATIC_CALL(unicode_casefold_hash_static_call);
 
-	return 0;
+DEFINE_STATIC_CALL(unicode_load_static_call, unicode_load_default);
+EXPORT_STATIC_CALL(unicode_load_static_call);
+
+static int utf8mod_get(void)
+{
+	int ret;
+
+	spin_lock(&utf8mod_lock);
+	ret = utf8mod_loaded && try_module_get(utf8mod);
+	spin_unlock(&utf8mod_lock);
+	return ret;
 }
 
 struct unicode_map *unicode_load(const char *version)
 {
-	struct unicode_map *um = NULL;
-	int unicode_version;
-
-	if (version) {
-		unsigned int maj, min, rev;
-
-		if (unicode_parse_version(version, &maj, &min, &rev) < 0)
-			return ERR_PTR(-EINVAL);
-
-		if (!utf8version_is_supported(maj, min, rev))
-			return ERR_PTR(-EINVAL);
-
-		unicode_version = UNICODE_AGE(maj, min, rev);
-	} else {
-		unicode_version = utf8version_latest();
-		printk(KERN_WARNING"UTF-8 version not specified. "
-		       "Assuming latest supported version (%d.%d.%d).",
-		       (unicode_version >> 16) & 0xff,
-		       (unicode_version >> 8) & 0xff,
-		       (unicode_version & 0xff));
+	struct unicode_map *um;
+
+	/*
+	 * try_then_request_module() is used here instead of using
+	 * request_module() because of the following problems that
+	 * could occur with the usage of request_module().
+	 * 1) Multiple calls in parallel to unicode_load() would fail if
+	 * kmod_concurrent_max == 0
+	 * 2) There would be unnecessary memory allocation and userspace
+	 * invocation in call_modprobe() that would always happen even if
+	 * the module is already loaded.
+	 * Hence, using try_then_request_module() would first check if the
+	 * module is already loaded, if not then it calls the request_module()
+	 * and finally would aquire the reference of the loaded module.
+	 */
+	if (!try_then_request_module(utf8mod_get(), "utf8")) {
+		pr_err("Failed to load UTF-8 module\n");
+		return ERR_PTR(-ENODEV);
 	}
-
-	um = kzalloc(sizeof(struct unicode_map), GFP_KERNEL);
-	if (!um)
-		return ERR_PTR(-ENOMEM);
-
-	um->charset = "UTF-8";
-	um->version = unicode_version;
+	um = static_call(unicode_load_static_call)(version);
+	if (IS_ERR(um))
+		module_put(utf8mod);
 
 	return um;
 }
@@ -230,8 +147,29 @@ EXPORT_SYMBOL(unicode_load);
 
 void unicode_unload(struct unicode_map *um)
 {
-	kfree(um);
+	if (um) {
+		kfree(um);
+		module_put(utf8mod);
+	}
 }
 EXPORT_SYMBOL(unicode_unload);
 
+void unicode_register(struct module *owner)
+{
+	spin_lock(&utf8mod_lock);
+	utf8mod = owner;
+	utf8mod_loaded = true;
+	spin_unlock(&utf8mod_lock);
+}
+EXPORT_SYMBOL(unicode_register);
+
+void unicode_unregister(void)
+{
+	spin_lock(&utf8mod_lock);
+	utf8mod = NULL;
+	utf8mod_loaded = false;
+	spin_unlock(&utf8mod_lock);
+}
+EXPORT_SYMBOL(unicode_unregister);
+
 MODULE_LICENSE("GPL v2");
diff --git a/fs/unicode/unicode-utf8.c b/fs/unicode/unicode-utf8.c
new file mode 100644
index 000000000000..e0180f1c5ea8
--- /dev/null
+++ b/fs/unicode/unicode-utf8.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/parser.h>
+#include <linux/errno.h>
+#include <linux/unicode.h>
+#include <linux/stringhash.h>
+#include <linux/static_call.h>
+
+#include "utf8n.h"
+
+static int utf8_validate(const struct unicode_map *um, const struct qstr *str)
+{
+	const struct utf8data *data = utf8nfdi(um->version);
+
+	if (utf8nlen(data, str->name, str->len) < 0)
+		return -1;
+	return 0;
+}
+
+static int utf8_strncmp(const struct unicode_map *um,
+			const struct qstr *s1, const struct qstr *s2)
+{
+	const struct utf8data *data = utf8nfdi(um->version);
+	struct utf8cursor cur1, cur2;
+	int c1, c2;
+
+	if (utf8ncursor(&cur1, data, s1->name, s1->len) < 0)
+		return -EINVAL;
+
+	if (utf8ncursor(&cur2, data, s2->name, s2->len) < 0)
+		return -EINVAL;
+
+	do {
+		c1 = utf8byte(&cur1);
+		c2 = utf8byte(&cur2);
+
+		if (c1 < 0 || c2 < 0)
+			return -EINVAL;
+		if (c1 != c2)
+			return 1;
+	} while (c1);
+
+	return 0;
+}
+
+static int utf8_strncasecmp(const struct unicode_map *um,
+			    const struct qstr *s1, const struct qstr *s2)
+{
+	const struct utf8data *data = utf8nfdicf(um->version);
+	struct utf8cursor cur1, cur2;
+	int c1, c2;
+
+	if (utf8ncursor(&cur1, data, s1->name, s1->len) < 0)
+		return -EINVAL;
+
+	if (utf8ncursor(&cur2, data, s2->name, s2->len) < 0)
+		return -EINVAL;
+
+	do {
+		c1 = utf8byte(&cur1);
+		c2 = utf8byte(&cur2);
+
+		if (c1 < 0 || c2 < 0)
+			return -EINVAL;
+		if (c1 != c2)
+			return 1;
+	} while (c1);
+
+	return 0;
+}
+
+/* String cf is expected to be a valid UTF-8 casefolded
+ * string.
+ */
+static int utf8_strncasecmp_folded(const struct unicode_map *um,
+				   const struct qstr *cf,
+				   const struct qstr *s1)
+{
+	const struct utf8data *data = utf8nfdicf(um->version);
+	struct utf8cursor cur1;
+	int c1, c2;
+	int i = 0;
+
+	if (utf8ncursor(&cur1, data, s1->name, s1->len) < 0)
+		return -EINVAL;
+
+	do {
+		c1 = utf8byte(&cur1);
+		c2 = cf->name[i++];
+		if (c1 < 0)
+			return -EINVAL;
+		if (c1 != c2)
+			return 1;
+	} while (c1);
+
+	return 0;
+}
+
+static int utf8_casefold(const struct unicode_map *um, const struct qstr *str,
+			 unsigned char *dest, size_t dlen)
+{
+	const struct utf8data *data = utf8nfdicf(um->version);
+	struct utf8cursor cur;
+	size_t nlen = 0;
+
+	if (utf8ncursor(&cur, data, str->name, str->len) < 0)
+		return -EINVAL;
+
+	for (nlen = 0; nlen < dlen; nlen++) {
+		int c = utf8byte(&cur);
+
+		dest[nlen] = c;
+		if (!c)
+			return nlen;
+		if (c == -1)
+			break;
+	}
+	return -EINVAL;
+}
+
+static int utf8_casefold_hash(const struct unicode_map *um, const void *salt,
+			      struct qstr *str)
+{
+	const struct utf8data *data = utf8nfdicf(um->version);
+	struct utf8cursor cur;
+	int c;
+	unsigned long hash = init_name_hash(salt);
+
+	if (utf8ncursor(&cur, data, str->name, str->len) < 0)
+		return -EINVAL;
+
+	while ((c = utf8byte(&cur))) {
+		if (c < 0)
+			return -EINVAL;
+		hash = partial_name_hash((unsigned char)c, hash);
+	}
+	str->hash = end_name_hash(hash);
+	return 0;
+}
+
+static int utf8_normalize(const struct unicode_map *um, const struct qstr *str,
+			  unsigned char *dest, size_t dlen)
+{
+	const struct utf8data *data = utf8nfdi(um->version);
+	struct utf8cursor cur;
+	ssize_t nlen = 0;
+
+	if (utf8ncursor(&cur, data, str->name, str->len) < 0)
+		return -EINVAL;
+
+	for (nlen = 0; nlen < dlen; nlen++) {
+		int c = utf8byte(&cur);
+
+		dest[nlen] = c;
+		if (!c)
+			return nlen;
+		if (c == -1)
+			break;
+	}
+	return -EINVAL;
+}
+
+static int utf8_parse_version(const char *version, unsigned int *maj,
+			      unsigned int *min, unsigned int *rev)
+{
+	substring_t args[3];
+	char version_string[12];
+	static const struct match_token token[] = {
+		{1, "%d.%d.%d"},
+		{0, NULL}
+	};
+
+	int ret = strscpy(version_string, version, sizeof(version_string));
+
+	if (ret < 0)
+		return ret;
+
+	if (match_token(version_string, token, args) != 1)
+		return -EINVAL;
+
+	if (match_int(&args[0], maj) || match_int(&args[1], min) ||
+	    match_int(&args[2], rev))
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct unicode_map *utf8_load(const char *version)
+{
+	struct unicode_map *um = NULL;
+	int unicode_version;
+
+	if (version) {
+		unsigned int maj, min, rev;
+
+		if (utf8_parse_version(version, &maj, &min, &rev) < 0)
+			return ERR_PTR(-EINVAL);
+
+		if (!utf8version_is_supported(maj, min, rev))
+			return ERR_PTR(-EINVAL);
+
+		unicode_version = UNICODE_AGE(maj, min, rev);
+	} else {
+		unicode_version = utf8version_latest();
+		pr_warn("UTF-8 version not specified. Assuming latest supported version (%d.%d.%d).",
+			(unicode_version >> 16) & 0xff,
+			(unicode_version >> 8) & 0xff,
+			(unicode_version & 0xfe));
+	}
+
+	um = kzalloc(sizeof(*um), GFP_KERNEL);
+	if (!um)
+		return ERR_PTR(-ENOMEM);
+
+	um->charset = "UTF-8";
+	um->version = unicode_version;
+
+	return um;
+}
+
+static int __init utf8_init(void)
+{
+	static_call_update(unicode_validate_static_call, utf8_validate);
+	static_call_update(unicode_strncmp_static_call, utf8_strncmp);
+	static_call_update(unicode_strncasecmp_static_call, utf8_strncasecmp);
+	static_call_update(unicode_strncasecmp_folded_static_call,
+			   utf8_strncasecmp_folded);
+	static_call_update(unicode_normalize_static_call, utf8_normalize);
+	static_call_update(unicode_casefold_static_call, utf8_casefold);
+	static_call_update(unicode_casefold_hash_static_call,
+			   utf8_casefold_hash);
+	static_call_update(unicode_load_static_call, utf8_load);
+
+	unicode_register(THIS_MODULE);
+	return 0;
+}
+
+static void __exit utf8_exit(void)
+{
+	static_call_update(unicode_validate_static_call,
+			   unicode_validate_default);
+	static_call_update(unicode_strncmp_static_call, unicode_strncmp_default);
+	static_call_update(unicode_strncasecmp_static_call,
+			   unicode_strncasecmp_default);
+	static_call_update(unicode_strncasecmp_folded_static_call,
+			   unicode_strncasecmp_folded_default);
+	static_call_update(unicode_normalize_static_call,
+			   unicode_normalize_default);
+	static_call_update(unicode_casefold_static_call,
+			   unicode_casefold_default);
+	static_call_update(unicode_casefold_hash_static_call,
+			   unicode_casefold_hash_default);
+	static_call_update(unicode_load_static_call, unicode_load_default);
+
+	unicode_unregister();
+}
+
+module_init(utf8_init);
+module_exit(utf8_exit);
+
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/unicode.h b/include/linux/unicode.h
index de23f9ee720b..0b157c6830c6 100644
--- a/include/linux/unicode.h
+++ b/include/linux/unicode.h
@@ -4,33 +4,101 @@
 
 #include <linux/init.h>
 #include <linux/dcache.h>
+#include <linux/static_call.h>
+
 
 struct unicode_map {
 	const char *charset;
 	int version;
 };
 
-int unicode_validate(const struct unicode_map *um, const struct qstr *str);
+int unicode_validate_default(const struct unicode_map *um,
+			     const struct qstr *str);
+
+int unicode_strncmp_default(const struct unicode_map *um,
+			    const struct qstr *s1,
+			    const struct qstr *s2);
+
+int unicode_strncasecmp_default(const struct unicode_map *um,
+				const struct qstr *s1,
+				const struct qstr *s2);
+
+int unicode_strncasecmp_folded_default(const struct unicode_map *um,
+				       const struct qstr *cf,
+				       const struct qstr *s1);
+
+int unicode_normalize_default(const struct unicode_map *um,
+			      const struct qstr *str,
+			      unsigned char *dest, size_t dlen);
+
+int unicode_casefold_default(const struct unicode_map *um,
+			     const struct qstr *str,
+			     unsigned char *dest, size_t dlen);
+
+int unicode_casefold_hash_default(const struct unicode_map *um,
+				  const void *salt, struct qstr *str);
 
-int unicode_strncmp(const struct unicode_map *um,
-		    const struct qstr *s1, const struct qstr *s2);
+struct unicode_map *unicode_load_default(const char *version);
 
-int unicode_strncasecmp(const struct unicode_map *um,
-			const struct qstr *s1, const struct qstr *s2);
-int unicode_strncasecmp_folded(const struct unicode_map *um,
-			       const struct qstr *cf,
-			       const struct qstr *s1);
+DECLARE_STATIC_CALL(unicode_validate_static_call, unicode_validate_default);
+DECLARE_STATIC_CALL(unicode_strncmp_static_call, unicode_strncmp_default);
+DECLARE_STATIC_CALL(unicode_strncasecmp_static_call,
+		    unicode_strncasecmp_default);
+DECLARE_STATIC_CALL(unicode_strncasecmp_folded_static_call,
+		    unicode_strncasecmp_folded_default);
+DECLARE_STATIC_CALL(unicode_normalize_static_call, unicode_normalize_default);
+DECLARE_STATIC_CALL(unicode_casefold_static_call, unicode_casefold_default);
+DECLARE_STATIC_CALL(unicode_casefold_hash_static_call,
+		    unicode_casefold_hash_default);
+DECLARE_STATIC_CALL(unicode_load_static_call, unicode_load_default);
 
-int unicode_normalize(const struct unicode_map *um, const struct qstr *str,
-		      unsigned char *dest, size_t dlen);
 
-int unicode_casefold(const struct unicode_map *um, const struct qstr *str,
-		     unsigned char *dest, size_t dlen);
+static inline int unicode_validate(const struct unicode_map *um, const struct qstr *str)
+{
+	return static_call(unicode_validate_static_call)(um, str);
+}
 
-int unicode_casefold_hash(const struct unicode_map *um, const void *salt,
-			  struct qstr *str);
+static inline int unicode_strncmp(const struct unicode_map *um,
+				  const struct qstr *s1, const struct qstr *s2)
+{
+	return static_call(unicode_strncmp_static_call)(um, s1, s2);
+}
+
+static inline int unicode_strncasecmp(const struct unicode_map *um,
+				      const struct qstr *s1, const struct qstr *s2)
+{
+	return static_call(unicode_strncasecmp_static_call)(um, s1, s2);
+}
+
+static inline int unicode_strncasecmp_folded(const struct unicode_map *um,
+					     const struct qstr *cf,
+					     const struct qstr *s1)
+{
+	return static_call(unicode_strncasecmp_folded_static_call)(um, cf, s1);
+}
+
+static inline int unicode_normalize(const struct unicode_map *um, const struct qstr *str,
+				    unsigned char *dest, size_t dlen)
+{
+	return static_call(unicode_normalize_static_call)(um, str, dest, dlen);
+}
+
+static inline int unicode_casefold(const struct unicode_map *um, const struct qstr *str,
+				   unsigned char *dest, size_t dlen)
+{
+	return static_call(unicode_casefold_static_call)(um, str, dest, dlen);
+}
+
+static inline int unicode_casefold_hash(const struct unicode_map *um, const void *salt,
+					struct qstr *str)
+{
+	return static_call(unicode_casefold_hash_static_call)(um, salt, str);
+}
 
 struct unicode_map *unicode_load(const char *version);
 void unicode_unload(struct unicode_map *um);
 
+void unicode_register(struct module *owner);
+void unicode_unregister(void);
+
 #endif /* _LINUX_UNICODE_H */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-23 20:51 ` [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
@ 2021-04-27  6:29   ` Christoph Hellwig
  2021-04-27 10:09     ` Shreeya Patel
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2021-04-27  6:29 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0, linux-ext4, linux-kernel, linux-f2fs-devel,
	linux-fsdevel, kernel, andre.almeida

On Sat, Apr 24, 2021 at 02:21:36AM +0530, Shreeya Patel wrote:
> utf8data.h_shipped has a large database table which is an auto-generated
> decodification trie for the unicode normalization functions.
> We can avoid carrying this large table in the kernel unless it is required
> by the filesystem during boot process.
> 
> Hence, make UTF-8 encoding loadable by converting it into a module and
> also add built-in UTF-8 support option for compiling it into the
> kernel whenever required by the filesystem.

The way this is implemement looks rather awkward.

Given that the large memory usage is for a data table and not for code,
why not treat is as a firmware blob and load it using request_firmware?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-27  6:29   ` Christoph Hellwig
@ 2021-04-27 10:09     ` Shreeya Patel
  2021-04-27 14:50       ` Theodore Ts'o
  0 siblings, 1 reply; 15+ messages in thread
From: Shreeya Patel @ 2021-04-27 10:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: tytso, adilger.kernel, jaegeuk, chao, krisman, ebiggers, drosen,
	ebiggers, yuchao0, linux-ext4, linux-kernel, linux-f2fs-devel,
	linux-fsdevel, kernel, andre.almeida


On 27/04/21 11:59 am, Christoph Hellwig wrote:
> On Sat, Apr 24, 2021 at 02:21:36AM +0530, Shreeya Patel wrote:
>> utf8data.h_shipped has a large database table which is an auto-generated
>> decodification trie for the unicode normalization functions.
>> We can avoid carrying this large table in the kernel unless it is required
>> by the filesystem during boot process.
>>
>> Hence, make UTF-8 encoding loadable by converting it into a module and
>> also add built-in UTF-8 support option for compiling it into the
>> kernel whenever required by the filesystem.
> The way this is implemement looks rather awkward.
>
> Given that the large memory usage is for a data table and not for code,
> why not treat is as a firmware blob and load it using request_firmware?


utf8 module not just has the data table but also has some kernel code.
The big part that we are trying to keep out of the kernel is a tree 
structure that gets traversed based on a key that is the file name.
This is done when issuing a lookup in the filesystem, which has to be 
very fast. So maybe it would not be so good to use request_firmware for
such a core feature.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-27 10:09     ` Shreeya Patel
@ 2021-04-27 14:50       ` Theodore Ts'o
  2021-04-27 15:06         ` Gabriel Krisman Bertazi
  0 siblings, 1 reply; 15+ messages in thread
From: Theodore Ts'o @ 2021-04-27 14:50 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: Christoph Hellwig, adilger.kernel, jaegeuk, chao, krisman,
	ebiggers, drosen, ebiggers, yuchao0, linux-ext4, linux-kernel,
	linux-f2fs-devel, linux-fsdevel, kernel, andre.almeida

On Tue, Apr 27, 2021 at 03:39:15PM +0530, Shreeya Patel wrote:
> > > Hence, make UTF-8 encoding loadable by converting it into a module and
> > > also add built-in UTF-8 support option for compiling it into the
> > > kernel whenever required by the filesystem.
> > The way this is implemement looks rather awkward.

I think that's a bit awkard is the trying to create an abstraction
separation between the unicode and utf8 layers, just in case, at some
point, we want fs/unicode to support more than just utf8.

I think we're better off being opinionated here, and say that the only
unicode encoding that will be supported by the kernel is UTF-8.
Period.  In which case, we don't need to try to insert this unneeded
abstraction layer.

If you really want to make make fs/unicode support more than one
encoding --- say, UTF-16LE, as used by NTFS --- at that point we can
think about what the abstractions should look like.  For example, it
doesn't _actually_ make sense for the data-trie structures to be part
of the utf-8 encoding.  The normalization tables are for Unicode, and
it wouldn't make sense for UTF-16 to have its own normalization
tables, bloating the kernel even more.

It *is* true that the normalization tables have been optimized for
utf-8, because that's what the whole world actually uses; utf-16le is
really a legacy use case.  So presumably, we would probably find a way
to code up the utf-16 functions in a way that used the utf-8 data
tables, even if it wasn't 100% optimal in terms of speed.

But it's probably not worth it at this point.

> > Given that the large memory usage is for a data table and not for code,
> > why not treat is as a firmware blob and load it using request_firmware?
> 
> utf8 module not just has the data table but also has some kernel code.
> The big part that we are trying to keep out of the kernel is a tree
> structure that gets traversed based on a key that is the file name.
> This is done when issuing a lookup in the filesystem, which has to be very
> fast. So maybe it would not be so good to use request_firmware for
> such a core feature.

Speed really isn't a great argument here; the request_firmware is
something that would only need to be done once, when a file system
which requires Unicode normalization and/or case-folding is mounted.

I think the better argument to make is just one of simplicity;
separating the Unicode data table from the kernel adds complexity.  It
also reduces flexibility, since for use cases where it's actually
_preferable_ to have Unicode functionality permanently built-in the
kernel, we now force the use of some kind of initial ramdisk to load a
module before the root file system (which might require Unicode
support) could even be mounted.

The argument *for* making the Unicode table be a loadable firmware is
that it might make it possible to upgrade to a newer version of
Unicode without needing to do a kernel recompile.  On average, Unicode
relases a new to support new character sets every year or so, or when
there Japanese Emperor requiring a new reign name :-).  Usually the
new character sets are for obscure ancient alphabets, and so it's
really not a big deal if the kernel doesn't support, say,
Chorasmian[1] or Dives Akuru[2].  Perhaps people would make a much
bigger deal about new Emoji characters, or new code points for the
Creative Commons symbols.  I'm personally not excited enough to claim
that it's worth the extra complexity, but some people might think so.  :-)

[1] used in Central Asia across Uzbekistan, Kazakhstan, and
Turkmenistan to write an extinct Eastern Iranian language.

[2] historically used in the Maldives until the 20th century.

Of course, using those new Emoji symbols in file names would reduce
portability of that file system if Strict Normalization was mandated.
Fortunately, ext4 and f2fs don't enable strict normalizaation by
default, which is also good, because it means if we don't have the
latest Unicode update in the kernel, it doesn't really matter that
much.... again, not worth the extra complexity/headache IMHO.

Cheers,

					- Ted

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-27 14:50       ` Theodore Ts'o
@ 2021-04-27 15:06         ` Gabriel Krisman Bertazi
  2021-04-28 14:12           ` Theodore Ts'o
  0 siblings, 1 reply; 15+ messages in thread
From: Gabriel Krisman Bertazi @ 2021-04-27 15:06 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Shreeya Patel, Christoph Hellwig, adilger.kernel, jaegeuk, chao,
	ebiggers, drosen, ebiggers, yuchao0, linux-ext4, linux-kernel,
	linux-f2fs-devel, linux-fsdevel, kernel, andre.almeida

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Tue, Apr 27, 2021 at 03:39:15PM +0530, Shreeya Patel wrote:
>> > > Hence, make UTF-8 encoding loadable by converting it into a module and
>> > > also add built-in UTF-8 support option for compiling it into the
>> > > kernel whenever required by the filesystem.
>> > The way this is implemement looks rather awkward.
>
> I think that's a bit awkard is the trying to create an abstraction
> separation between the unicode and utf8 layers, just in case, at some
> point, we want fs/unicode to support more than just utf8.
>
> I think we're better off being opinionated here, and say that the only
> unicode encoding that will be supported by the kernel is UTF-8.
> Period.  In which case, we don't need to try to insert this unneeded
> abstraction layer.
>
> If you really want to make make fs/unicode support more than one
> encoding --- say, UTF-16LE, as used by NTFS --- at that point we can
> think about what the abstractions should look like.  For example, it
> doesn't _actually_ make sense for the data-trie structures to be part
> of the utf-8 encoding.  The normalization tables are for Unicode, and
> it wouldn't make sense for UTF-16 to have its own normalization
> tables, bloating the kernel even more.
>
> It *is* true that the normalization tables have been optimized for
> utf-8, because that's what the whole world actually uses; utf-16le is
> really a legacy use case.  So presumably, we would probably find a way
> to code up the utf-16 functions in a way that used the utf-8 data
> tables, even if it wasn't 100% optimal in terms of speed.
>
> But it's probably not worth it at this point.
>
>> > Given that the large memory usage is for a data table and not for code,
>> > why not treat is as a firmware blob and load it using request_firmware?
>> 
>> utf8 module not just has the data table but also has some kernel code.
>> The big part that we are trying to keep out of the kernel is a tree
>> structure that gets traversed based on a key that is the file name.
>> This is done when issuing a lookup in the filesystem, which has to be very
>> fast. So maybe it would not be so good to use request_firmware for
>> such a core feature.
>
> Speed really isn't a great argument here; the request_firmware is
> something that would only need to be done once, when a file system
> which requires Unicode normalization and/or case-folding is mounted.
>
> I think the better argument to make is just one of simplicity;
> separating the Unicode data table from the kernel adds complexity.  It
> also reduces flexibility, since for use cases where it's actually
> _preferable_ to have Unicode functionality permanently built-in the
> kernel, we now force the use of some kind of initial ramdisk to load a
> module before the root file system (which might require Unicode
> support) could even be mounted.

FWIW, embedding FW images to the kernel is also well supported.  Making
the data trie a firmware doesn't make a ramdisk more of a requirement
than the module solution, I think.

> The argument *for* making the Unicode table be a loadable firmware is
> that it might make it possible to upgrade to a newer version of
> Unicode without needing to do a kernel recompile.  On average, Unicode
> relases a new to support new character sets every year or so, or when
> there Japanese Emperor requiring a new reign name :-).  Usually the
> new character sets are for obscure ancient alphabets, and so it's
> really not a big deal if the kernel doesn't support, say,
> Chorasmian[1] or Dives Akuru[2].  Perhaps people would make a much
> bigger deal about new Emoji characters, or new code points for the
> Creative Commons symbols.  I'm personally not excited enough to claim
> that it's worth the extra complexity, but some people might think so.
> :-)

We don't really care about emojis since they are not usually
normalized/folded, and unless you are using strict mode, they will be
invisible for the user. On a unrelated note, newer scripts are more
interesting and we should come up with some update policy someday, since
we are already lagging the unicode spec.  At least we are still in the
Reiwa Era, which was first supported in 12.1 :)

>
> [1] used in Central Asia across Uzbekistan, Kazakhstan, and
> Turkmenistan to write an extinct Eastern Iranian language.
>
> [2] historically used in the Maldives until the 20th century.
>
> Of course, using those new Emoji symbols in file names would reduce
> portability of that file system if Strict Normalization was mandated.
> Fortunately, ext4 and f2fs don't enable strict normalizaation by
> default, which is also good, because it means if we don't have the
> latest Unicode update in the kernel, it doesn't really matter that
> much.... again, not worth the extra complexity/headache IMHO.

ah yes, exactly.

>
> Cheers,
>
> 					- Ted

-- 
Gabriel Krisman Bertazi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-27 15:06         ` Gabriel Krisman Bertazi
@ 2021-04-28 14:12           ` Theodore Ts'o
  2021-04-28 18:58             ` Gabriel Krisman Bertazi
  0 siblings, 1 reply; 15+ messages in thread
From: Theodore Ts'o @ 2021-04-28 14:12 UTC (permalink / raw)
  To: Gabriel Krisman Bertazi
  Cc: Shreeya Patel, Christoph Hellwig, adilger.kernel, jaegeuk, chao,
	ebiggers, drosen, ebiggers, yuchao0, linux-ext4, linux-kernel,
	linux-f2fs-devel, linux-fsdevel, kernel, andre.almeida

On Tue, Apr 27, 2021 at 11:06:33AM -0400, Gabriel Krisman Bertazi wrote:
> > I think the better argument to make is just one of simplicity;
> > separating the Unicode data table from the kernel adds complexity.  It
> > also reduces flexibility, since for use cases where it's actually
> > _preferable_ to have Unicode functionality permanently built-in the
> > kernel, we now force the use of some kind of initial ramdisk to load a
> > module before the root file system (which might require Unicode
> > support) could even be mounted.
> 
> FWIW, embedding FW images to the kernel is also well supported.  Making
> the data trie a firmware doesn't make a ramdisk more of a requirement
> than the module solution, I think.

I don't think we support building firmware directly into the kernel
any more.  We used to, but IIRC, there was the feeling that 99.99% of
the time, firmware modules were not GPL compliant, and so we ripped
out that support.

So my point was with the module support, it's *optional* that it be
compiled as a module, which is convenient for those use cases, such as
for example a mobile handset --- where there is no need for modules
since the hardware doesn't change, and so modules and an initrd is
just unnecessary complexity --- and firmware, which would make an
initial ramdisk mandatory if you wanted to use the casefold feature.

Put another way, the only reason why putting the unicode tables in a
module is to make life easier for desktop distros.  For mobile
handsets, modules are an anti-feature, which is why there was no call
for supporting this initially, given the initial use case for the
casefold feature.

Cheers,

					- Ted

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-04-28 14:12           ` Theodore Ts'o
@ 2021-04-28 18:58             ` Gabriel Krisman Bertazi
       [not found]               ` <7caab939-2800-0cc2-7b65-345af3fce73d@collabora.com>
  0 siblings, 1 reply; 15+ messages in thread
From: Gabriel Krisman Bertazi @ 2021-04-28 18:58 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Shreeya Patel, Christoph Hellwig, adilger.kernel, jaegeuk, chao,
	ebiggers, drosen, ebiggers, yuchao0, linux-ext4, linux-kernel,
	linux-f2fs-devel, linux-fsdevel, kernel, andre.almeida

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Tue, Apr 27, 2021 at 11:06:33AM -0400, Gabriel Krisman Bertazi wrote:
>> > I think the better argument to make is just one of simplicity;
>> > separating the Unicode data table from the kernel adds complexity.  It
>> > also reduces flexibility, since for use cases where it's actually
>> > _preferable_ to have Unicode functionality permanently built-in the
>> > kernel, we now force the use of some kind of initial ramdisk to load a
>> > module before the root file system (which might require Unicode
>> > support) could even be mounted.
>> 
>> FWIW, embedding FW images to the kernel is also well supported.  Making
>> the data trie a firmware doesn't make a ramdisk more of a requirement
>> than the module solution, I think.
>
> I don't think we support building firmware directly into the kernel
> any more.  We used to, but IIRC, there was the feeling that 99.99% of
> the time, firmware modules were not GPL compliant, and so we ripped
> out that support.

Support is still there on 5.12. See
Documentation/driver-api/firmware/built-in-fw.rst.

Personally, I use this feature very often on my development workflow,
for similar reasons to what you said below. In my case, avoiding the
complexity of initramfs but still being able to use my crappy
FW-dependent NIC card to boot from NFS. :)

> compiled as a module, which is convenient for those use cases, such as
> for example a mobile handset --- where there is no need for modules
> since the hardware doesn't change, and so modules and an initrd is
> just unnecessary complexity --- and firmware, which would make an
> initial ramdisk mandatory if you wanted to use the casefold feature.
>
> Put another way, the only reason why putting the unicode tables in a
> module is to make life easier for desktop distros.  For mobile
> handsets, modules are an anti-feature, which is why there was no call
> for supporting this initially, given the initial use case for the
> casefold feature.

What about support for firmware generation from the kernel tree and
installation to /lib/firmware? With a module, I can just call
modules_install, and dealing with modules is hardcoded in the mind of
every kernel developer.  Dealing with firmwares inside the kernel tree
is not common, and I didn't find an equivalent Makefile rule to build
and deploy firmwares on a path that firmware_loader knows about.

I think of firmware as code/data for a device, while modules is for the
kernel domain, even if it is a gross oversimplification.  Are there
other examples of firmware built from the kernel tree that are meant
exclusively to be used by the kernel, without hardware involvement?

For mobile devices, it wouldn't really matter whether it is built-in or
a firmware, right?  On a controlled environment like Android, you know
what to expect of your filesystem, so you know beforehand if your kernel
needs the table loaded (apart from sd cards.  Do people use ext4 for
sdcards in Android or is it all exfat?).  In those scenarios, you gain
very little by not making it built-in.

-- 
Gabriel Krisman Bertazi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
       [not found]               ` <7caab939-2800-0cc2-7b65-345af3fce73d@collabora.com>
@ 2021-05-11  4:35                 ` Christoph Hellwig
  2021-05-20 20:19                   ` Shreeya Patel
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2021-05-11  4:35 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: Gabriel Krisman Bertazi, Theodore Ts'o, Christoph Hellwig,
	adilger.kernel, jaegeuk, chao, ebiggers, drosen, ebiggers,
	yuchao0, linux-ext4, linux-kernel, linux-f2fs-devel,
	linux-fsdevel, kernel, andre.almeida

On Tue, May 11, 2021 at 02:17:00AM +0530, Shreeya Patel wrote:
> Theodore / Christoph, since we haven't come up with any final decision with
> this discussion, how do you think we should proceed on this?

I think loading it as a firmware-like table is much preferable to
a module with all the static call magic papering over that it really is
just one specific table.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-05-11  4:35                 ` Christoph Hellwig
@ 2021-05-20 20:19                   ` Shreeya Patel
  2021-06-03  0:07                     ` Gabriel Krisman Bertazi
  0 siblings, 1 reply; 15+ messages in thread
From: Shreeya Patel @ 2021-05-20 20:19 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Gabriel Krisman Bertazi, Theodore Ts'o, adilger.kernel,
	jaegeuk, chao, ebiggers, drosen, ebiggers, yuchao0, linux-ext4,
	linux-kernel, linux-f2fs-devel, linux-fsdevel, kernel


On 11/05/21 10:05 am, Christoph Hellwig wrote:
> On Tue, May 11, 2021 at 02:17:00AM +0530, Shreeya Patel wrote:
>> Theodore / Christoph, since we haven't come up with any final decision with
>> this discussion, how do you think we should proceed on this?
> I think loading it as a firmware-like table is much preferable to
> a module with all the static call magic papering over that it really is
> just one specific table.


Christoph, I get you point here but request_firmware API requires a 
device pointer and I don't
see anywhere it being NULL so I am not sure if we are going in the right 
way by loading the data as firmware like table.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-05-20 20:19                   ` Shreeya Patel
@ 2021-06-03  0:07                     ` Gabriel Krisman Bertazi
  2021-06-16  4:09                       ` Christoph Hellwig
  0 siblings, 1 reply; 15+ messages in thread
From: Gabriel Krisman Bertazi @ 2021-06-03  0:07 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: Christoph Hellwig, Theodore Ts'o, adilger.kernel, jaegeuk,
	chao, ebiggers, drosen, ebiggers, yuchao0, linux-ext4,
	linux-kernel, linux-f2fs-devel, linux-fsdevel, kernel

Shreeya Patel <shreeya.patel@collabora.com> writes:

> On 11/05/21 10:05 am, Christoph Hellwig wrote:
>> On Tue, May 11, 2021 at 02:17:00AM +0530, Shreeya Patel wrote:
>>> Theodore / Christoph, since we haven't come up with any final decision with
>>> this discussion, how do you think we should proceed on this?
>> I think loading it as a firmware-like table is much preferable to
>> a module with all the static call magic papering over that it really is
>> just one specific table.
>
>
> Christoph, I get you point here but request_firmware API requires a
> device pointer and I don't
> see anywhere it being NULL so I am not sure if we are going in the right
> way by loading the data as firmware like table.

I wasn't going to really oppose it from being a firmware but this
detail, if required, makes the whole firmware idea more awkward.  If the
whole reason to make it a firmware is to avoid the module boilerplate,
this is just different boilerplate.  Once again, I don't know about
precedent of kernel data as a module, and there is the problem with
Makefile rules to install this stuff, that I mentioned.

We know we can get rid of the static call stuff already, since we likely
won't support more encodings anyway, so that would simplify a lot the
module specific code.

-- 
Gabriel Krisman Bertazi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer
  2021-06-03  0:07                     ` Gabriel Krisman Bertazi
@ 2021-06-16  4:09                       ` Christoph Hellwig
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2021-06-16  4:09 UTC (permalink / raw)
  To: Gabriel Krisman Bertazi
  Cc: Shreeya Patel, Christoph Hellwig, Theodore Ts'o,
	adilger.kernel, jaegeuk, chao, ebiggers, drosen, ebiggers,
	yuchao0, linux-ext4, linux-kernel, linux-f2fs-devel,
	linux-fsdevel, kernel

On Wed, Jun 02, 2021 at 08:07:07PM -0400, Gabriel Krisman Bertazi wrote:
> I wasn't going to really oppose it from being a firmware but this
> detail, if required, makes the whole firmware idea more awkward.  If the
> whole reason to make it a firmware is to avoid the module boilerplate,
> this is just different boilerplate.  Once again, I don't know about
> precedent of kernel data as a module, and there is the problem with
> Makefile rules to install this stuff, that I mentioned.
> 
> We know we can get rid of the static call stuff already, since we likely
> won't support more encodings anyway, so that would simplify a lot the
> module specific code.

Well, another thing we can do is a data-only module.  That is a module
that just contains the tables, with the core code doing a symbol_get
on them.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-06-16  4:09 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 20:51 [PATCH v8 0/4] Make UTF-8 encoding loadable Shreeya Patel
2021-04-23 20:51 ` [PATCH v8 1/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
2021-04-23 20:51 ` [PATCH v8 2/4] fs: unicode: Rename function names from utf8 to unicode Shreeya Patel
2021-04-23 20:51 ` [PATCH v8 3/4] fs: unicode: Rename utf8-core file to unicode-core Shreeya Patel
2021-04-23 20:51 ` [PATCH v8 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
2021-04-27  6:29   ` Christoph Hellwig
2021-04-27 10:09     ` Shreeya Patel
2021-04-27 14:50       ` Theodore Ts'o
2021-04-27 15:06         ` Gabriel Krisman Bertazi
2021-04-28 14:12           ` Theodore Ts'o
2021-04-28 18:58             ` Gabriel Krisman Bertazi
     [not found]               ` <7caab939-2800-0cc2-7b65-345af3fce73d@collabora.com>
2021-05-11  4:35                 ` Christoph Hellwig
2021-05-20 20:19                   ` Shreeya Patel
2021-06-03  0:07                     ` Gabriel Krisman Bertazi
2021-06-16  4:09                       ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).