Hey!
On 9/5/24 18:28, Gabriel Krisman Bertazi wrote:
Hi,
André Almeida <andrealmeid@xxxxxxxxxx> writes:
@@ -3427,6 +3431,10 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir,
if (IS_ERR(inode))
return PTR_ERR(inode);
+ if (IS_ENABLED(CONFIG_UNICODE))
+ if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
+ return -EINVAL;
+
if (IS_ENABLED(CONFIG_UNICODE) &&
generic_ci_validate_strict_name(dir, &dentry->d_name))
static const struct constant_table shmem_param_enums_huge[] = {
@@ -4081,9 +4111,62 @@ const struct fs_parameter_spec shmem_fs_parameters[] = {
fsparam_string("grpquota_block_hardlimit", Opt_grpquota_block_hardlimit),
fsparam_string("grpquota_inode_hardlimit", Opt_grpquota_inode_hardlimit),
#endif
+ fsparam_string("casefold", Opt_casefold_version),
+ fsparam_flag ("casefold", Opt_casefold),
+ fsparam_flag ("strict_encoding", Opt_strict_encoding),
I don't know if it is possible, but can we do it with a single parameter?
I tried, but when you use casefold with no args, the code fails
somewhere before that, claiming that there's no arg.
+static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *param,
+ bool latest_version)
Instead of the boolean, can't you check if param->string != NULL? (real
question, I never used fs_parameter.
+{
+ struct shmem_options *ctx = fc->fs_private;
+ unsigned int maj = 0, min = 0, rev = 0, version = 0;
+ struct unicode_map *encoding;
+ char *version_str = param->string + 5;
+ int ret;
unsigned int version = UTF8_LATEST;
and kill the if/else below:
+
+ if (latest_version) {
+ version = UTF8_LATEST;
+ } else {
+ if (strncmp(param->string, "utf8-", 5))
+ return invalfc(fc, "Only UTF-8 encodings are supported "
+ "in the format: utf8-<version number>");
+
+ ret = utf8_parse_version(version_str, &maj, &min, &rev);
utf8_parse_version interface could return UNICODE_AGE() already, so we hide the details
from the caller. wdyt?
I like it!
+ if (ret)
+ return invalfc(fc, "Invalid UTF-8 version: %s", version_str);
+
+ version = UNICODE_AGE(maj, min, rev);
+ }
+
+ encoding = utf8_load(version);
+
+ if (IS_ERR(encoding)) {
+ if (latest_version)
+ return invalfc(fc, "Failed loading latest UTF-8 version");
+ else
+ return invalfc(fc, "Failed loading UTF-8 version: %s", version_str);
The following covers both legs (untested):
if (IS_ERR(encoding))
return invalfc(fc, "Failed loading UTF-8 version: utf8-%u.%u.%u\n"",
unicode_maj(version), unicode_min(version), unicode_rev(version));
+ if (latest_version)
+ pr_info("tmpfs: Using the latest UTF-8 version available");
+ else
+ pr_info("tmpfs: Using encoding provided by mount
options: %s\n", param->string);
The following covers both legs (untested):
pr_info (fc, "tmpfs: Using encoding : utf8-%u.%u.%u\n"
unicode_maj(version), unicode_min(version), unicode_rev(version));
+
+ ctx->encoding = encoding;
+
+ return 0;
+}
+#else
+static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *param,
+ bool latest_version)
+{
+ return invalfc(fc, "tmpfs: No kernel support for casefold filesystems\n");
+}
A message like "Kernel not built with CONFIG_UNICODE" immediately tells
you how to fix it.
@@ -4515,6 +4610,16 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
}
sb->s_export_op = &shmem_export_ops;
sb->s_flags |= SB_NOSEC | SB_I_VERSION;
+
+#if IS_ENABLED(CONFIG_UNICODE)
+ if (ctx->encoding) {
+ sb->s_encoding = ctx->encoding;
+ generic_set_sb_d_ops(sb);
This is the right place for setting d_ops (see the next comment), but you
should be loading generic_ci_always_del_dentry_ops, right?
Also, since generic_ci_always_del_dentry_ops is only used by this one,
can you move it to this file?
+static struct dentry *shmem_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
+{
+ const struct dentry_operations *d_ops = &simple_dentry_operations;
+
+#if IS_ENABLED(CONFIG_UNICODE)
+ if (dentry->d_sb->s_encoding)
+ d_ops = &generic_ci_always_del_dentry_ops;
+#endif
This needs to be done at mount time through sb->s_d_op. See
https://lore.kernel.org/all/20240221171412.10710-1-krisman@xxxxxxx/
I suppose we can do it at mount-time for
generic_ci_always_del_dentry_ops and simple_dentry_operations.
+
+ if (dentry->d_name.len > NAME_MAX)
+ return ERR_PTR(-ENAMETOOLONG);
+
+ if (!dentry->d_sb->s_d_op)
+ d_set_d_op(dentry, d_ops);
+
+ /*
+ * For now, VFS can't deal with case-insensitive negative dentries, so
+ * we prevent them from being created
+ */
+ if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
+ return NULL;
Thinking out loud:
I misunderstood always_delete_dentry before. It removes negative
dentries right after the lookup, since ->d_delete is called on dput.
But you still need this check here, IMO, to prevent the negative dentry
from ever being hashed. Otherwise it can be found by a concurrent
lookup. And you cannot drop ->d_delete from the case-insensitive
operations too, because we still wants it for !IS_CASEFOLDED(dir).
The window is that, without this code, the negative dentry dentry would
be hashed in d_add() and a concurrent lookup might find it between that
time and the d_put, where it is removed at the end of the concurrent
lookup.
All of this would hopefully go away with the negative dentry for
casefolded directories.
+
+ d_add(dentry, NULL);
+
+ return NULL;
+}
The sole reason you are doing this custom function is to exclude negative
dentries from casefolded directories. I doubt we care about the extra
check being done. Can we just do it in simple_lookup?
So, in summary:
* set d_ops at mount time to generic_ci_always_del_dentry_ops
* use simple_lookup(), get rid of shmem_lookup()
* inside of simple_lookup(), add (IS_CASEFOLDED(dir)) return NULL
Right?
+
static const struct inode_operations shmem_dir_inode_operations = {
#ifdef CONFIG_TMPFS
.getattr = shmem_getattr,
.create = shmem_create,
- .lookup = simple_lookup,
+ .lookup = shmem_lookup,
.link = shmem_link,
.unlink = shmem_unlink,
.symlink = shmem_symlink,
@@ -4791,6 +4923,8 @@ int shmem_init_fs_context(struct fs_context *fc)
ctx->uid = current_fsuid();
ctx->gid = current_fsgid();
+ ctx->encoding = NULL;
+
fc->fs_private = ctx;
fc->ops = &shmem_fs_context_ops;
return 0;
