On Tue, Nov 17, 2020 at 04:03:13AM +0000, Daniel Rosenberg wrote:
>
> Currently the casefolding dentry operation are always set if the
> filesystem defines an encoding because the features is toggleable on
> empty directories. Since we don't know what set of functions we'll
> eventually need, and cannot change them later, we add just add them.
This isn't a very useful explanation, since encryption can be toggled on empty
directories too (at least from off to on --- not the other way). Why is
casefolding different?
> +static const struct dentry_operations generic_ci_dentry_ops = {
> + .d_hash = generic_ci_d_hash,
> + .d_compare = generic_ci_d_compare,
> +};
> #endif
> +
> +#ifdef CONFIG_FS_ENCRYPTION
> +static const struct dentry_operations generic_encrypted_dentry_ops = {
> + .d_revalidate = fscrypt_d_revalidate,
> +};
> +#endif
> +
> +#if IS_ENABLED(CONFIG_UNICODE) && IS_ENABLED(CONFIG_FS_ENCRYPTION)
> +static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
> + .d_hash = generic_ci_d_hash,
> + .d_compare = generic_ci_d_compare,
> + .d_revalidate = fscrypt_d_revalidate,
> +};
> +#endif
> +
> +/**
> + * generic_set_encrypted_ci_d_ops - helper for setting d_ops for given dentry
> + * @dentry: dentry to set ops on
> + *
> + * This function sets the dentry ops for the given dentry to handle both
> + * casefolded and encrypted dentry names.
> + *
> + * Encryption requires d_revalidate to remove nokey names once the key is present.
> + * Casefolding is toggleable on an empty directory. Since we can't change the
> + * operations later on, we just add the casefolding ops if the filesystem defines an
> + * encoding.
> + */
There are some overly long lines here (> 80 columns).
But more importantly this still isn't a good explanation. Encryption can also
be enabled on empty directories; what makes casefolding different?
It's also not obvious why so many different copies of the dentry operations
needed, instead of just using generic_encrypted_ci_dentry_ops on all.
If I'm still struggling to understand this after following these patches for a
long time, I expect everyone else will have trouble too...
Here's a suggestion which I think explains it a lot better. It's still possible
I'm misunderstanding something, though, so please check it carefully:
/**
* generic_set_encrypted_ci_d_ops - helper for setting d_ops for given dentry
* @dentry: dentry to set ops on
*
* Casefolded directories need d_hash and d_compare set, so that the dentries
* contained in them are handled case-insensitively. Note that these operations
* are needed on the parent directory rather than on the dentries in it, and the
* casefolding flag can be enabled on an empty directory later but the
* dentry_operations can't be changed later. As a result, if the filesystem has
* casefolding support enabled at all, we have to give all dentries the
* casefolding operations even if their inode doesn't have the casefolding flag
* currently (and thus the casefolding ops would be no-ops for now).
*
* Encryption works differently in that the only dentry operation it needs is
* d_revalidate, which it only needs on dentries that have the no-key name flag.
* The no-key flag can't be set "later", so we don't have to worry about that.
*
* Finally, to maximize compatibility with overlayfs (which isn't compatible
* with certain dentry operations) and to avoid taking an unnecessary
* performance hit, we use custom dentry_operations for each possible
* combination rather always installing all operations.
*/
> +void generic_set_encrypted_ci_d_ops(struct dentry *dentry)
> +{
> +#ifdef CONFIG_FS_ENCRYPTION
> + bool needs_encrypt_ops = dentry->d_flags & DCACHE_NOKEY_NAME;
> +#endif
> +#ifdef CONFIG_UNICODE
> + bool needs_ci_ops = dentry->d_sb->s_encoding;
> +#endif
> +#if defined(CONFIG_FS_ENCRYPTION) && defined(CONFIG_UNICODE)
> + if (needs_encrypt_ops && needs_ci_ops) {
> + d_set_d_op(dentry, &generic_encrypted_ci_dentry_ops);
> + return;
> + }
The return statement above has the wrong indentation level.
> +#endif
> +#ifdef CONFIG_FS_ENCRYPTION
> + if (needs_encrypt_ops) {
> + d_set_d_op(dentry, &generic_encrypted_dentry_ops);
> + return;
> + }
> +#endif
> +#ifdef CONFIG_UNICODE
> + if (needs_ci_ops) {
> + d_set_d_op(dentry, &generic_ci_dentry_ops);
> + return;
> + }
> +#endif
> +}
> +EXPORT_SYMBOL(generic_set_encrypted_ci_d_ops);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 8667d0cdc71e..11345e66353b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3202,6 +3202,7 @@ extern int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str);
> extern int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
> const char *str, const struct qstr *name);
> #endif
> +extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
>
> #ifdef CONFIG_MIGRATION
> extern int buffer_migrate_page(struct address_space *,
