fs: Remove usage of broken nls_utf8 and drop it

Currently iocharset=utf8 mount option is broken and error is printed to dmesg when it is used. To use UTF-8 as iocharset, it is required to use utf8=1 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8=1 mount option and remove printing error from dmesg.

Quoted 6 lines · Pali Rohár

Currently iocharset=utf8 mount option is broken and error is printed to dmesg when it is used. To use UTF-8 as iocharset, it is required to use utf8=1 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8=1 mount option and remove printing error from dmesg.

This change is not equivalent to utf8=1. In the case of utf8=1, vfat uses iocharset's conversion table and it can handle more than ascii.

So this patch is incompatible changes, and handles less chars than utf8=1. So I think this is clean though, but this would be regression for user of utf8=1.

Thanks.

Signature · 1 line

OGAWA Hirofumi <[email protected]>

Quoted 6 lines

This change is not equivalent to utf8=1. In the case of utf8=1, vfat uses iocharset's conversion table and it can handle more than ascii.

So this patch is incompatible changes, and handles less chars than utf8=1. So I think this is clean though, but this would be regression for user of utf8=1.

I do not think so... But please correct me, as this code around is mess.

Without this change when utf8=1 is set then iocharset= encoding is used for case-insensitivity implementation (toupper / tolower conversion). For all other parts are use correct utf8* conversion functions.

But you use touppper / tolower functions from iocharset= encoding on stream of utf8 bytes then you either get identity or some unpredictable garbage in utf8. So when comparing two (different) non-ASCII filenames via this method you in most cases get that filenames are different. Because converting their utf8 bytes via toupper / tolower functions from iocharset= encoding results in two different byte sequences in most cases. Even for two utf8 case-insensitive same strings.

But you can play with it and I guess it is possible to find two different utf8 strings which after toupper / tolower conversion from some iocharset= encoding would lead to same byte sequence.

This patch uses for utf8 tolower / touppser function simple 7-bit tolower / toupper ascii function. And so for 7-bit ascii file names there is no change.

So this patch changes behavior when comparing non 7-bit ascii file names, but only in cases when previously two different file names were marked as same. As now they are marked correctly as different. So this is changed behavior, but I guess it is bug fix which is needed. If you want I can put this change into separate patch.

Issue that two case-insensitive same files are marked as different is not changed by this patch and therefore this issue stay here.

Currently iocharset=utf8 mount option is broken and error is printed to dmesg when it is used. To use UTF-8 as iocharset, it is required to use utf8=1 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8=1 mount option and remove printing error from dmesg.

FAT by definition is case-insensitive but current Linux implementation is case-sensitive for non-ASCII characters when UTF-8 is used. This patch does not change this UTF-8 behavior. Only more comments in fat_utf8_strnicmp() function are added about it.

After this patch iocharset=utf8 starts working, so there is no need to have separate config option FAT_DEFAULT_UTF8 as FAT_DEFAULT_IOCHARSET for utf8 also starts working. So remove redundant config option FAT_DEFAULT_UTF8.

Signed-off-by Pali Rohár <[email protected]>

---
 fs/fat/Kconfig      | 15 ---------------
 fs/fat/dir.c        | 17 +++++++----------
 fs/fat/fat.h        | 22 ++++++++++++++++++++++
 fs/fat/inode.c      | 28 +++++++++++-----------------
 fs/fat/namei_vfat.c | 26 +++++++++++++++++++-------
 5 files changed, 59 insertions(+), 49 deletions(-)

fs/fat/Kconfig +0 -15

index 66532a71e8fd..a31594137d5e 100644
@@ -100,18 +100,3 @@ config FAT_DEFAULT_IOCHARSET
 
 	  Enable any character sets you need in File Systems/Native Language
 	  Support.
-
-config FAT_DEFAULT_UTF8
-	bool "Enable FAT UTF-8 option by default"
-	depends on VFAT_FS
-	default n
-	help
-	  Set this if you would like to have "utf8" mount option set
-	  by default when mounting FAT filesystems.
-
-	  Even if you say Y here can always disable UTF-8 for
-	  particular mount by adding "utf8=0" to mount options.
-
-	  Say Y if you use UTF-8 encoding for file names, N otherwise.
-
-	  See <file:Documentation/filesystems/vfat.rst> for more information.

fs/fat/dir.c +7 -10

index c4a274285858..49fe8dc6e5f0 100644
@@ -33,11 +33,6 @@
 #define FAT_MAX_UNI_CHARS	((MSDOS_SLOTS - 1) * 13 + 1)
 #define FAT_MAX_UNI_SIZE	(FAT_MAX_UNI_CHARS * sizeof(wchar_t))
 
-static inline unsigned char fat_tolower(unsigned char c)
-{
-	return ((c >= 'A') && (c <= 'Z')) ? c+32 : c;
-}
-
 static inline loff_t fat_make_i_pos(struct super_block *sb,
 				    struct buffer_head *bh,
 				    struct msdos_dir_entry *de)

@@ -258,10 +253,12 @@ static inline int fat_name_match(struct msdos_sb_info *sbi,
 	if (a_len != b_len)
 		return 0;
 
-	if (sbi->options.name_check != 's')
-		return !nls_strnicmp(sbi->nls_io, a, b, a_len);
-	else
+	if (sbi->options.name_check == 's')
 		return !memcmp(a, b, a_len);
+	else if (sbi->options.utf8)
+		return !fat_utf8_strnicmp(a, b, a_len);
+	else
+		return !nls_strnicmp(sbi->nls_io, a, b, a_len);
 }
 
 enum { PARSE_INVALID = 1, PARSE_NOT_LONGNAME, PARSE_EOF, };

