Re: [PATCH v3 6/9] tmpfs: Add casefold lookup support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux