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

[Gabriel Krisman Bertazi <gabriel@krisman.be>]

Hi,

André Almeida <andrealmeid@igalia.com> 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?

> +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?

> +		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@suse.de/

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?

> +
>  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;

-- 
Gabriel Krisman Bertazi