@@ -384,7 +381,7 @@ static int fat_parse_short(struct super_block *sb,
 					de->lcase & CASE_LOWER_BASE);
 		if (chl <= 1) {
 			if (!isvfat)
-				ptname[i] = nocase ? c : fat_tolower(c);
+				ptname[i] = nocase ? c : fat_ascii_to_lower(c);
 			i++;
 			if (c != ' ') {
 				name_len = i;

@@ -421,7 +418,7 @@ static int fat_parse_short(struct super_block *sb,
 		if (chl <= 1) {
 			k++;
 			if (!isvfat)
-				ptname[i] = nocase ? c : fat_tolower(c);
+				ptname[i] = nocase ? c : fat_ascii_to_lower(c);
 			i++;
 			if (c != ' ') {
 				name_len = i;

fs/fat/fat.h +22 -0

index 02d4d4234956..0cd15fb3b042 100644
@@ -310,6 +310,28 @@ static inline void fatwchar_to16(__u8 *dst, const wchar_t *src, size_t len)
 #endif
 }
 
+static inline unsigned char fat_ascii_to_lower(unsigned char c)
+{
+	return ((c >= 'A') && (c <= 'Z')) ? c+32 : c;
+}
+
+static inline int fat_utf8_strnicmp(const unsigned char *a,
+				    const unsigned char *b,
+				    int len)
+{
+	int i;
+
+	/*
+	 * FIXME: UTF-8 doesn't provide FAT semantics
+	 * Case-insensitive support is only for 7-bit ASCII characters
+	 */
+	for (i = 0; i < len; i++) {
+		if (fat_ascii_to_lower(a[i]) != fat_ascii_to_lower(b[i]))
+			return 1;
+	}
+	return 0;
+}
+
 /* fat/cache.c */
 extern void fat_cache_inval_inode(struct inode *inode);
 extern int fat_get_cluster(struct inode *inode, int cluster,

fs/fat/inode.c +0 -0

index de0c9b013a85..f8c8a739f8f0 100644

fs/fat/inode.c +9 -16

+		else if (sbi->nls_io)
 			seq_printf(m, ",iocharset=%s", sbi->nls_io->charset);
 
 		switch (opts->shortname) {

@@ -994,8 +996,6 @@ static int fat_show_options(struct seq_file *m, struct dentry *root)
 		if (opts->nocase)
 			seq_puts(m, ",nocase");
 	} else {
-		if (opts->utf8)
-			seq_puts(m, ",utf8");
 		if (opts->unicode_xlate)
 			seq_puts(m, ",uni_xlate");
 		if (!opts->numtail)

@@ -1157,8 +1157,6 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
 	opts->errors = FAT_ERRORS_RO;
 	*debug = 0;
 
-	opts->utf8 = IS_ENABLED(CONFIG_FAT_DEFAULT_UTF8) && is_vfat;
-
 	if (!options)
 		goto out;
 

@@ -1319,10 +1317,14 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
 					| VFAT_SFN_CREATE_WIN95;
 			break;
 		case Opt_utf8_no:		/* 0 or no or false */
-			opts->utf8 = 0;
+			fat_reset_iocharset(opts);
 			break;
 		case Opt_utf8_yes:		/* empty or 1 or yes or true */
-			opts->utf8 = 1;
+			fat_reset_iocharset(opts);
+			iocharset = kstrdup("utf8", GFP_KERNEL);
+			if (!iocharset)
+				return -ENOMEM;
+			opts->iocharset = iocharset;
 			break;
 		case Opt_uni_xl_no:		/* 0 or no or false */
 			opts->unicode_xlate = 0;

@@ -1360,18 +1362,11 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
 	}
 
 out:
-	/* UTF-8 doesn't provide FAT semantics */
-	if (!strcmp(opts->iocharset, "utf8")) {
-		fat_msg(sb, KERN_WARNING, "utf8 is not a recommended IO charset"
-		       " for FAT filesystems, filesystem will be "
-		       "case sensitive!");
-	}
+	opts->utf8 = !strcmp(opts->iocharset, "utf8") && is_vfat;
 
 	/* If user doesn't specify allow_utime, it's initialized from dmask. */
 	if (opts->allow_utime == (unsigned short)-1)
 		opts->allow_utime = ~opts->fs_dmask & (S_IWGRP | S_IWOTH);
-	if (opts->unicode_xlate)
-		opts->utf8 = 0;
 	if (opts->nfs == FAT_NFS_NOSTALE_RO) {
 		sb->s_flags |= SB_RDONLY;
 		sb->s_export_op = &fat_export_ops_nostale;

@@ -1832,8 +1827,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 		goto out_fail;
 	}
 
-	/* FIXME: utf8 is using iocharset for upper/lower conversion */
-	if (sbi->options.isvfat) {
+	if (sbi->options.isvfat && !sbi->options.utf8) {
 		sbi->nls_io = load_nls(sbi->options.iocharset);
 		if (!sbi->nls_io) {
 			fat_msg(sb, KERN_ERR, "IO charset %s not found",

fs/fat/namei_vfat.c +19 -7

index 5369d82e0bfb..efb3cb9ea8a8 100644
@@ -134,6 +134,7 @@ static int vfat_hash(const struct dentry *dentry, struct qstr *qstr)
 static int vfat_hashi(const struct dentry *dentry, struct qstr *qstr)
 {
 	struct nls_table *t = MSDOS_SB(dentry->d_sb)->nls_io;
+	int utf8 = MSDOS_SB(dentry->d_sb)->options.utf8;
 	const unsigned char *name;
 	unsigned int len;
 	unsigned long hash;

@@ -142,8 +143,17 @@ static int vfat_hashi(const struct dentry *dentry, struct qstr *qstr)
 	len = vfat_striptail_len(qstr);
 
 	hash = init_name_hash(dentry);
-	while (len--)
-		hash = partial_name_hash(nls_tolower(t, *name++), hash);
+	if (utf8) {
+		/*
+		 * FIXME: UTF-8 doesn't provide FAT semantics
+		 * Case-insensitive support is only for 7-bit ASCII characters
+		 */
+		while (len--)
+			hash = partial_name_hash(fat_ascii_to_lower(*name++), hash);
+	} else {
+		while (len--)
+			hash = partial_name_hash(nls_tolower(t, *name++), hash);
+	}
 	qstr->hash = end_name_hash(hash);
 
 	return 0;

@@ -156,16 +166,18 @@ static int vfat_cmpi(const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	struct nls_table *t = MSDOS_SB(dentry->d_sb)->nls_io;
+	int utf8 = MSDOS_SB(dentry->d_sb)->options.utf8;
 	unsigned int alen, blen;
 
 	/* A filename cannot end in '.' or we treat it like it has none */
 	alen = vfat_striptail_len(name);
 	blen = __vfat_striptail_len(len, str);
-	if (alen == blen) {
-		if (nls_strnicmp(t, name->name, str, alen) == 0)
-			return 0;
-	}
-	return 1;
+	if (alen != blen)
+		return 1;
+	else if (utf8)
+		return fat_utf8_strnicmp(name->name, str, alen);
+	else
+		return nls_strnicmp(t, name->name, str, alen);
 }
 
 /*

Signature · 1 line

2.20.1

Other fs drivers are using iocharset= mount option for specifying charset. So add it also for hfsplus and mark old nls= mount option as deprecated.

Signed-off-by Pali Rohár <[email protected]>

---
fs/hfsplus/options.c | 7 ++++++-

1 file changed, 6 insertions(+), 1 deletion(-)

fs/hfsplus/options.c +6 -1

index 047e05c57560..a975548f6b91 100644
@@ -23,6 +23,7 @@ enum {
 	opt_creator, opt_type,
 	opt_umask, opt_uid, opt_gid,
 	opt_part, opt_session, opt_nls,
+	opt_iocharset,
 	opt_nodecompose, opt_decompose,
 	opt_barrier, opt_nobarrier,
 	opt_force, opt_err

@@ -37,6 +38,7 @@ static const match_table_t tokens = {
 	{ opt_part, "part=%u" },
 	{ opt_session, "session=%u" },
 	{ opt_nls, "nls=%s" },
+	{ opt_iocharset, "iocharset=%s" },
 	{ opt_decompose, "decompose" },
 	{ opt_nodecompose, "nodecompose" },
 	{ opt_barrier, "barrier" },

@@ -166,6 +168,9 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
 			}
 			break;
 		case opt_nls:
+			pr_warn("option nls= is deprecated, use iocharset=\n");
+			/* fallthrough */
+		case opt_iocharset:
 			if (sbi->nls) {
 				pr_err("unable to change nls mapping\n");
 				return 0;

@@ -230,7 +235,7 @@ int hfsplus_show_options(struct seq_file *seq, struct dentry *root)
 	if (sbi->session >= 0)
 		seq_printf(seq, ",session=%u", sbi->session);
 	if (sbi->nls)
-		seq_printf(seq, ",nls=%s", sbi->nls->charset);
+		seq_printf(seq, ",iocharset=%s", sbi->nls->charset);
 	if (test_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags))
 		seq_puts(seq, ",nodecompose");
 	if (test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))

Signature · 1 line

2.20.1

Other fs drivers are using iocharset= mount option for specifying charset. So add it also for hfsplus and mark old nls= mount option as deprecated.

Quoted 2 lines · Pali Rohár

Other fs drivers are using iocharset= mount option for specifying charset. So add it also for hfsplus and mark old nls= mount option as deprecated.

It would be good to also update Documentation/filesystems/hfsplus.rst.

Currently iocharset=utf8 mount option is broken. To use UTF-8 as iocharset, it is required to use utf8 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8 mount option.

If UTF-8 as iocharset is used then s_nls_map is set to NULL. So simplify code around, remove UDF_FLAG_NLS_MAP and UDF_FLAG_UTF8 flags as to distinguish between UTF-8 and non-UTF-8 it is needed just to check if s_nls_map set to NULL or not.

Signed-off-by Pali Rohár <[email protected]>

---
 fs/udf/super.c   | 50 ++++++++++++++++++------------------------------
 fs/udf/udf_sb.h  |  2 --
 fs/udf/unicode.c |  4 ++--
 3 files changed, 21 insertions(+), 35 deletions(-)

fs/udf/super.c +19 -31

index 2f83c1204e20..6e8c29107b04 100644
@@ -349,10 +349,10 @@ static int udf_show_options(struct seq_file *seq, struct dentry *root)
 		seq_printf(seq, ",lastblock=%u", sbi->s_last_block);
 	if (sbi->s_anchor != 0)
 		seq_printf(seq, ",anchor=%u", sbi->s_anchor);
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8))
-		seq_puts(seq, ",utf8");
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP) && sbi->s_nls_map)
+	if (sbi->s_nls_map)
 		seq_printf(seq, ",iocharset=%s", sbi->s_nls_map->charset);
+	else
+		seq_puts(seq, ",iocharset=utf8");
 
 	return 0;
 }

@@ -558,19 +558,24 @@ static int udf_parse_options(char *options, struct udf_options *uopt,
 			/* Ignored (never implemented properly) */
 			break;
 		case Opt_utf8:
-			uopt->flags |= (1 << UDF_FLAG_UTF8);
+			if (!remount) {
+				unload_nls(uopt->nls_map);
+				uopt->nls_map = NULL;
+			}
 			break;
 		case Opt_iocharset:
 			if (!remount) {
-				if (uopt->nls_map)
-					unload_nls(uopt->nls_map);
-				/*
-				 * load_nls() failure is handled later in
-				 * udf_fill_super() after all options are
-				 * parsed.
-				 */
+				unload_nls(uopt->nls_map);
+				uopt->nls_map = NULL;
+			}
+			/* When nls_map is not loaded then UTF-8 is used */
+			if (!remount && strcmp(args[0].from, "utf8") != 0) {
 				uopt->nls_map = load_nls(args[0].from);
-				uopt->flags |= (1 << UDF_FLAG_NLS_MAP);
+				if (!uopt->nls_map) {
+					pr_err("iocharset %s not found\n",
+						args[0].from);
+					return 0;
+				}
 			}
 			break;
 		case Opt_uforget:

@@ -2139,21 +2144,6 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	if (!udf_parse_options((char *)options, &uopt, false))
 		goto parse_options_failure;
 
-	if (uopt.flags & (1 << UDF_FLAG_UTF8) &&
-	    uopt.flags & (1 << UDF_FLAG_NLS_MAP)) {
-		udf_err(sb, "utf8 cannot be combined with iocharset\n");
-		goto parse_options_failure;
-	}
-	if ((uopt.flags & (1 << UDF_FLAG_NLS_MAP)) && !uopt.nls_map) {
-		uopt.nls_map = load_nls_default();
-		if (!uopt.nls_map)
-			uopt.flags &= ~(1 << UDF_FLAG_NLS_MAP);
-		else
-			udf_debug("Using default NLS map\n");
-	}
-	if (!(uopt.flags & (1 << UDF_FLAG_NLS_MAP)))
-		uopt.flags |= (1 << UDF_FLAG_UTF8);
-
 	fileset.logicalBlockNum = 0xFFFFFFFF;
 	fileset.partitionReferenceNum = 0xFFFF;
 

@@ -2308,8 +2298,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 error_out:
 	iput(sbi->s_vat_inode);
 parse_options_failure:
-	if (uopt.nls_map)
-		unload_nls(uopt.nls_map);
+	unload_nls(uopt.nls_map);
 	if (lvid_open)
 		udf_close_lvid(sb);
 	brelse(sbi->s_lvid_bh);

@@ -2359,8 +2348,7 @@ static void udf_put_super(struct super_block *sb)
 	sbi = UDF_SB(sb);
 
 	iput(sbi->s_vat_inode);
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP))
-		unload_nls(sbi->s_nls_map);
+	unload_nls(sbi->s_nls_map);
 	if (!sb_rdonly(sb))
 		udf_close_lvid(sb);
 	brelse(sbi->s_lvid_bh);

fs/udf/udf_sb.h +0 -2

index 758efe557a19..4fa620543d30 100644
@@ -20,8 +20,6 @@
 #define UDF_FLAG_UNDELETE		6
 #define UDF_FLAG_UNHIDE			7
 #define UDF_FLAG_VARCONV		8
-#define UDF_FLAG_NLS_MAP		9
-#define UDF_FLAG_UTF8			10
 #define UDF_FLAG_UID_FORGET     11    /* save -1 for uid to disk */
 #define UDF_FLAG_GID_FORGET     12
 #define UDF_FLAG_UID_SET	13

fs/udf/unicode.c +2 -2

index 5fcfa96463eb..622569007b53 100644
@@ -177,7 +177,7 @@ static int udf_name_from_CS0(struct super_block *sb,
 		return 0;
 	}
 
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP))
+	if (UDF_SB(sb)->s_nls_map)
 		conv_f = UDF_SB(sb)->s_nls_map->uni2char;
 	else
 		conv_f = NULL;

@@ -285,7 +285,7 @@ static int udf_name_to_CS0(struct super_block *sb,
 	if (ocu_max_len <= 0)
 		return 0;
 
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP))
+	if (UDF_SB(sb)->s_nls_map)
 		conv_f = UDF_SB(sb)->s_nls_map->char2uni;
 	else
 		conv_f = NULL;

