| Message ID | 20240823173332.281211-2-andrealmeid@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tmpfs: Add case-insesitive support for tmpfs | expand |
André Almeida <andrealmeid@igalia.com> writes: > Enable casefold lookup in tmpfs, based on the enconding defined by > userspace. That means that instead of comparing byte per byte a file > name, it compares to a case-insensitive equivalent of the Unicode > string. Hey, > > * dcache handling > > There's a special need when dealing with case-insensitive dentries. > First of all, we currently invalidated every negative casefold dentries. > That happens because currently VFS code has no proper support to deal > with that, giving that it could incorrectly reuse a previous filename > for a new file that has a casefold match. For instance, this could > happen: > > $ mkdir DIR > $ rm -r DIR > $ mkdir dir > $ ls > DIR/ Right. Hopefully we can lift the limitation once we get the negative dentry support for casefolded directories merged. > And would be perceived as inconsistency from userspace point of view, > because even that we match files in a case-insensitive manner, we still > honor whatever is the initial filename. > > Along with that, tmpfs stores only the first equivalent name dentry used > in the dcache, preventing duplications of dentries in the dcache. The > d_compare() version for casefold files stores a normalized string, and > before every lookup, the filename is normalized as well, achieving a > casefolded lookup. This is a bit misleading, because d_compare doesn't store anything. d_compare does a case-insensitive comparison of the filename-under-lookup with the dentry name, but it doesn't store filename-under-lookup. > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h > index 1d06b1e5408a..1a1196b077a6 100644 > --- a/include/linux/shmem_fs.h > +++ b/include/linux/shmem_fs.h > @@ -73,6 +73,7 @@ struct shmem_sb_info { > struct list_head shrinklist; /* List of shinkable inodes */ > unsigned long shrinklist_len; /* Length of shrinklist */ > struct shmem_quota_limits qlimits; /* Default quota limits */ > + bool casefold; This is redundant. you can just check sb->s_encoding != NULL. > }; > > static inline struct shmem_inode_info *SHMEM_I(struct inode *inode) > diff --git a/mm/shmem.c b/mm/shmem.c > index 5a77acf6ac6a..aa272c62f811 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -40,6 +40,8 @@ > #include <linux/fs_parser.h> > #include <linux/swapfile.h> > #include <linux/iversion.h> > +#include <linux/unicode.h> > +#include <linux/parser.h> > #include "swap.h" > > static struct vfsmount *shm_mnt __ro_after_init; > @@ -123,6 +125,8 @@ struct shmem_options { > bool noswap; > unsigned short quota_types; > struct shmem_quota_limits qlimits; > + struct unicode_map *encoding; > + bool strict_encoding; > #define SHMEM_SEEN_BLOCKS 1 > #define SHMEM_SEEN_INODES 2 > #define SHMEM_SEEN_HUGE 4 > @@ -3427,6 +3431,12 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir, > if (IS_ERR(inode)) > return PTR_ERR(inode); > > +#if IS_ENABLED(CONFIG_UNICODE) > + if (sb_has_strict_encoding(dir->i_sb) && IS_CASEFOLDED(dir) && > + dir->i_sb->s_encoding && utf8_validate(dir->i_sb->s_encoding, &dentry->d_name)) > + return -EINVAL; > +#endif Can you made this a helper that other filesystems can use? Also, sorting it to check IS_CASEFOLDED(dir) first would be a good idea. > + > error = simple_acl_create(dir, inode); > if (error) > goto out_iput; > @@ -3435,6 +3445,9 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir, > if (error && error != -EOPNOTSUPP) > goto out_iput; > > + if (IS_CASEFOLDED(dir)) > + d_add(dentry, NULL); > + I get why you do this here and elsewhere: Since we disable negative dentries in case-insensitive directories, you have an unhashed dentry here. We can get away with it in ext4/f2fs because the next lookup will find the file on disk and create the dentry, but in shmem we need to hash it. But it is not the right way to do it. you are effectively creating a negative dentry to turn it positive right below. One problem with that is that if simple_offset_add() fails, you left an illegal negative dentry in a case-insensitive directory. Another, is that a parallel lookup will be able to find the negative dentry temporarily. fsnotify will also behave weirdly. What I think you should do is call d_add once with the proper inode and never call d_instantiate for it. > + /* > + * For now, VFS can't deal with case-insensitive negative dentries, so > + * we destroy them > + */ > + if (IS_CASEFOLDED(dir)) > + d_invalidate(dentry); > + > return 0; > } s/destroy/invalidate/ > @@ -4471,6 +4497,11 @@ static void shmem_put_super(struct super_block *sb) > { > struct shmem_sb_info *sbinfo = SHMEM_SB(sb); > > +#if IS_ENABLED(CONFIG_UNICODE) > + if (sbinfo->casefold) > + utf8_unload(sb->s_encoding); > +#endif if (sb->s_encoding) utf8_unload(sb->s_encoding); > +#if IS_ENABLED(CONFIG_UNICODE) > + if (ctx->encoding) { > + sb->s_encoding = ctx->encoding; > + generic_set_sb_d_ops(sb); > + if (ctx->strict_encoding) > + sb->s_encoding_flags = SB_ENC_STRICT_MODE_FL; > + sbinfo->casefold = true; > + } > +#endif > + > #else > sb->s_flags |= SB_NOUSER; > #endif > @@ -4704,11 +4746,28 @@ static const struct inode_operations shmem_inode_operations = { > #endif > }; > > +static struct dentry *shmem_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) > +{ > + if (dentry->d_name.len > NAME_MAX) > + return ERR_PTR(-ENAMETOOLONG); > + > + /* > + * For now, VFS can't deal with case-insensitive negative dentries, so > + * we prevent them from being created > + */ > + if (IS_CASEFOLDED(dir)) > + return NULL; > + > + d_add(dentry, NULL); > + > + return NULL; > +} > + > 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 +4850,8 @@ int shmem_init_fs_context(struct fs_context *fc) > ctx->uid = current_fsuid(); > ctx->gid = current_fsgid(); > > + ctx->encoding = NULL; > + I find the organization of this patchset a bit weird because the other part of the init_fs_context is only done in patch 3. Perhaps 1 and 3 should be merged together to make review easier.
Hi André, kernel test robot noticed the following build warnings: [auto build test WARNING on akpm-mm/mm-everything] [also build test WARNING on linus/master v6.11-rc5 next-20240826] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/tmpfs-Add-casefold-lookup-support/20240826-135457 base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20240823173332.281211-2-andrealmeid%40igalia.com patch subject: [PATCH 1/5] tmpfs: Add casefold lookup support config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20240827/202408270609.Nj6iM21E-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408270609.Nj6iM21E-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408270609.Nj6iM21E-lkp@intel.com/ All warnings (new ones prefixed by >>): >> mm/shmem.c:4874:23: warning: 'shmem_lookup' defined but not used [-Wunused-function] 4874 | static struct dentry *shmem_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) | ^~~~~~~~~~~~ vim +/shmem_lookup +4874 mm/shmem.c 4873 > 4874 static struct dentry *shmem_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) 4875 { 4876 if (dentry->d_name.len > NAME_MAX) 4877 return ERR_PTR(-ENAMETOOLONG); 4878 4879 /* 4880 * For now, VFS can't deal with case-insensitive negative dentries, so 4881 * we prevent them from being created 4882 */ 4883 if (IS_CASEFOLDED(dir)) 4884 return NULL; 4885 4886 d_add(dentry, NULL); 4887 4888 return NULL; 4889 } 4890
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index 1d06b1e5408a..1a1196b077a6 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -73,6 +73,7 @@ struct shmem_sb_info { struct list_head shrinklist; /* List of shinkable inodes */ unsigned long shrinklist_len; /* Length of shrinklist */ struct shmem_quota_limits qlimits; /* Default quota limits */ + bool casefold; }; static inline struct shmem_inode_info *SHMEM_I(struct inode *inode) diff --git a/mm/shmem.c b/mm/shmem.c index 5a77acf6ac6a..aa272c62f811 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -40,6 +40,8 @@ #include <linux/fs_parser.h> #include <linux/swapfile.h> #include <linux/iversion.h> +#include <linux/unicode.h> +#include <linux/parser.h> #include "swap.h" static struct vfsmount *shm_mnt __ro_after_init; @@ -123,6 +125,8 @@ struct shmem_options { bool noswap; unsigned short quota_types; struct shmem_quota_limits qlimits; + struct unicode_map *encoding; + bool strict_encoding; #define SHMEM_SEEN_BLOCKS 1 #define SHMEM_SEEN_INODES 2 #define SHMEM_SEEN_HUGE 4 @@ -3427,6 +3431,12 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir, if (IS_ERR(inode)) return PTR_ERR(inode); +#if IS_ENABLED(CONFIG_UNICODE) + if (sb_has_strict_encoding(dir->i_sb) && IS_CASEFOLDED(dir) && + dir->i_sb->s_encoding && utf8_validate(dir->i_sb->s_encoding, &dentry->d_name)) + return -EINVAL; +#endif + error = simple_acl_create(dir, inode); if (error) goto out_iput; @@ -3435,6 +3445,9 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir, if (error && error != -EOPNOTSUPP) goto out_iput; + if (IS_CASEFOLDED(dir)) + d_add(dentry, NULL); + error = simple_offset_add(shmem_get_offset_ctx(dir), dentry); if (error) goto out_iput; @@ -3526,6 +3539,9 @@ static int shmem_link(struct dentry *old_dentry, struct inode *dir, goto out; } + if (IS_CASEFOLDED(dir)) + d_add(dentry, NULL); + dir->i_size += BOGO_DIRENT_SIZE; inode_set_mtime_to_ts(dir, inode_set_ctime_to_ts(dir, inode_set_ctime_current(inode))); @@ -3553,6 +3569,14 @@ static int shmem_unlink(struct inode *dir, struct dentry *dentry) inode_inc_iversion(dir); drop_nlink(inode); dput(dentry); /* Undo the count from "create" - does all the work */ + + /* + * For now, VFS can't deal with case-insensitive negative dentries, so + * we destroy them + */ + if (IS_CASEFOLDED(dir)) + d_invalidate(dentry); + return 0; } @@ -3697,6 +3721,8 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir, dir->i_size += BOGO_DIRENT_SIZE; inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir)); inode_inc_iversion(dir); + if (IS_CASEFOLDED(dir)) + d_add(dentry, NULL); d_instantiate(dentry, inode); dget(dentry); return 0; @@ -4471,6 +4497,11 @@ static void shmem_put_super(struct super_block *sb) { struct shmem_sb_info *sbinfo = SHMEM_SB(sb); +#if IS_ENABLED(CONFIG_UNICODE) + if (sbinfo->casefold) + utf8_unload(sb->s_encoding); +#endif + #ifdef CONFIG_TMPFS_QUOTA shmem_disable_quotas(sb); #endif @@ -4515,6 +4546,17 @@ 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); + if (ctx->strict_encoding) + sb->s_encoding_flags = SB_ENC_STRICT_MODE_FL; + sbinfo->casefold = true; + } +#endif + #else sb->s_flags |= SB_NOUSER; #endif @@ -4704,11 +4746,28 @@ static const struct inode_operations shmem_inode_operations = { #endif }; +static struct dentry *shmem_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) +{ + if (dentry->d_name.len > NAME_MAX) + return ERR_PTR(-ENAMETOOLONG); + + /* + * For now, VFS can't deal with case-insensitive negative dentries, so + * we prevent them from being created + */ + if (IS_CASEFOLDED(dir)) + return NULL; + + d_add(dentry, NULL); + + return NULL; +} + 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 +4850,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;
Enable casefold lookup in tmpfs, based on the enconding defined by userspace. That means that instead of comparing byte per byte a file name, it compares to a case-insensitive equivalent of the Unicode string. * dcache handling There's a special need when dealing with case-insensitive dentries. First of all, we currently invalidated every negative casefold dentries. That happens because currently VFS code has no proper support to deal with that, giving that it could incorrectly reuse a previous filename for a new file that has a casefold match. For instance, this could happen: $ mkdir DIR $ rm -r DIR $ mkdir dir $ ls DIR/ And would be perceived as inconsistency from userspace point of view, because even that we match files in a case-insensitive manner, we still honor whatever is the initial filename. Along with that, tmpfs stores only the first equivalent name dentry used in the dcache, preventing duplications of dentries in the dcache. The d_compare() version for casefold files stores a normalized string, and before every lookup, the filename is normalized as well, achieving a casefolded lookup. Signed-off-by: André Almeida <andrealmeid@igalia.com> --- include/linux/shmem_fs.h | 1 + mm/shmem.c | 63 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-)