Signature · 1 line

2.20.1

Quoted 11 lines · Pali Rohár

Currently iocharset=utf8 mount option is broken. To use UTF-8 as iocharset, it is required to use utf8 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8 mount option.

If UTF-8 as iocharset is used then s_nls_map is set to NULL. So simplify code around, remove UDF_FLAG_NLS_MAP and UDF_FLAG_UTF8 flags as to distinguish between UTF-8 and non-UTF-8 it is needed just to check if s_nls_map set to NULL or not.

Quoted 1 line

Signed-off-by Pali Rohár <[email protected]>

Thanks for the cleanup. It looks good. Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Or should I take this patch through my tree?

Honza

Quoted 138 lines

---
 fs/udf/super.c   | 50 ++++++++++++++++++------------------------------
 fs/udf/udf_sb.h  |  2 --
 fs/udf/unicode.c |  4 ++--
 3 files changed, 21 insertions(+), 35 deletions(-)

fs/udf/super.c +19 -31

index 2f83c1204e20..6e8c29107b04 100644
@@ -349,10 +349,10 @@ static int udf_show_options(struct seq_file *seq, struct dentry *root)
 		seq_printf(seq, ",lastblock=%u", sbi->s_last_block);
 	if (sbi->s_anchor != 0)
 		seq_printf(seq, ",anchor=%u", sbi->s_anchor);
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8))
-		seq_puts(seq, ",utf8");
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP) && sbi->s_nls_map)
+	if (sbi->s_nls_map)
 		seq_printf(seq, ",iocharset=%s", sbi->s_nls_map->charset);
+	else
+		seq_puts(seq, ",iocharset=utf8");
 
 	return 0;
 }

@@ -558,19 +558,24 @@ static int udf_parse_options(char *options, struct udf_options *uopt,
 			/* Ignored (never implemented properly) */
 			break;
 		case Opt_utf8:
-			uopt->flags |= (1 << UDF_FLAG_UTF8);
+			if (!remount) {
+				unload_nls(uopt->nls_map);
+				uopt->nls_map = NULL;
+			}
 			break;
 		case Opt_iocharset:
 			if (!remount) {
-				if (uopt->nls_map)
-					unload_nls(uopt->nls_map);
-				/*
-				 * load_nls() failure is handled later in
-				 * udf_fill_super() after all options are
-				 * parsed.
-				 */
+				unload_nls(uopt->nls_map);
+				uopt->nls_map = NULL;
+			}
+			/* When nls_map is not loaded then UTF-8 is used */
+			if (!remount && strcmp(args[0].from, "utf8") != 0) {
 				uopt->nls_map = load_nls(args[0].from);
-				uopt->flags |= (1 << UDF_FLAG_NLS_MAP);
+				if (!uopt->nls_map) {
+					pr_err("iocharset %s not found\n",
+						args[0].from);
+					return 0;
+				}
 			}
 			break;
 		case Opt_uforget:

@@ -2139,21 +2144,6 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	if (!udf_parse_options((char *)options, &uopt, false))
 		goto parse_options_failure;
 
-	if (uopt.flags & (1 << UDF_FLAG_UTF8) &&
-	    uopt.flags & (1 << UDF_FLAG_NLS_MAP)) {
-		udf_err(sb, "utf8 cannot be combined with iocharset\n");
-		goto parse_options_failure;
-	}
-	if ((uopt.flags & (1 << UDF_FLAG_NLS_MAP)) && !uopt.nls_map) {
-		uopt.nls_map = load_nls_default();
-		if (!uopt.nls_map)
-			uopt.flags &= ~(1 << UDF_FLAG_NLS_MAP);
-		else
-			udf_debug("Using default NLS map\n");
-	}
-	if (!(uopt.flags & (1 << UDF_FLAG_NLS_MAP)))
-		uopt.flags |= (1 << UDF_FLAG_UTF8);
-
 	fileset.logicalBlockNum = 0xFFFFFFFF;
 	fileset.partitionReferenceNum = 0xFFFF;
 

@@ -2308,8 +2298,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 error_out:
 	iput(sbi->s_vat_inode);
 parse_options_failure:
-	if (uopt.nls_map)
-		unload_nls(uopt.nls_map);
+	unload_nls(uopt.nls_map);
 	if (lvid_open)
 		udf_close_lvid(sb);
 	brelse(sbi->s_lvid_bh);

@@ -2359,8 +2348,7 @@ static void udf_put_super(struct super_block *sb)
 	sbi = UDF_SB(sb);
 
 	iput(sbi->s_vat_inode);
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP))
-		unload_nls(sbi->s_nls_map);
+	unload_nls(sbi->s_nls_map);
 	if (!sb_rdonly(sb))
 		udf_close_lvid(sb);
 	brelse(sbi->s_lvid_bh);

fs/udf/udf_sb.h +0 -2

index 758efe557a19..4fa620543d30 100644
@@ -20,8 +20,6 @@
 #define UDF_FLAG_UNDELETE		6
 #define UDF_FLAG_UNHIDE			7
 #define UDF_FLAG_VARCONV		8
-#define UDF_FLAG_NLS_MAP		9
-#define UDF_FLAG_UTF8			10
 #define UDF_FLAG_UID_FORGET     11    /* save -1 for uid to disk */
 #define UDF_FLAG_GID_FORGET     12
 #define UDF_FLAG_UID_SET	13

fs/udf/unicode.c +2 -2

index 5fcfa96463eb..622569007b53 100644
@@ -177,7 +177,7 @@ static int udf_name_from_CS0(struct super_block *sb,
 		return 0;
 	}
 
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP))
+	if (UDF_SB(sb)->s_nls_map)
 		conv_f = UDF_SB(sb)->s_nls_map->uni2char;
 	else
 		conv_f = NULL;

@@ -285,7 +285,7 @@ static int udf_name_to_CS0(struct super_block *sb,
 	if (ocu_max_len <= 0)
 		return 0;
 
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP))
+	if (UDF_SB(sb)->s_nls_map)
 		conv_f = UDF_SB(sb)->s_nls_map->char2uni;
 	else
 		conv_f = NULL;

Signature · 1 line

2.20.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 16 lines · On Thursday 12 August 2021 16:17:36 Jan Kara wrote:

Quoted 6 lines · Pali Rohár

Currently iocharset=utf8 mount option is broken. To use UTF-8 as iocharset, it is required to use utf8 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8 mount option.

Quoted 6 lines

If UTF-8 as iocharset is used then s_nls_map is set to NULL. So simplify code around, remove UDF_FLAG_NLS_MAP and UDF_FLAG_UTF8 flags as to distinguish between UTF-8 and non-UTF-8 it is needed just to check if s_nls_map set to NULL or not.

Signed-off-by Pali Rohár <[email protected]>

Thanks for the cleanup. It looks good. Feel free to add:

Quoted 3 lines

Reviewed-by Jan Kara <[email protected]>

Or should I take this patch through my tree?

Hello! Patches are just RFC, mostly untested and not ready for merging. I will wait for feedback and then I do more testing nad prepare new patch series.

Quoted 19 lines · Pali Rohár

Quoted 18 lines · On Thursday 12 August 2021 16:17:36 Jan Kara wrote:

Quoted 6 lines · Pali Rohár

Currently iocharset=utf8 mount option is broken. To use UTF-8 as iocharset, it is required to use utf8 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8 mount option.

Quoted 6 lines

If UTF-8 as iocharset is used then s_nls_map is set to NULL. So simplify code around, remove UDF_FLAG_NLS_MAP and UDF_FLAG_UTF8 flags as to distinguish between UTF-8 and non-UTF-8 it is needed just to check if s_nls_map set to NULL or not.

Signed-off-by Pali Rohár <[email protected]>

Thanks for the cleanup. It looks good. Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Quoted 5 lines

Quoted 1 line

Or should I take this patch through my tree?

Hello! Patches are just RFC, mostly untested and not ready for merging. I will wait for feedback and then I do more testing nad prepare new patch series.

OK, FWIW I've also tested the UDF and isofs patches.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 20 lines · On Friday 13 August 2021 15:48:22 Jan Kara wrote:

Quoted 19 lines · Pali Rohár

Quoted 18 lines · On Thursday 12 August 2021 16:17:36 Jan Kara wrote:

Quoted 6 lines · Pali Rohár

Currently iocharset=utf8 mount option is broken. To use UTF-8 as iocharset, it is required to use utf8 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8 mount option.

Quoted 6 lines

If UTF-8 as iocharset is used then s_nls_map is set to NULL. So simplify code around, remove UDF_FLAG_NLS_MAP and UDF_FLAG_UTF8 flags as to distinguish between UTF-8 and non-UTF-8 it is needed just to check if s_nls_map set to NULL or not.

Signed-off-by Pali Rohár <[email protected]>

Thanks for the cleanup. It looks good. Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Or should I take this patch through my tree?

Hello! Patches are just RFC, mostly untested and not ready for merging. I will wait for feedback and then I do more testing nad prepare new patch series.

OK, FWIW I've also tested the UDF and isofs patches.

Well, if you have already done tests, patches are correct and these fs driver are working fine then fell free to take it through your tree.

I just wanted to warn people that patches in this RFC are mostly untested to prevent some issues. But if somebody else was faster than me, did testing + reviewing and there was no issue, I do not see any problem with including them. Just I cannot put my own Tested-by (yet) :-)

Currently iocharset=utf8 mount option is broken. To use UTF-8 as iocharset, it is required to use utf8 mount option.

Fix iocharset=utf8 mount option to use be equivalent to the utf8 mount option.

If UTF-8 as iocharset is used then s_nls_iocharset is set to NULL. So simplify code around, remove s_utf8 field as to distinguish between UTF-8 and non-UTF-8 it is needed just to check if s_nls_iocharset is set to NULL or not.

Signed-off-by Pali Rohár <[email protected]>

---
 fs/isofs/inode.c  | 27 +++++++++++++--------------
 fs/isofs/isofs.h  |  1 -
 fs/isofs/joliet.c |  4 +---
 3 files changed, 14 insertions(+), 18 deletions(-)

fs/isofs/inode.c +13 -14

index 21edc423b79f..678e2c51b855 100644
@@ -155,7 +155,6 @@ struct iso9660_options{
 	unsigned int overriderockperm:1;
 	unsigned int uid_set:1;
 	unsigned int gid_set:1;
-	unsigned int utf8:1;
 	unsigned char map;
 	unsigned char check;
 	unsigned int blocksize;

@@ -356,7 +355,6 @@ static int parse_options(char *options, struct iso9660_options *popt)
 	popt->gid = GLOBAL_ROOT_GID;
 	popt->uid = GLOBAL_ROOT_UID;
 	popt->iocharset = NULL;
-	popt->utf8 = 0;
 	popt->overriderockperm = 0;
 	popt->session=-1;
 	popt->sbsector=-1;

@@ -389,10 +387,13 @@ static int parse_options(char *options, struct iso9660_options *popt)
 		case Opt_cruft:
 			popt->cruft = 1;
 			break;
+#ifdef CONFIG_JOLIET
 		case Opt_utf8:
-			popt->utf8 = 1;
+			kfree(popt->iocharset);
+			popt->iocharset = kstrdup("utf8", GFP_KERNEL);
+			if (!popt->iocharset)
+				return 0;
 			break;
-#ifdef CONFIG_JOLIET
 		case Opt_iocharset:
 			kfree(popt->iocharset);
 			popt->iocharset = match_strdup(&args[0]);

@@ -495,7 +496,6 @@ static int isofs_show_options(struct seq_file *m, struct dentry *root)
 	if (sbi->s_nocompress)		seq_puts(m, ",nocompress");
 	if (sbi->s_overriderockperm)	seq_puts(m, ",overriderockperm");
 	if (sbi->s_showassoc)		seq_puts(m, ",showassoc");
-	if (sbi->s_utf8)		seq_puts(m, ",utf8");
 
 	if (sbi->s_check)		seq_printf(m, ",check=%c", sbi->s_check);
 	if (sbi->s_mapping)		seq_printf(m, ",map=%c", sbi->s_mapping);

@@ -518,9 +518,10 @@ static int isofs_show_options(struct seq_file *m, struct dentry *root)
 		seq_printf(m, ",fmode=%o", sbi->s_fmode);
 
 #ifdef CONFIG_JOLIET
-	if (sbi->s_nls_iocharset &&
-	    strcmp(sbi->s_nls_iocharset->charset, CONFIG_NLS_DEFAULT) != 0)
+	if (sbi->s_nls_iocharset)
 		seq_printf(m, ",iocharset=%s", sbi->s_nls_iocharset->charset);
+	else
+		seq_puts(m, ",iocharset=utf8");
 #endif
 	return 0;
 }

@@ -863,14 +864,13 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 	sbi->s_nls_iocharset = NULL;
 
 #ifdef CONFIG_JOLIET
-	if (joliet_level && opt.utf8 == 0) {
+	if (joliet_level) {
 		char *p = opt.iocharset ? opt.iocharset : CONFIG_NLS_DEFAULT;
-		sbi->s_nls_iocharset = load_nls(p);
-		if (! sbi->s_nls_iocharset) {
-			/* Fail only if explicit charset specified */
-			if (opt.iocharset)
+		if (strcmp(p, "utf8") != 0) {
+			sbi->s_nls_iocharset = opt.iocharset ?
+				load_nls(opt.iocharset) : load_nls_default();
+			if (!sbi->s_nls_iocharset)
 				goto out_freesbi;
-			sbi->s_nls_iocharset = load_nls_default();
 		}
 	}
 #endif

@@ -886,7 +886,6 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 	sbi->s_gid = opt.gid;
 	sbi->s_uid_set = opt.uid_set;
 	sbi->s_gid_set = opt.gid_set;
-	sbi->s_utf8 = opt.utf8;
 	sbi->s_nocompress = opt.nocompress;
 	sbi->s_overriderockperm = opt.overriderockperm;
 	/*

fs/isofs/isofs.h +0 -1

index 055ec6c586f7..dcdc191ed183 100644
@@ -44,7 +44,6 @@ struct isofs_sb_info {
 	unsigned char s_session;
 	unsigned int  s_high_sierra:1;
 	unsigned int  s_rock:2;
-	unsigned int  s_utf8:1;
 	unsigned int  s_cruft:1; /* Broken disks with high byte of length
 				  * containing junk */
 	unsigned int  s_nocompress:1;

fs/isofs/joliet.c +1 -3

index be8b6a9d0b92..c0f04a1e7f69 100644
@@ -41,14 +41,12 @@ uni16_to_x8(unsigned char *ascii, __be16 *uni, int len, struct nls_table *nls)
 int
 get_joliet_filename(struct iso_directory_record * de, unsigned char *outname, struct inode * inode)
 {
-	unsigned char utf8;
 	struct nls_table *nls;
 	unsigned char len = 0;
 
-	utf8 = ISOFS_SB(inode->i_sb)->s_utf8;
 	nls = ISOFS_SB(inode->i_sb)->s_nls_iocharset;
 
-	if (utf8) {
+	if (!nls) {
 		len = utf16s_to_utf8s((const wchar_t *) de->name,
 				de->name_len[0] >> 1, UTF16_BIG_ENDIAN,
 				outname, PAGE_SIZE);

Signature · 1 line

2.20.1

Other fs drivers are using iocharset= mount option for specifying charset. So mark iocharset= mount option as preferred and deprecate nls= mount option.

Other fs drivers are using iocharset= mount option for specifying charset. So mark iocharset= mount option as preferred and deprecate nls= mount option.

Quoted 3 lines · Pali Rohár

Other fs drivers are using iocharset= mount option for specifying charset. So mark iocharset= mount option as preferred and deprecate nls= mount option.

One idea is also make this change to fs/fc_parser.c and then when we want we can drop support from all filesystem same time. This way we can get more deprecated code off the fs drivers. Draw back is that then every filesstem has this deprecated nls= option if it support iocharsets option. But that should imo be ok.

Quoted 10 lines · On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:

Quoted 3 lines · Pali Rohár

Other fs drivers are using iocharset= mount option for specifying charset. So mark iocharset= mount option as preferred and deprecate nls= mount option.

One idea is also make this change to fs/fc_parser.c and then when we want we can drop support from all filesystem same time. This way we can get more deprecated code off the fs drivers. Draw back is that then every filesstem has this deprecated nls= option if it support iocharsets option. But that should imo be ok.

Beware that iocharset= is required only for fs which store filenames in some specific encoding (in this case extension to UTF-16). For fs which store filenames in raw bytes this option should not be parsed at all.

Therefore I'm not sure if this parsing should be in global fs/fc_parser.c file...

Quoted 12 lines · Pali Rohár

Quoted 10 lines · On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:

Quoted 3 lines · Pali Rohár

Other fs drivers are using iocharset= mount option for specifying charset. So mark iocharset= mount option as preferred and deprecate nls= mount option.

One idea is also make this change to fs/fc_parser.c and then when we want we can drop support from all filesystem same time. This way we can get more deprecated code off the fs drivers. Draw back is that then every filesstem has this deprecated nls= option if it support iocharsets option. But that should imo be ok.

Quoted 3 lines

Beware that iocharset= is required only for fs which store filenames in some specific encoding (in this case extension to UTF-16). For fs which store filenames in raw bytes this option should not be parsed at all.

Yeah of course. I was thinking that what we do is that if key is nls= we change key to iocharset, print deprecated and then send it to driver parser as usual. This way driver parser will never know that user specifie nls= because it just get iocharset. But this is probebly too fancy way to think simple problem. Just idea.

Quoted 2 lines

Therefore I'm not sure if this parsing should be in global fs/fc_parser.c file...

Quoted 17 lines · On Thursday 19 August 2021 13:23:42 Kari Argillander wrote:

Quoted 12 lines · Pali Rohár

Quoted 10 lines · On Thursday 19 August 2021 04:21:08 Kari Argillander wrote:

Quoted 3 lines · Pali Rohár

Other fs drivers are using iocharset= mount option for specifying charset. So mark iocharset= mount option as preferred and deprecate nls= mount option.

One idea is also make this change to fs/fc_parser.c and then when we want we can drop support from all filesystem same time. This way we can get more deprecated code off the fs drivers. Draw back is that then every filesstem has this deprecated nls= option if it support iocharsets option. But that should imo be ok.

Quoted 3 lines

Beware that iocharset= is required only for fs which store filenames in some specific encoding (in this case extension to UTF-16). For fs which store filenames in raw bytes this option should not be parsed at all.

Quoted 5 lines

Yeah of course. I was thinking that what we do is that if key is nls= we change key to iocharset, print deprecated and then send it to driver parser as usual. This way driver parser will never know that user specifie nls= because it just get iocharset. But this is probebly too fancy way to think simple problem. Just idea.

This has an issue that when you use nls= option for e.g. ext4 fs then kernel starts reporting that nls= for ext4 is deprecated. But there is no nls= option and neither iocharset= option for ext4. So kernel should not start reporting such warnings for ext4.

Quoted 4 lines

Quoted 3 lines

Therefore I'm not sure if this parsing should be in global fs/fc_parser.c file...

It does not make any sense to set hsb->nls_io (NLS iocharset used between VFS and hfs driver) when hsb->nls_disk (NLS codepage used between hfs driver and disk) is not set.

Reverse engineering driver code shown what is doing in this special case:

When codepage was not defined but iocharset was then hfs driver copied 8bit character from disk directly to 16bit unicode wchar_t type. Which means it did conversion from Latin1 (ISO-8859-1) to Unicode because first 256 Unicode code points matches 8bit ISO-8859-1 codepage table. So when iocharset was specified and codepage not, then codepage used implicit value "iso8859-1".

So when hsb->nls_disk is not set and hsb->nls_io is then explicitly set hsb->nls_disk to "iso8859-1".

Such setup is obviously incompatible with Mac OS systems as they do not support iso8859-1 encoding for hfs. So print warning into dmesg about this fact.

After this change hsb->nls_disk is always set, so remove code paths for case when hsb->nls_disk was not set as they are not needed anymore.

Signed-off-by Pali Rohár <[email protected]>

---
fs/hfs/super.c | 31 +++++++++++++++++++++++++++++++ fs/hfs/trans.c | 38 ++++++++++++++------------------------

2 files changed, 45 insertions(+), 24 deletions(-)

fs/hfs/super.c +31 -0

index 12d9bae39363..86bc46746c7f 100644
@@ -351,6 +351,37 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 		}
 	}
 
+	if (hsb->nls_io && !hsb->nls_disk) {
+		/*
+		 * Previous version of hfs driver did something unexpected:
+		 * When codepage was not defined but iocharset was then
+		 * hfs driver copied 8bit character from disk directly to
+		 * 16bit unicode wchar_t type. Which means it did conversion
+		 * from Latin1 (ISO-8859-1) to Unicode because first 256
+		 * Unicode code points matches 8bit ISO-8859-1 codepage table.
+		 * So when iocharset was specified and codepage not, then
+		 * codepage used implicit value "iso8859-1".
+		 *
+		 * To not change this previous default behavior as some users
+		 * may depend on it, we load iso8859-1 NLS table explicitly
+		 * to simplify code and make it more reable what happens.
+		 *
+		 * In context of hfs driver it is really strange to use
+		 * ISO-8859-1 codepage table for storing data to disk, but
+		 * nothing forbids it. Just it is highly incompatible with
+		 * Mac OS systems. So via pr_warn() inform user that this
+		 * is not probably what he wants.
+		 */
+		pr_warn("iocharset was specified but codepage not, "
+			"using default codepage=iso8859-1\n");
+		pr_warn("this default codepage=iso8859-1 is incompatible with "
+			"Mac OS systems and may be changed in the future");
+		hsb->nls_disk = load_nls("iso8859-1");
+		if (!hsb->nls_disk) {
+			pr_err("unable to load iso8859-1 codepage\n");
+			return 0;
+		}
+	}
 	if (hsb->nls_disk && !hsb->nls_io) {
 		hsb->nls_io = load_nls_default();
 		if (!hsb->nls_io) {

fs/hfs/trans.c +14 -24

index 39f5e343bf4d..c75682c61b06 100644
@@ -48,18 +48,13 @@ int hfs_mac2asc(struct super_block *sb, char *out, const struct hfs_name *in)
 		wchar_t ch;
 
 		while (srclen > 0) {
-			if (nls_disk) {
-				size = nls_disk->char2uni(src, srclen, &ch);
-				if (size <= 0) {
-					ch = '?';
-					size = 1;
-				}
-				src += size;
-				srclen -= size;
-			} else {
-				ch = *src++;
-				srclen--;
+			size = nls_disk->char2uni(src, srclen, &ch);
+			if (size <= 0) {
+				ch = '?';
+				size = 1;
 			}
+			src += size;
+			srclen -= size;
 			if (ch == '/')
 				ch = ':';
 			size = nls_io->uni2char(ch, dst, dstlen);

@@ -119,20 +114,15 @@ void hfs_asc2mac(struct super_block *sb, struct hfs_name *out, const struct qstr
 			srclen -= size;
 			if (ch == ':')
 				ch = '/';
-			if (nls_disk) {
-				size = nls_disk->uni2char(ch, dst, dstlen);
-				if (size < 0) {
-					if (size == -ENAMETOOLONG)
-						goto out;
-					*dst = '?';
-					size = 1;
-				}
-				dst += size;
-				dstlen -= size;
-			} else {
-				*dst++ = ch > 0xff ? '?' : ch;
-				dstlen--;
+			size = nls_disk->uni2char(ch, dst, dstlen);
+			if (size < 0) {
+				if (size == -ENAMETOOLONG)
+					goto out;
+				*dst = '?';
+				size = 1;
 			}
+			dst += size;
+			dstlen -= size;
 		}
 	} else {
 		char ch;

Signature · 1 line

2.20.1

Quoted 23 lines · Pali Rohár

It does not make any sense to set hsb->nls_io (NLS iocharset used between VFS and hfs driver) when hsb->nls_disk (NLS codepage used between hfs driver and disk) is not set.

Reverse engineering driver code shown what is doing in this special case:

When codepage was not defined but iocharset was then hfs driver copied 8bit character from disk directly to 16bit unicode wchar_t type. Which means it did conversion from Latin1 (ISO-8859-1) to Unicode because first 256 Unicode code points matches 8bit ISO-8859-1 codepage table. So when iocharset was specified and codepage not, then codepage used implicit value "iso8859-1".

So when hsb->nls_disk is not set and hsb->nls_io is then explicitly set hsb->nls_disk to "iso8859-1".

Such setup is obviously incompatible with Mac OS systems as they do not support iso8859-1 encoding for hfs. So print warning into dmesg about this fact.

Quoted 2 lines

After this change hsb->nls_disk is always set, so remove code paths for case when hsb->nls_disk was not set as they are not needed anymore.

Sounds reasonable. But it will be great to know that the change has been tested reasonably well.

Quoted 27 lines · Viacheslav Dubeyko

Quoted 25 lines · Pali Rohár

It does not make any sense to set hsb->nls_io (NLS iocharset used between VFS and hfs driver) when hsb->nls_disk (NLS codepage used between hfs driver and disk) is not set.

Reverse engineering driver code shown what is doing in this special case:

When codepage was not defined but iocharset was then hfs driver copied 8bit character from disk directly to 16bit unicode wchar_t type. Which means it did conversion from Latin1 (ISO-8859-1) to Unicode because first 256 Unicode code points matches 8bit ISO-8859-1 codepage table. So when iocharset was specified and codepage not, then codepage used implicit value "iso8859-1".

So when hsb->nls_disk is not set and hsb->nls_io is then explicitly set hsb->nls_disk to "iso8859-1".

Such setup is obviously incompatible with Mac OS systems as they do not support iso8859-1 encoding for hfs. So print warning into dmesg about this fact.

After this change hsb->nls_disk is always set, so remove code paths for case when hsb->nls_disk was not set as they are not needed anymore.

Quoted 1 line

Sounds reasonable. But it will be great to know that the change has been tested reasonably well.

I don't think it's reasonable to ask Pali to test every single filesystem. That's something the maintainer should do, as you're more likely to have the infrastructure already set up to do testing of your filesystem and be aware of fun corner cases and use cases than someone who's working across all filesystems.

Quoted 28 lines · On Monday 09 August 2021 18:37:19 Matthew Wilcox wrote:

Quoted 27 lines · Viacheslav Dubeyko

Quoted 25 lines · Pali Rohár

It does not make any sense to set hsb->nls_io (NLS iocharset used between VFS and hfs driver) when hsb->nls_disk (NLS codepage used between hfs driver and disk) is not set.

Reverse engineering driver code shown what is doing in this special case:

When codepage was not defined but iocharset was then hfs driver copied 8bit character from disk directly to 16bit unicode wchar_t type. Which means it did conversion from Latin1 (ISO-8859-1) to Unicode because first 256 Unicode code points matches 8bit ISO-8859-1 codepage table. So when iocharset was specified and codepage not, then codepage used implicit value "iso8859-1".

So when hsb->nls_disk is not set and hsb->nls_io is then explicitly set hsb->nls_disk to "iso8859-1".

Such setup is obviously incompatible with Mac OS systems as they do not support iso8859-1 encoding for hfs. So print warning into dmesg about this fact.

After this change hsb->nls_disk is always set, so remove code paths for case when hsb->nls_disk was not set as they are not needed anymore.

Quoted 7 lines

Quoted 1 line

Sounds reasonable. But it will be great to know that the change has been tested reasonably well.

I don't think it's reasonable to ask Pali to test every single filesystem. That's something the maintainer should do, as you're more likely to have the infrastructure already set up to do testing of your filesystem and be aware of fun corner cases and use cases than someone who's working across all filesystems.

This patch series is currently in RFC form, as stated in cover letter mostly untested. So they are not in form for merging or detailed reviewing. I just would like to know if this is the right direction with filesystems and if I should continue with this my effort or not. And I thought that sending RFC "incomplete" patches is better way than just describing what to do and how...

Quoted 27 lines · Viacheslav Dubeyko

Quoted 25 lines · Pali Rohár

It does not make any sense to set hsb->nls_io (NLS iocharset used between VFS and hfs driver) when hsb->nls_disk (NLS codepage used between hfs driver and disk) is not set.

Reverse engineering driver code shown what is doing in this special case:

When codepage was not defined but iocharset was then hfs driver copied 8bit character from disk directly to 16bit unicode wchar_t type. Which means it did conversion from Latin1 (ISO-8859-1) to Unicode because first 256 Unicode code points matches 8bit ISO-8859-1 codepage table. So when iocharset was specified and codepage not, then codepage used implicit value "iso8859-1".

So when hsb->nls_disk is not set and hsb->nls_io is then explicitly set hsb->nls_disk to "iso8859-1".

Such setup is obviously incompatible with Mac OS systems as they do not support iso8859-1 encoding for hfs. So print warning into dmesg about this fact.

After this change hsb->nls_disk is always set, so remove code paths for case when hsb->nls_disk was not set as they are not needed anymore.

Quoted 1 line

Sounds reasonable. But it will be great to know that the change has been tested reasonably well.

I don't think it's reasonable to ask Pali to test every single filesystem. That's something the maintainer should do, as you're more likely to have the infrastructure already set up to do testing of your filesystem and be aware of fun corner cases and use cases than someone who's working across all filesystems.

NLS table for utf8 is broken and cannot be fixed.

So instead of broken utf8 nls functions char2uni() and uni2char() use functions utf8_to_utf32() and utf32_to_utf8() which implements correct encoding and decoding between Unicode code points and UTF-8 sequence.

When iochatset=utf8 is used then set hsb->nls_io to NULL and use it for distinguish between the fact if NLS table or native UTF-8 functions should be used.

Signed-off-by Pali Rohár <[email protected]>

---
fs/hfs/super.c | 33 ++++++++++++++++++++++----------- fs/hfs/trans.c | 24 ++++++++++++++++++++----

2 files changed, 42 insertions(+), 15 deletions(-)

fs/hfs/super.c +0 -0

index 86bc46746c7f..076308df41cf 100644

fs/hfs/super.c +5 -2

-	if (sbi->nls_io)
-		seq_printf(seq, ",iocharset=%s", sbi->nls_io->charset);
+		if (sbi->nls_io)
+			seq_printf(seq, ",iocharset=%s", sbi->nls_io->charset);
+		else
+			seq_puts(seq, ",iocharset=utf8");
+	}
 	if (sbi->s_quiet)
 		seq_printf(seq, ",quiet");
 	return 0;

fs/hfs/super.c +3 -6

+
 	while ((p = strsep(&options, ",")) != NULL) {
 		if (!*p)
 			continue;

@@ -332,18 +338,22 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 			kfree(p);
 			break;
 		case opt_iocharset:
-			if (hsb->nls_io) {
+			if (have_iocharset) {
 				pr_err("unable to change iocharset\n");
 				return 0;
 			}
 			p = match_strdup(&args[0]);
-			if (p)
-				hsb->nls_io = load_nls(p);
-			if (!hsb->nls_io) {
-				pr_err("unable to load iocharset \"%s\"\n", p);
-				kfree(p);
+			if (!p)

fs/hfs/super.c +1 -1

 			kfree(p);
 			break;
 		default:

@@ -351,7 +361,7 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 		}
 	}
 
-	if (hsb->nls_io && !hsb->nls_disk) {
+	if (have_iocharset && !hsb->nls_disk) {
 		/*
 		 * Previous version of hfs driver did something unexpected:
 		 * When codepage was not defined but iocharset was then

fs/hfs/super.c +0 -0

 		hsb->nls_io = load_nls_default();
 		if (!hsb->nls_io) {
 			pr_err("unable to load default iocharset\n");

fs/hfs/trans.c +0 -0

index c75682c61b06..bff8e54003ab 100644

fs/hfs/trans.c +19 -3

 		while (srclen > 0) {

@@ -57,7 +57,12 @@ int hfs_mac2asc(struct super_block *sb, char *out, const struct hfs_name *in)
 			srclen -= size;
 			if (ch == '/')
 				ch = ':';
-			size = nls_io->uni2char(ch, dst, dstlen);
+			if (nls_io)
+				size = nls_io->uni2char(ch, dst, dstlen);
+			else if (dstlen > 0)
+				size = utf32_to_utf8(ch, dst, dstlen);
+			else
+				size = -ENAMETOOLONG;
 			if (size < 0) {
 				if (size == -ENAMETOOLONG)
 					goto out;

@@ -101,11 +106,22 @@ void hfs_asc2mac(struct super_block *sb, struct hfs_name *out, const struct qstr
 	srclen = in->len;
 	dst = out->name;
 	dstlen = HFS_NAMELEN;
-	if (nls_io) {
+	if (nls_disk) {
 		wchar_t ch;
+		unicode_t u;
 
 		while (srclen > 0) {
-			size = nls_io->char2uni(src, srclen, &ch);
+			if (nls_io)
+				size = nls_io->char2uni(src, srclen, &ch);
+			else {
+				size = utf8_to_utf32(str, strlen, &u);
+				if (size >= 0) {
+					if (u <= MAX_WCHAR_T)
+						ch = u;
+					else
+						size = -EINVAL;
+				}
+			}
 			if (size < 0) {
 				ch = '?';
 				size = 1;

Signature · 1 line

2.20.1

Quoted 18 lines · Pali Rohár

NLS table for utf8 is broken and cannot be fixed.

So instead of broken utf8 nls functions char2uni() and uni2char() use functions utf8_to_utf32() and utf32_to_utf8() which implements correct encoding and decoding between Unicode code points and UTF-8 sequence.

When iochatset=utf8 is used then set hsb->nls_io to NULL and use it for distinguish between the fact if NLS table or native UTF-8 functions should be used.

Signed-off-by Pali Rohár <[email protected]>

---
fs/hfs/super.c | 33 ++++++++++++++++++++++----------- fs/hfs/trans.c | 24 ++++++++++++++++++++----

2 files changed, 42 insertions(+), 15 deletions(-)

fs/hfs/super.c static int hfs_show_options(struct seq_file *seq, struct dentry *root) · line 153

Quoted 11 lines

fs/hfs/super.c +1 -1

index 86bc46746c7f..076308df41cf 100644
@@ -149,10 +149,13 @@ static int hfs_show_options(struct seq_file *seq, struct dentry *root)
 		seq_printf(seq, ",part=%u", sbi->part);
 	if (sbi->session >= 0)
 		seq_printf(seq, ",session=%u", sbi->session);
-	if (sbi->nls_disk)
+	if (sbi->nls_disk) {
 		seq_printf(seq, ",codepage=%s", sbi->nls_disk->charset);

Maybe, I am missing something. But where is the closing “}”?

fs/hfs/super.c static int hfs_show_options(struct seq_file *seq, struct dentry *root) · line 158

Quoted 8 lines

fs/hfs/super.c +5 -2

-	if (sbi->nls_io)
-		seq_printf(seq, ",iocharset=%s", sbi->nls_io->charset);
+		if (sbi->nls_io)
+			seq_printf(seq, ",iocharset=%s", sbi->nls_io->charset);
+		else
+			seq_puts(seq, ",iocharset=utf8");
+	}

fs/hfs/super.c static int parse_options(char *options, struct hfs_sb_info *hsb) · line 231

Quoted 8 lines

fs/hfs/super.c +1 -0

 	if (sbi->s_quiet)
 		seq_printf(seq, ",quiet");
 	return 0;

@@ -225,6 +228,7 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 	char *p;
 	substring_t args[MAX_OPT_ARGS];
 	int tmp, token;
+	int have_iocharset;

What’s about boolean type?

fs/hfs/super.c static int parse_options(char *options, struct hfs_sb_info *hsb) · line 246

Quoted 8 lines

fs/hfs/super.c +1 -0

 	/* initialize the sb with defaults */
 	hsb->s_uid = current_uid();

@@ -239,6 +243,8 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 	if (!options)
 		return 1;
 
+	have_iocharset = 0;

What’s about false here?

Quoted 20 lines

fs/hfs/super.c +3 -6

+
 	while ((p = strsep(&options, ",")) != NULL) {
 		if (!*p)
 			continue;

@@ -332,18 +338,22 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 			kfree(p);
 			break;
 		case opt_iocharset:
-			if (hsb->nls_io) {
+			if (have_iocharset) {
 				pr_err("unable to change iocharset\n");
 				return 0;
 			}
 			p = match_strdup(&args[0]);
-			if (p)
-				hsb->nls_io = load_nls(p);
-			if (!hsb->nls_io) {
-				pr_err("unable to load iocharset \"%s\"\n", p);
-				kfree(p);
+			if (!p)

fs/hfs/super.c static int parse_options(char *options, struct hfs_sb_info *hsb) · line 356

Quoted 10 lines

fs/hfs/super.c +8 -0

 				return 0;
+			if (strcmp(p, "utf8") != 0) {
+				hsb->nls_io = load_nls(p);
+				if (!hsb->nls_io) {
+					pr_err("unable to load iocharset \"%s\"\n", p);
+					kfree(p);
+					return 0;
+				}
 			}
+			have_iocharset = 1;

What’s about true here?

Quoted 7 lines

fs/hfs/super.c +0 -0

 			kfree(p);
 			break;
 		default:

@@ -351,7 +361,7 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 		}
 	}
 

fs/hfs/super.c static int parse_options(char *options, struct hfs_sb_info *hsb) · line 396

Quoted 12 lines

fs/hfs/super.c +3 -2

-	if (hsb->nls_io && !hsb->nls_disk) {
+	if (have_iocharset && !hsb->nls_disk) {
 		/*
 		 * Previous version of hfs driver did something unexpected:
 		 * When codepage was not defined but iocharset was then

@@ -382,7 +392,8 @@ static int parse_options(char *options, struct hfs_sb_info *hsb)
 			return 0;
 		}
 	}
-	if (hsb->nls_disk && !hsb->nls_io) {
+	if (hsb->nls_disk &&
+	    !have_iocharset && strcmp(CONFIG_NLS_DEFAULT, "utf8") != 0) {

Maybe, introduce the variable to calculate the boolean value here? Then if statement will look much cleaner.

Quoted 9 lines

fs/hfs/super.c +0 -0

 		hsb->nls_io = load_nls_default();
 		if (!hsb->nls_io) {
 			pr_err("unable to load default iocharset\n");

fs/hfs/trans.c +0 -0

index c75682c61b06..bff8e54003ab 100644
@@ -44,7 +44,7 @@ int hfs_mac2asc(struct super_block *sb, char *out, const struct hfs_name *in)
 		srclen = HFS_NAMELEN;

fs/hfs/trans.c int hfs_mac2asc(struct super_block *sb, char *out, const struct hfs_name *in) · line 49

Quoted 6 lines

fs/hfs/trans.c +1 -1

 	dst = out;
 	dstlen = HFS_MAX_NAMELEN;
-	if (nls_io) {
+	if (nls_disk) {
 		wchar_t ch;
 

I could miss something here. But what’s about the closing “}”?

Thanks,
Slava.

Quoted 43 lines

fs/hfs/trans.c +19 -3

 		while (srclen > 0) {

@@ -57,7 +57,12 @@ int hfs_mac2asc(struct super_block *sb, char *out, const struct hfs_name *in)
 			srclen -= size;
 			if (ch == '/')
 				ch = ':';
-			size = nls_io->uni2char(ch, dst, dstlen);
+			if (nls_io)
+				size = nls_io->uni2char(ch, dst, dstlen);
+			else if (dstlen > 0)
+				size = utf32_to_utf8(ch, dst, dstlen);
+			else
+				size = -ENAMETOOLONG;
 			if (size < 0) {
 				if (size == -ENAMETOOLONG)
 					goto out;

@@ -101,11 +106,22 @@ void hfs_asc2mac(struct super_block *sb, struct hfs_name *out, const struct qstr
 	srclen = in->len;
 	dst = out->name;
 	dstlen = HFS_NAMELEN;
-	if (nls_io) {
+	if (nls_disk) {
 		wchar_t ch;
+		unicode_t u;
 
 		while (srclen > 0) {
-			size = nls_io->char2uni(src, srclen, &ch);
+			if (nls_io)
+				size = nls_io->char2uni(src, srclen, &ch);
+			else {
+				size = utf8_to_utf32(str, strlen, &u);
+				if (size >= 0) {
+					if (u <= MAX_WCHAR_T)
+						ch = u;
+					else
+						size = -EINVAL;
+				}
+			}
 			if (size < 0) {
 				ch = '?';
 				size = 1;

Signature · 1 line

2.20.1

NLS table for utf8 is broken and cannot be fixed.

So instead of broken utf8 nls functions char2uni() and uni2char() use functions utf8_to_utf32() and utf32_to_utf8() which implements correct encoding and decoding between Unicode code points and UTF-8 sequence.

Note that this fs driver does not support full Unicode range, specially UTF-16 surrogate pairs are unsupported. This patch does not change this limitation and support for UTF-16 surrogate pairs stay unimplemented.

When iochatset=utf8 is used then set sbi->nls to NULL and use it for distinguish between the fact if NLS table or native UTF-8 functions should be used.

Signed-off-by Pali Rohár <[email protected]>

---

fs/hfsplus/dir.c            |  6 ++++--
fs/hfsplus/options.c        | 32 ++++++++++++++++++--------------
fs/hfsplus/super.c          |  7 +------
fs/hfsplus/unicode.c        | 31 ++++++++++++++++++++++++++++---
fs/hfsplus/xattr.c          | 14 +++++++++-----
fs/hfsplus/xattr_security.c |  3 ++-
6 files changed, 62 insertions(+), 31 deletions(-)

fs/hfsplus/dir.c +0 -0

index 84714bbccc12..2caf0cd82221 100644

fs/hfsplus/dir.c +2 -1

 	if (!strbuf) {
 		err = -ENOMEM;
 		goto out;

@@ -203,7 +204,8 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
 			fd.entrylength);
 		type = be16_to_cpu(entry.type);
-		len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN;
+		len = (HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) *
+		      HFSPLUS_MAX_STRLEN;
 		err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
 		if (err)
 			goto out;

fs/hfsplus/options.c +0 -0

index a975548f6b91..16c08cb5c4f8 100644

fs/hfsplus/options.c +3 -7

 	if (!input)
 		goto done;

@@ -171,20 +174,24 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
 			pr_warn("option nls= is deprecated, use iocharset=\n");
 			/* fallthrough */
 		case opt_iocharset:
-			if (sbi->nls) {
+			if (have_iocharset) {
 				pr_err("unable to change nls mapping\n");
 				return 0;
 			}
 			p = match_strdup(&args[0]);
-			if (p)
-				sbi->nls = load_nls(p);
-			if (!sbi->nls) {
-				pr_err("unable to load nls mapping \"%s\"\n",
-				       p);
-				kfree(p);
+			if (!p)
 				return 0;
+			if (strcmp(p, "utf8") != 0) {

fs/hfsplus/options.c +0 -0

 			break;
 		case opt_decompose:
 			clear_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags);

@@ -207,13 +214,10 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
 	}
 

done:

fs/hfsplus/options.c +4 -7

-	if (!sbi->nls) {
-		/* try utf8 first, as this is the old default behaviour */
-		sbi->nls = load_nls("utf8");
-		if (!sbi->nls)
-			sbi->nls = load_nls_default();
-		if (!sbi->nls)
-			return 0;
+	if (!have_iocharset) {
+		/* use utf8, as this is the old default behaviour */
+		pr_debug("using native UTF-8 without nls\n");
+		/* no sbi->nls means that native UTF-8 code is used */

	}

	return 1;

fs/hfsplus/super.c +1 -6

index b9e3db3f855f..985662451bfc 100644
@@ -403,11 +403,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* temporarily use utf8 to correctly find the hidden dir below */
 	nls = sbi->nls;
-	sbi->nls = load_nls("utf8");
-	if (!sbi->nls) {
-		pr_err("unable to load nls for utf8\n");
-		goto out_unload_nls;
-	}
+	sbi->nls = NULL;
 
 	/* Grab the volume header */
 	if (hfsplus_read_wrapper(sb)) {

@@ -585,7 +581,6 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 		}
 	}
 
-	unload_nls(sbi->nls);
 	sbi->nls = nls;
 	return 0;
 

fs/hfsplus/unicode.c +6 -1

index 73342c925a4b..1d8c31c5126f 100644
@@ -190,7 +190,12 @@ int hfsplus_uni2asc(struct super_block *sb,
 				c0 = ':';
 				break;
 			}
-			res = nls->uni2char(c0, op, len);
+			if (nls)
+				res = nls->uni2char(c0, op, len);
+			else (len > 0)
+				res = utf32_to_utf8(c0, op, len);
+			else
+				res = -ENAMETOOLONG;
 			if (res < 0) {
 				if (res == -ENAMETOOLONG)
 					goto out;

@@ -233,7 +238,12 @@ int hfsplus_uni2asc(struct super_block *sb,
 			cc = c0;
 		}

done:

fs/hfsplus/unicode.c +6 -1

-		res = nls->uni2char(cc, op, len);
+		if (nls)
+			res = nls->uni2char(cc, op, len);
+		else (len > 0)
+			res = utf32_to_utf8(cc, op, len);
+		else
+			res = -ENAMETOOLONG;

		if (res < 0) {
			if (res == -ENAMETOOLONG)
				goto out;

fs/hfsplus/unicode.c +0 -1

@@ -256,7 +266,22 @@ int hfsplus_uni2asc(struct super_block *sb,
 static inline int asc2unichar(struct super_block *sb, const char *astr, int len,
 			      wchar_t *uc)
 {
-	int size = HFSPLUS_SB(sb)->nls->char2uni(astr, len, uc);

fs/hfsplus/unicode.c +6 -0

+			if (u <= MAX_WCHAR_T)
+				*uc = u;
+			else
+				size = -EINVAL;
+		}
+	}
 	if (size <= 0) {
 		*uc = '?';
 		size = 1;

fs/hfsplus/xattr.c +0 -0

index e2855ceefd39..9b2653f08a5f 100644

fs/hfsplus/xattr.c +2 -1

 	if (!strbuf) {
 		res = -ENOMEM;
 		goto out;

@@ -732,7 +735,8 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		if (be32_to_cpu(attr_key.cnid) != inode->i_ino)
 			goto end_listxattr;
 
-		xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN;
+		xattr_name_len = (HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4)
+				* HFSPLUS_ATTR_MAX_STRLEN;
 		if (hfsplus_uni2asc(inode->i_sb,
 			(const struct hfsplus_unistr *)&fd.key->attr.key_name,
 					strbuf, &xattr_name_len)) {

fs/hfsplus/xattr_security.c +0 -0

index c1c7a16cbf21..438ebcd1359b 100644

fs/hfsplus/xattr_security.c +0 -0

 	if (!xattr_name)
 		return -ENOMEM;

Signature · 1 line

2.20.1

Quoted 26 lines · Pali Rohár

NLS table for utf8 is broken and cannot be fixed.

So instead of broken utf8 nls functions char2uni() and uni2char() use functions utf8_to_utf32() and utf32_to_utf8() which implements correct encoding and decoding between Unicode code points and UTF-8 sequence.

Note that this fs driver does not support full Unicode range, specially UTF-16 surrogate pairs are unsupported. This patch does not change this limitation and support for UTF-16 surrogate pairs stay unimplemented.

When iochatset=utf8 is used then set sbi->nls to NULL and use it for distinguish between the fact if NLS table or native UTF-8 functions should be used.

Signed-off-by Pali Rohár <[email protected]>

---

fs/hfsplus/dir.c            |  6 ++++--
fs/hfsplus/options.c        | 32 ++++++++++++++++++--------------
fs/hfsplus/super.c          |  7 +------
fs/hfsplus/unicode.c        | 31 ++++++++++++++++++++++++++++---
fs/hfsplus/xattr.c          | 14 +++++++++-----
fs/hfsplus/xattr_security.c |  3 ++-
6 files changed, 62 insertions(+), 31 deletions(-)

fs/hfsplus/dir.c static int hfsplus_readdir(struct file *file, struct dir_context *ctx) · line 148

Quoted 11 lines

fs/hfsplus/dir.c +2 -1

index 84714bbccc12..2caf0cd82221 100644
@@ -144,7 +144,8 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
 	if (err)
 		return err;
-	strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);
+	strbuf = kmalloc((HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) *
+			HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);

Maybe, introduce some variable that will contain the length calculation?

Quoted 18 lines

fs/hfsplus/dir.c +2 -1

 	if (!strbuf) {
 		err = -ENOMEM;
 		goto out;

@@ -203,7 +204,8 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
 			fd.entrylength);
 		type = be16_to_cpu(entry.type);
-		len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN;
+		len = (HFSPLUS_SB(sb)->nls ? NLS_MAX_CHARSET_SIZE : 4) *
+		      HFSPLUS_MAX_STRLEN;
 		err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
 		if (err)
 			goto out;

fs/hfsplus/options.c +0 -0

index a975548f6b91..16c08cb5c4f8 100644
@@ -104,6 +104,9 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)