* [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs @ 2023-08-12 0:41 Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding Gabriel Krisman Bertazi ` (9 more replies) 0 siblings, 10 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, linux-f2fs-devel Hi, This is the v5 of this patchset. Thanks Christian and Eric for you review. In this version, the patchset grew a bit because it adds: - a preparation patch to merge d_revalidate_name and d_revalidate, attending Christian's request. The original patch is now touching several filesystems to update the hook signature, so it has grown quite a bit. But, all of that was autogenerated with coccinelle and tested with allyesconfig only. - A new patch to expose a helper from libfs that I use in ecryptfs. - Code to prevent ecryptfs from mounting on top of casefolded directories. Also following on Christian's review. Other than these, there are some minor fixes, listed in each patch changelog, and more clarifications on the locking, thanks to the excellent feedback from Eric. Eric, I believe I have covered the cases where instantiation can happen and I don't think it is possible for a dentry to become positive amidst the d_revalidation, since we are holding the inode parent lock to synchronize with creations. Please let me know if you find anything else. Finally, I've dropped the r-b tags from patch "fs: Expose name under lookup to d_revalidate hooks", because it changed too much since the time of review. Regarding testing, I verified it doesn't regress fstests for f2fs and ext4, verified building all filesystems work fine with allyesconfig, even with variation of CONFIG_FS_ENCRYPTION and CONFIG_UNICODE, and, finally, I checked I wasn't able to mount or lookup casefolded directories with ecryptfs and overlayfs. Thanks, Gabriel Krisman Bertazi (10): fs: Expose helper to check if a directory needs casefolding ecryptfs: Reject casefold directory inodes 9p: Split ->weak_revalidate from ->revalidate fs: Expose name under lookup to d_revalidate hooks fs: Add DCACHE_CASEFOLDED_NAME flag libfs: Validate negative dentries in case-insensitive directories libfs: Chain encryption checks after case-insensitive revalidation libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops ext4: Enable negative dentries on case-insensitive lookup f2fs: Enable negative dentries on case-insensitive lookup Documentation/filesystems/locking.rst | 3 +- Documentation/filesystems/vfs.rst | 11 ++- fs/9p/vfs_dentry.c | 11 ++- fs/afs/dir.c | 6 +- fs/afs/dynroot.c | 4 +- fs/ceph/dir.c | 3 +- fs/coda/dir.c | 3 +- fs/crypto/fname.c | 3 +- fs/dcache.c | 8 ++ fs/ecryptfs/dentry.c | 5 +- fs/ecryptfs/inode.c | 8 ++ fs/exfat/namei.c | 3 +- fs/ext4/namei.c | 35 +------- fs/f2fs/namei.c | 25 +----- fs/fat/namei_vfat.c | 6 +- fs/fuse/dir.c | 3 +- fs/gfs2/dentry.c | 3 +- fs/hfs/sysdep.c | 3 +- fs/jfs/namei.c | 3 +- fs/kernfs/dir.c | 3 +- fs/libfs.c | 124 +++++++++++++++++--------- fs/namei.c | 18 ++-- fs/nfs/dir.c | 9 +- fs/ocfs2/dcache.c | 4 +- fs/orangefs/dcache.c | 3 +- fs/overlayfs/super.c | 20 +++-- fs/proc/base.c | 6 +- fs/proc/fd.c | 3 +- fs/proc/generic.c | 6 +- fs/proc/proc_sysctl.c | 3 +- fs/reiserfs/xattr.c | 3 +- fs/smb/client/dir.c | 3 +- fs/vboxsf/dir.c | 4 +- include/linux/dcache.h | 10 ++- include/linux/fs.h | 21 +++++ include/linux/fscrypt.h | 4 +- 36 files changed, 242 insertions(+), 148 deletions(-) -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 1:59 ` Eric Biggers 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 02/10] ecryptfs: Reject casefold directory inodes Gabriel Krisman Bertazi ` (8 subsequent siblings) 9 siblings, 1 reply; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, linux-f2fs-devel In preparation to use it in ecryptfs, move needs_casefolding into a public header and give it a namespaced name. Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> --- fs/libfs.c | 14 ++------------ include/linux/fs.h | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/fs/libfs.c b/fs/libfs.c index 5b851315eeed..8d0b64cfd5da 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode) } #if IS_ENABLED(CONFIG_UNICODE) -/* - * Determine if the name of a dentry should be casefolded. - * - * Return: if names will need casefolding - */ -static bool needs_casefold(const struct inode *dir) -{ - return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding; -} - /** * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems * @dentry: dentry whose name we are checking against @@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len, char strbuf[DNAME_INLINE_LEN]; int ret; - if (!dir || !needs_casefold(dir)) + if (!dir || !dir_is_casefolded(dir)) goto fallback; /* * If the dentry name is stored in-line, then it may be concurrently @@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str) const struct unicode_map *um = sb->s_encoding; int ret = 0; - if (!dir || !needs_casefold(dir)) + if (!dir || !dir_is_casefolded(dir)) return 0; ret = utf8_casefold_hash(um, dentry, str); diff --git a/include/linux/fs.h b/include/linux/fs.h index 6867512907d6..e3b631c6d24a 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode) return !IS_DEADDIR(inode); } +/** + * dir_is_casefolded - Safely determine if filenames inside of a + * directory should be casefolded. + * @dir: The directory inode to be checked + * + * Filesystems should usually rely on this instead of checking the + * S_CASEFOLD flag directly when handling inodes, to avoid surprises + * with corrupted volumes. Checking i_sb->s_encoding ensures the + * filesystem knows how to deal with case-insensitiveness. + * + * Return: if names will need casefolding + */ +static inline bool dir_is_casefolded(const struct inode *dir) +{ +#if IS_ENABLED(CONFIG_UNICODE) + return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding; +#else + return false; +#endif +} + extern bool path_noexec(const struct path *path); extern void inode_nohighmem(struct inode *inode); -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding Gabriel Krisman Bertazi @ 2023-08-12 1:59 ` Eric Biggers 2023-08-12 23:06 ` Theodore Ts'o 2023-08-14 15:02 ` Gabriel Krisman Bertazi 0 siblings, 2 replies; 32+ messages in thread From: Eric Biggers @ 2023-08-12 1:59 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Fri, Aug 11, 2023 at 08:41:37PM -0400, Gabriel Krisman Bertazi wrote: > In preparation to use it in ecryptfs, move needs_casefolding into a > public header and give it a namespaced name. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> > --- > fs/libfs.c | 14 ++------------ > include/linux/fs.h | 21 +++++++++++++++++++++ > 2 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/fs/libfs.c b/fs/libfs.c > index 5b851315eeed..8d0b64cfd5da 100644 > --- a/fs/libfs.c > +++ b/fs/libfs.c > @@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode) > } > > #if IS_ENABLED(CONFIG_UNICODE) > -/* > - * Determine if the name of a dentry should be casefolded. > - * > - * Return: if names will need casefolding > - */ > -static bool needs_casefold(const struct inode *dir) > -{ > - return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding; > -} > - > /** > * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems > * @dentry: dentry whose name we are checking against > @@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len, > char strbuf[DNAME_INLINE_LEN]; > int ret; > > - if (!dir || !needs_casefold(dir)) > + if (!dir || !dir_is_casefolded(dir)) > goto fallback; > /* > * If the dentry name is stored in-line, then it may be concurrently > @@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str) > const struct unicode_map *um = sb->s_encoding; > int ret = 0; > > - if (!dir || !needs_casefold(dir)) > + if (!dir || !dir_is_casefolded(dir)) > return 0; > > ret = utf8_casefold_hash(um, dentry, str); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 6867512907d6..e3b631c6d24a 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode) > return !IS_DEADDIR(inode); > } > > +/** > + * dir_is_casefolded - Safely determine if filenames inside of a > + * directory should be casefolded. > + * @dir: The directory inode to be checked > + * > + * Filesystems should usually rely on this instead of checking the > + * S_CASEFOLD flag directly when handling inodes, to avoid surprises > + * with corrupted volumes. Checking i_sb->s_encoding ensures the > + * filesystem knows how to deal with case-insensitiveness. > + * > + * Return: if names will need casefolding > + */ > +static inline bool dir_is_casefolded(const struct inode *dir) > +{ > +#if IS_ENABLED(CONFIG_UNICODE) > + return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding; > +#else > + return false; > +#endif > +} To be honest I've always been confused about why the ->s_encoding check is there. It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops caused by spurious casefold flag") to address a fuzzing report for a filesystem that had a casefolded directory but didn't have the casefold feature flag set. It seems like an unnecessarily complex fix, though. The filesystem should just reject the inode earlier, in __ext4_iget(). And likewise for f2fs. Then no other code has to worry about this problem. Actually, f2fs already does it, as I added it in commit f6322f3f1212: if ((fi->i_flags & F2FS_CASEFOLD_FL) && !f2fs_sb_has_casefold(sbi)) { set_sbi_flag(sbi, SBI_NEED_FSCK); f2fs_warn(sbi, "%s: inode (ino=%lx) has casefold flag, but casefold feature is off", __func__, inode->i_ino); return false; } So just __ext4_iget() needs to be fixed. I think we should consider doing that before further entrenching all the extra ->s_encoding checks. Also I don't understand why this needs to be part of your patch series anyway. Shouldn't eCryptfs check IS_CASEFOLDED() anyway? - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-12 1:59 ` Eric Biggers @ 2023-08-12 23:06 ` Theodore Ts'o 2023-08-13 0:12 ` Eric Biggers 2023-08-13 3:08 ` Matthew Wilcox 2023-08-14 15:02 ` Gabriel Krisman Bertazi 1 sibling, 2 replies; 32+ messages in thread From: Theodore Ts'o @ 2023-08-12 23:06 UTC (permalink / raw) To: Eric Biggers Cc: Gabriel Krisman Bertazi, brauner, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Fri, Aug 11, 2023 at 06:59:15PM -0700, Eric Biggers wrote: > > To be honest I've always been confused about why the ->s_encoding check is > there. It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops > caused by spurious casefold flag") to address a fuzzing report for a filesystem > that had a casefolded directory but didn't have the casefold feature flag set. > It seems like an unnecessarily complex fix, though. The filesystem should just > reject the inode earlier, in __ext4_iget(). And likewise for f2fs. Then no > other code has to worry about this problem. It's not enough to check it in ext4_iget, since the casefold flag can get set *after* the inode has been fetched, but before you try to use it. This can happen because syzbot has opened the block device for writing, and edits the superblock while it is mounted. One could say that this is an insane threat model, but the syzbot team thinks that this can be used to break out of a kernel lockdown after a UEFI secure boot. Which is fine, except I don't think I've been able to get any company (including Google) to pay for headcount to fix problems like this, and the unremitting stream of these sorts of syzbot reports have already caused one major file system developer to burn out and step down. So problems like this get fixed on my own time, and when I have some free time. And if we "simplify" the code, it will inevitably cause more syzbot reports, which I will then have to ignore, and the syzbot team will write more "kernel security disaster" slide deck presentations to senior VP's, although I'll note this has never resulted in my getting any additional SWE's to help me fix the problem... > So just __ext4_iget() needs to be fixed. I think we should consider doing that > before further entrenching all the extra ->s_encoding checks. If we can get an upstream kernel consensus that syzbot reports caused by writing to a mounted file system aren't important, and we can publish this somewhere where hopefully the syzbot team will pay attention to it, sure... - Ted _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-12 23:06 ` Theodore Ts'o @ 2023-08-13 0:12 ` Eric Biggers 2023-08-13 3:08 ` Matthew Wilcox 1 sibling, 0 replies; 32+ messages in thread From: Eric Biggers @ 2023-08-13 0:12 UTC (permalink / raw) To: Theodore Ts'o Cc: Gabriel Krisman Bertazi, brauner, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Sat, Aug 12, 2023 at 07:06:47PM -0400, Theodore Ts'o wrote: > On Fri, Aug 11, 2023 at 06:59:15PM -0700, Eric Biggers wrote: > > > > To be honest I've always been confused about why the ->s_encoding check is > > there. It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops > > caused by spurious casefold flag") to address a fuzzing report for a filesystem > > that had a casefolded directory but didn't have the casefold feature flag set. > > It seems like an unnecessarily complex fix, though. The filesystem should just > > reject the inode earlier, in __ext4_iget(). And likewise for f2fs. Then no > > other code has to worry about this problem. > > the casefold flag can get set *after* the inode has been fetched, but before > you try to use it. This can happen because syzbot has opened the block device > for writing, and edits the superblock while it is mounted. I don't see how that is relevant here. I think the actual problem you're hinting at is that checking the casefold feature after the filesystem has been mounted is not guaranteed to work properly, as ->s_encoding will be NULL if the casefold feature was not present at mount time. If we'd like to be robust in the event of the casefold feature being concurrently enabled by a write to the block device, then all we need to do is avoid checking the casefold feature after mount time, and instead check ->s_encoding. I believe __ext4_iget() is still the only place it's needed. > One could say that this is an insane threat model, but the syzbot team > thinks that this can be used to break out of a kernel lockdown after a > UEFI secure boot. Which is fine, except I don't think I've been able > to get any company (including Google) to pay for headcount to fix > problems like this, and the unremitting stream of these sorts of > syzbot reports have already caused one major file system developer to > burn out and step down. > > So problems like this get fixed on my own time, and when I have some > free time. And if we "simplify" the code, it will inevitably cause > more syzbot reports, which I will then have to ignore, and the syzbot > team will write more "kernel security disaster" slide deck > presentations to senior VP's, although I'll note this has never > resulted in my getting any additional SWE's to help me fix the > problem... > > > So just __ext4_iget() needs to be fixed. I think we should consider doing that > > before further entrenching all the extra ->s_encoding checks. > > If we can get an upstream kernel consensus that syzbot reports caused > by writing to a mounted file system aren't important, and we can > publish this somewhere where hopefully the syzbot team will pay > attention to it, sure... But, more generally, I think it's clear that concurrent writes to the block device's page cache is not something that filesystems can be robust against. I think this needs to be solved by providing an option to forbid this, as Jan Kara's patchset "block: Add config option to not allow writing to mounted devices" does, and then transitioning legacy use cases to new APIs. Yes, "transitioning legacy use cases" will be a lot of work. And if The Linux Filesystem Maintainers(TM) do not have time for it, that's the way it is. Someone who cares about it (such as someone who actually cares about the potential impact on the Lockdown feature) will need to come along and do it. But I think that should be the plan, and The Linux Filesystem Maintainers(TM) do not need to try to play whack-a-mole with "fixing" filesystem code to be consistently revalidating already-validated cached metadata. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-12 23:06 ` Theodore Ts'o 2023-08-13 0:12 ` Eric Biggers @ 2023-08-13 3:08 ` Matthew Wilcox 2023-08-13 4:30 ` Eric Biggers 1 sibling, 1 reply; 32+ messages in thread From: Matthew Wilcox @ 2023-08-13 3:08 UTC (permalink / raw) To: Theodore Ts'o Cc: Gabriel Krisman Bertazi, brauner, linux-f2fs-devel, Eric Biggers, viro, linux-fsdevel, jaegeuk, linux-ext4 On Sat, Aug 12, 2023 at 07:06:47PM -0400, Theodore Ts'o wrote: > One could say that this is an insane threat model, but the syzbot team > thinks that this can be used to break out of a kernel lockdown after a > UEFI secure boot. Which is fine, except I don't think I've been able > to get any company (including Google) to pay for headcount to fix > problems like this, and the unremitting stream of these sorts of > syzbot reports have already caused one major file system developer to > burn out and step down. > > So problems like this get fixed on my own time, and when I have some > free time. And if we "simplify" the code, it will inevitably cause > more syzbot reports, which I will then have to ignore, and the syzbot > team will write more "kernel security disaster" slide deck > presentations to senior VP's, although I'll note this has never > resulted in my getting any additional SWE's to help me fix the > problem... > > > So just __ext4_iget() needs to be fixed. I think we should consider doing that > > before further entrenching all the extra ->s_encoding checks. > > If we can get an upstream kernel consensus that syzbot reports caused > by writing to a mounted file system aren't important, and we can > publish this somewhere where hopefully the syzbot team will pay > attention to it, sure... What the syzbot team don't seem to understand is that more bug reports aren't better. I used to investigate one most days, but the onslaught is relentless and I just ignore syzbot reports now. I appreciate maintainers don't necessarily get that privilege. They seem motivated to find new and exciting ways to break the kernel without realising that they're sapping the will to work on the bugs that we already have. Hm. Maybe this is a topic for kernel-summit? _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-13 3:08 ` Matthew Wilcox @ 2023-08-13 4:30 ` Eric Biggers 2023-08-14 11:38 ` Theodore Ts'o 0 siblings, 1 reply; 32+ messages in thread From: Eric Biggers @ 2023-08-13 4:30 UTC (permalink / raw) To: Matthew Wilcox Cc: Gabriel Krisman Bertazi, brauner, Theodore Ts'o, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Sun, Aug 13, 2023 at 04:08:58AM +0100, Matthew Wilcox wrote: > On Sat, Aug 12, 2023 at 07:06:47PM -0400, Theodore Ts'o wrote: > > One could say that this is an insane threat model, but the syzbot team > > thinks that this can be used to break out of a kernel lockdown after a > > UEFI secure boot. Which is fine, except I don't think I've been able > > to get any company (including Google) to pay for headcount to fix > > problems like this, and the unremitting stream of these sorts of > > syzbot reports have already caused one major file system developer to > > burn out and step down. > > > > So problems like this get fixed on my own time, and when I have some > > free time. And if we "simplify" the code, it will inevitably cause > > more syzbot reports, which I will then have to ignore, and the syzbot > > team will write more "kernel security disaster" slide deck > > presentations to senior VP's, although I'll note this has never > > resulted in my getting any additional SWE's to help me fix the > > problem... > > > > > So just __ext4_iget() needs to be fixed. I think we should consider doing that > > > before further entrenching all the extra ->s_encoding checks. > > > > If we can get an upstream kernel consensus that syzbot reports caused > > by writing to a mounted file system aren't important, and we can > > publish this somewhere where hopefully the syzbot team will pay > > attention to it, sure... > > What the syzbot team don't seem to understand is that more bug reports > aren't better. I used to investigate one most days, but the onslaught is > relentless and I just ignore syzbot reports now. I appreciate maintainers > don't necessarily get that privilege. > > They seem motivated to find new and exciting ways to break the kernel > without realising that they're sapping the will to work on the bugs that > we already have. > Well, one thing that the kernel community can do to make things better is identify when a large number of bug reports are caused by a single issue ("userspace can write to mounted block devices"), and do something about that underlying issue (https://lore.kernel.org/r/20230704122727.17096-1-jack@suse.cz) instead of trying to "fix" large numbers of individual "bugs". We can have 1000 bugs or 1 bug, it is actually our choice in this case. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-13 4:30 ` Eric Biggers @ 2023-08-14 11:38 ` Theodore Ts'o 2023-08-14 17:22 ` Eric Biggers 0 siblings, 1 reply; 32+ messages in thread From: Theodore Ts'o @ 2023-08-14 11:38 UTC (permalink / raw) To: Eric Biggers Cc: Gabriel Krisman Bertazi, brauner, Matthew Wilcox, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Sat, Aug 12, 2023 at 09:30:22PM -0700, Eric Biggers wrote: > Well, one thing that the kernel community can do to make things better is > identify when a large number of bug reports are caused by a single issue > ("userspace can write to mounted block devices"), and do something about that > underlying issue (https://lore.kernel.org/r/20230704122727.17096-1-jack@suse.cz) > instead of trying to "fix" large numbers of individual "bugs". We can have 1000 > bugs or 1 bug, it is actually our choice in this case. That's assuming the syzbot folks are willing to enable the config in Jan's patch. The syzbot folks refused to enable it, unless the config was gated on CONFIG_INSECURE, which I object to, because that's presuming a threat model that we have not all agreed is valid. Or rather, if it *is* valid some community members (or cough, cough, **companies**) need to step up and supply patches. As the saying goes, "patches gratefully accepted". It is *not* the maintainer's responsibility to grant every single person whining about a feature request, or even a bug fix. - Ted _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-14 11:38 ` Theodore Ts'o @ 2023-08-14 17:22 ` Eric Biggers 2023-08-15 3:59 ` Theodore Ts'o 0 siblings, 1 reply; 32+ messages in thread From: Eric Biggers @ 2023-08-14 17:22 UTC (permalink / raw) To: Theodore Ts'o Cc: Gabriel Krisman Bertazi, brauner, Matthew Wilcox, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Mon, Aug 14, 2023 at 07:38:52AM -0400, Theodore Ts'o wrote: > On Sat, Aug 12, 2023 at 09:30:22PM -0700, Eric Biggers wrote: > > Well, one thing that the kernel community can do to make things better is > > identify when a large number of bug reports are caused by a single issue > > ("userspace can write to mounted block devices"), and do something about that > > underlying issue (https://lore.kernel.org/r/20230704122727.17096-1-jack@suse.cz) > > instead of trying to "fix" large numbers of individual "bugs". We can have 1000 > > bugs or 1 bug, it is actually our choice in this case. > > That's assuming the syzbot folks are willing to enable the config in > Jan's patch. The syzbot folks refused to enable it, unless the config > was gated on CONFIG_INSECURE, which I object to, because that's > presuming a threat model that we have not all agreed is valid. > > Or rather, if it *is* valid some community members (or cough, cough, > **companies**) need to step up and supply patches. As the saying > goes, "patches gratefully accepted". It is *not* the maintainer's > responsibility to grant every single person whining about a feature > request, or even a bug fix. They did disable CONFIG_XFS_SUPPORT_V4. Yes, there was some pushback, but they are very understandably (and correctly) concerned about ending up in a situation where buggy code is disabled for syzkaller but enabled for everyone else. They eventually did accept the proposal to disable CONFIG_XFS_SUPPORT_V4, for reasons including the fact that there is a concrete deprecation plan. (FWIW, the XFS maintainers had also pushed back strongly when I suggested adding a kconfig option for XFS v4 support back in 2018. Sometimes these things just take time.) Anyway, syzkaller is just the messenger that is reminding us of a problem. The actual problem here is that "make filesystems robust against concurrent changes to block device's page cache" is effectively unsolvable. *Every* memory access to the cache would need to use READ_ONCE/WRITE_ONCE, and *every* re-read of every field would need to be fully re-validated. PageChecked would need to go away for metadata, as it would be invalid to cache the checked status at all. There's basically zero chance of getting this correct; filesystems struggle to validate even the metadata read from disk correctly, so how are they going to successfully continually revalidate all cached metadata in memory? I don't understand why we would waste time trying to do that instead of focusing on an actual solution. Sure, probably The Linux Filesystem Maintainers(TM) don't have time to help with migrating legacy use cases of writing to mounted block devices, but that just means that someone needs to step up to do it. It doesn't mean that they need to instead waste time on pointless "fixes". Keep in mind, the syzkaller team isn't asking for these pointless "fixes" either. They'd very much prefer 1 fix to 1000 fixes. I think some confusion might be arising from the very different types of problems that syzkaller finds. Sometimes 1 syzkaller report == 1 bug == 1 high-priority "must fix" bug == 1 vulnerability == 1 fix needed. But in general syzkaller is just letting kernel developers know about a problem, and it is up to them to decide what to do about it. In this case there is one underlying issue that needs to be fixed, and the individual syzkaller reports that result from that issue are not important. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-14 17:22 ` Eric Biggers @ 2023-08-15 3:59 ` Theodore Ts'o 0 siblings, 0 replies; 32+ messages in thread From: Theodore Ts'o @ 2023-08-15 3:59 UTC (permalink / raw) To: Eric Biggers Cc: Gabriel Krisman Bertazi, brauner, Matthew Wilcox, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Mon, Aug 14, 2023 at 10:22:44AM -0700, Eric Biggers wrote: > > Keep in mind, the syzkaller team isn't asking for these pointless "fixes" > either. They'd very much prefer 1 fix to 1000 fixes. I think some confusion > might be arising from the very different types of problems that syzkaller finds. > Sometimes 1 syzkaller report == 1 bug == 1 high-priority "must fix" bug == 1 > vulnerability == 1 fix needed. But in general syzkaller is just letting kernel > developers know about a problem, and it is up to them to decide what to do about > it. In this case there is one underlying issue that needs to be fixed, and the > individual syzkaller reports that result from that issue are not important. ... except that the Syzkaller folks have created slide decks talking about "Linux kernel security disaster", blaming the entire community, where they quote the number unresolved syzkaller reports, without the kind of nuance that you are referring to. There is also not a great way of categorizing syzkaller reports as "requires maliciously fuzzed file system image", or "writing to mounted file system" --- either manually, or (ideally) automatically, since the syzbot test generators knows what they are doing. And finally, the reality is even if someone where to fix the "one underlying issue", the reality is that it will be ten years or so before said fixed can be rolled out, since it requires changes in userspace utilities, as well as rolled out kernels, and enterprise distros are around for a decade; even community distros need to be supported for at least 3-5 years. Finally, it's not just "one underlying issue"; there are also "maliciously fuzzed file systems", and working around those syzbot reports can be quite painful, especially the ones that lead to lockdep deadlock reports. Many of these are spurious, caused by an inode being used in two contexts, that can only happen in a badly corrupted file system, and for which we've already signalled that the file system is corrupted, so if you panic on error, it wouldn't deadlock. (And if you deadlock, it's not _that_ much worse than panicing on a maliciously fuzzed file system.) And all of these bugs get counted, one for each lockdep report variation (so there can be 3-4 per root cause) as a "security bug" in the "Linux kernel security disaster" statistics. I might not mind the hyperbole if said slide decks asked for more headcount. But instead, they blame the "Linux upstream community" for not fixing bugs, or treating the bugs seriously. Sigh.... - Ted _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-12 1:59 ` Eric Biggers 2023-08-12 23:06 ` Theodore Ts'o @ 2023-08-14 15:02 ` Gabriel Krisman Bertazi 2023-08-14 19:14 ` Eric Biggers 1 sibling, 1 reply; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-14 15:02 UTC (permalink / raw) To: Eric Biggers Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 Eric Biggers <ebiggers@kernel.org> writes: > On Fri, Aug 11, 2023 at 08:41:37PM -0400, Gabriel Krisman Bertazi wrote: >> In preparation to use it in ecryptfs, move needs_casefolding into a >> public header and give it a namespaced name. >> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> >> --- >> fs/libfs.c | 14 ++------------ >> include/linux/fs.h | 21 +++++++++++++++++++++ >> 2 files changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/fs/libfs.c b/fs/libfs.c >> index 5b851315eeed..8d0b64cfd5da 100644 >> --- a/fs/libfs.c >> +++ b/fs/libfs.c >> @@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode) >> } >> >> #if IS_ENABLED(CONFIG_UNICODE) >> -/* >> - * Determine if the name of a dentry should be casefolded. >> - * >> - * Return: if names will need casefolding >> - */ >> -static bool needs_casefold(const struct inode *dir) >> -{ >> - return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding; >> -} >> - >> /** >> * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems >> * @dentry: dentry whose name we are checking against >> @@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len, >> char strbuf[DNAME_INLINE_LEN]; >> int ret; >> >> - if (!dir || !needs_casefold(dir)) >> + if (!dir || !dir_is_casefolded(dir)) >> goto fallback; >> /* >> * If the dentry name is stored in-line, then it may be concurrently >> @@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str) >> const struct unicode_map *um = sb->s_encoding; >> int ret = 0; >> >> - if (!dir || !needs_casefold(dir)) >> + if (!dir || !dir_is_casefolded(dir)) >> return 0; >> >> ret = utf8_casefold_hash(um, dentry, str); >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 6867512907d6..e3b631c6d24a 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode) >> return !IS_DEADDIR(inode); >> } >> >> +/** >> + * dir_is_casefolded - Safely determine if filenames inside of a >> + * directory should be casefolded. >> + * @dir: The directory inode to be checked >> + * >> + * Filesystems should usually rely on this instead of checking the >> + * S_CASEFOLD flag directly when handling inodes, to avoid surprises >> + * with corrupted volumes. Checking i_sb->s_encoding ensures the >> + * filesystem knows how to deal with case-insensitiveness. >> + * >> + * Return: if names will need casefolding >> + */ >> +static inline bool dir_is_casefolded(const struct inode *dir) >> +{ >> +#if IS_ENABLED(CONFIG_UNICODE) >> + return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding; >> +#else >> + return false; >> +#endif >> +} > > To be honest I've always been confused about why the ->s_encoding check is > there. It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops > caused by spurious casefold flag") to address a fuzzing report for a filesystem > that had a casefolded directory but didn't have the casefold feature flag set. > It seems like an unnecessarily complex fix, though. The filesystem should just > reject the inode earlier, in __ext4_iget(). And likewise for f2fs. Then no > other code has to worry about this problem. > > Actually, f2fs already does it, as I added it in commit f6322f3f1212: > > if ((fi->i_flags & F2FS_CASEFOLD_FL) && !f2fs_sb_has_casefold(sbi)) { > set_sbi_flag(sbi, SBI_NEED_FSCK); > f2fs_warn(sbi, "%s: inode (ino=%lx) has casefold flag, but casefold feature is off", > __func__, inode->i_ino); > return false; > } > > So just __ext4_iget() needs to be fixed. I think we should consider doing that > before further entrenching all the extra ->s_encoding checks. > > Also I don't understand why this needs to be part of your patch series anyway. > Shouldn't eCryptfs check IS_CASEFOLDED() anyway? While I agree with Ted's point about how this change is tiny compared to the benefit of preventing casefold-related superblock corruptions on runtime (and we want to keep it being done in ext4), I also agree with you that we don't need to check it also in ecryptfs. But, I'll preserve it in d_revalidate, since it is what we are currently doing in d_compare/d_hash. Also, this patchset has been sitting for years before the latest discussions, and I'm tired of it, so I'm happy to keep this discussion for another time. Will drop this patch and just check IS_CASEFOLDED in ecryptfs for the next iteration. I'll follow up with another case-insensitive cleanup patchset I've been siting on forever, which includes this patch and will restart this discussion, among others. -- Gabriel Krisman Bertazi _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-14 15:02 ` Gabriel Krisman Bertazi @ 2023-08-14 19:14 ` Eric Biggers 2023-08-14 19:26 ` Gabriel Krisman Bertazi 0 siblings, 1 reply; 32+ messages in thread From: Eric Biggers @ 2023-08-14 19:14 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 On Mon, Aug 14, 2023 at 11:02:38AM -0400, Gabriel Krisman Bertazi wrote: > > Also, this patchset has been sitting for years before the latest > discussions, and I'm tired of it, so I'm happy to keep this > discussion for another time. Will drop this patch and just check > IS_CASEFOLDED in ecryptfs for the next iteration. > > I'll follow up with another case-insensitive cleanup patchset I've been > siting on forever, which includes this patch and will restart this > discussion, among others. > Well, as we know unfortunately filesystem developers are in short supply, and doing proper reviews (identifying issues and working closely with the patchset author over multiple iterations to address them, as opposed to just slapping on a Reviewed-by) is very time consuming. Earlier this year I tried to get the Android Systems team, which is ostensibly responsible for Android's use of casefolding, to take a look, but their entire feedback was just "looks good to me". Also, the fact that this patchset originally excluded the casefold+encrypt case technically made it not applicable to Android, and discouraged me from taking a look since encryption is my focus. Sorry for not taking a look sooner. Anyway, thanks for doing this, and I think it's near the finish line now. Once you address the latest feedback and get a couple acks, I think that Christian should take this through the VFS tree. BTW, in my opinion, as the maintainer of the "Unicode subsystem" you are also authorized to send a pull request for this to Linus yourself. But VFS does seem ideal in this case, given the diffstat, and Christian has been fairly active with taking patches. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding 2023-08-14 19:14 ` Eric Biggers @ 2023-08-14 19:26 ` Gabriel Krisman Bertazi 0 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-14 19:26 UTC (permalink / raw) To: Eric Biggers Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4 Eric Biggers <ebiggers@kernel.org> writes: > On Mon, Aug 14, 2023 at 11:02:38AM -0400, Gabriel Krisman Bertazi wrote: >> >> Also, this patchset has been sitting for years before the latest >> discussions, and I'm tired of it, so I'm happy to keep this >> discussion for another time. Will drop this patch and just check >> IS_CASEFOLDED in ecryptfs for the next iteration. >> >> I'll follow up with another case-insensitive cleanup patchset I've been >> siting on forever, which includes this patch and will restart this >> discussion, among others. >> > > Well, as we know unfortunately filesystem developers are in short supply, and > doing proper reviews (identifying issues and working closely with the patchset > author over multiple iterations to address them, as opposed to just slapping on > a Reviewed-by) is very time consuming. Earlier this year I tried to get the > Android Systems team, which is ostensibly responsible for Android's use of > casefolding, to take a look, but their entire feedback was just "looks good to > me". Also, the fact that this patchset originally excluded the casefold+encrypt > case technically made it not applicable to Android, and discouraged me from > taking a look since encryption is my focus. Sorry for not taking a look sooner. > > Anyway, thanks for doing this, and I think it's near the finish line now. Once On the contrary, thank *you* for your review. I always appreciate your feedback, particularly since you are always able to find the corner cases I missed. That, and your responsiveness, are the reasons I always put you in my --cc list since v1 for anything related to unicode/fs :) -- Gabriel Krisman Bertazi _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 02/10] ecryptfs: Reject casefold directory inodes 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 03/10] 9p: Split ->weak_revalidate from ->revalidate Gabriel Krisman Bertazi ` (7 subsequent siblings) 9 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, linux-f2fs-devel Even though it seems to be able to resolve some names of case-insensitive directories, the lack of d_hash and d_compare means we end up with a broken state in the d_cache. Considering it was never a goal to support these two together, and we are preparing to use d_revalidate in case-insensitive filesystems, which would make the combination even more broken, reject any attempt to get a casefolded inode from ecryptfs. Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> --- fs/ecryptfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 83274915ba6d..1305dc49df78 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -78,6 +78,14 @@ static struct inode *__ecryptfs_get_inode(struct inode *lower_inode, if (lower_inode->i_sb != ecryptfs_superblock_to_lower(sb)) return ERR_PTR(-EXDEV); + + /* Reject dealing with casefold directories. */ + if (S_ISDIR(lower_inode->i_mode) && dir_is_casefolded(lower_inode)) { + pr_err_ratelimited("%s: Can't handle casefolded directory.\n", + __func__); + return ERR_PTR(-EREMOTE); + } + if (!igrab(lower_inode)) return ERR_PTR(-ESTALE); inode = iget5_locked(sb, (unsigned long)lower_inode, -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 03/10] 9p: Split ->weak_revalidate from ->revalidate 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 02/10] ecryptfs: Reject casefold directory inodes Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi ` (6 subsequent siblings) 9 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, linux-f2fs-devel In preparation to change the signature of dentry_ops->revalidate, avoid reusing the handler directly for d_weak_revalidate in 9p. Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> --- fs/9p/vfs_dentry.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c index f16f73581634..0c6fa1f53530 100644 --- a/fs/9p/vfs_dentry.c +++ b/fs/9p/vfs_dentry.c @@ -94,9 +94,15 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags) return 1; } +static int v9fs_lookup_weak_revalidate(struct dentry *dentry, + unsigned int flags) +{ + return v9fs_lookup_revalidate(dentry, flags); +} + const struct dentry_operations v9fs_cached_dentry_operations = { .d_revalidate = v9fs_lookup_revalidate, - .d_weak_revalidate = v9fs_lookup_revalidate, + .d_weak_revalidate = v9fs_lookup_weak_revalidate, .d_delete = v9fs_cached_dentry_delete, .d_release = v9fs_dentry_release, }; -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (2 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 03/10] 9p: Split ->weak_revalidate from ->revalidate Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 2:15 ` Eric Biggers ` (2 more replies) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag Gabriel Krisman Bertazi ` (5 subsequent siblings) 9 siblings, 3 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> Negative dentries support on case-insensitive ext4/f2fs will require access to the name under lookup to ensure it matches the dentry. This adds the information on d_revalidate and updates its implementation. This was done through a Coccinelle hook and tested by building with allyesconfig. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v3: - Merge d_revalidate_name with d_revalidate (Christian) - Drop Ted's r-b since patch changed quite a bit. (Me) Changes since v2: - Document d_revalidate_name hook. (Eric) --- Documentation/filesystems/locking.rst | 3 ++- Documentation/filesystems/vfs.rst | 11 ++++++++++- fs/9p/vfs_dentry.c | 5 +++-- fs/afs/dir.c | 6 ++++-- fs/afs/dynroot.c | 4 +++- fs/ceph/dir.c | 3 ++- fs/coda/dir.c | 3 ++- fs/crypto/fname.c | 3 ++- fs/ecryptfs/dentry.c | 5 +++-- fs/exfat/namei.c | 3 ++- fs/fat/namei_vfat.c | 6 ++++-- fs/fuse/dir.c | 3 ++- fs/gfs2/dentry.c | 3 ++- fs/hfs/sysdep.c | 3 ++- fs/jfs/namei.c | 3 ++- fs/kernfs/dir.c | 3 ++- fs/namei.c | 18 ++++++++++-------- fs/nfs/dir.c | 9 ++++++--- fs/ocfs2/dcache.c | 4 +++- fs/orangefs/dcache.c | 3 ++- fs/overlayfs/super.c | 20 ++++++++++++-------- fs/proc/base.c | 6 ++++-- fs/proc/fd.c | 3 ++- fs/proc/generic.c | 6 ++++-- fs/proc/proc_sysctl.c | 3 ++- fs/reiserfs/xattr.c | 3 ++- fs/smb/client/dir.c | 3 ++- fs/vboxsf/dir.c | 4 +++- include/linux/dcache.h | 2 +- include/linux/fscrypt.h | 4 +++- 30 files changed, 103 insertions(+), 52 deletions(-) diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst index ed148919e11a..1603c53a1688 100644 --- a/Documentation/filesystems/locking.rst +++ b/Documentation/filesystems/locking.rst @@ -17,7 +17,8 @@ dentry_operations prototypes:: - int (*d_revalidate)(struct dentry *, unsigned int); + int (*d_revalidate)(struct dentry *, const struct qstr *, + unsigned int); int (*d_weak_revalidate)(struct dentry *, unsigned int); int (*d_hash)(const struct dentry *, struct qstr *); int (*d_compare)(const struct dentry *, diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst index cb2a97e49872..ddd542c2a722 100644 --- a/Documentation/filesystems/vfs.rst +++ b/Documentation/filesystems/vfs.rst @@ -1251,7 +1251,8 @@ defined: .. code-block:: c struct dentry_operations { - int (*d_revalidate)(struct dentry *, unsigned int); + int (*d_revalidate)(struct dentry *, const struct qstr *, + unsigned int); int (*d_weak_revalidate)(struct dentry *, unsigned int); int (*d_hash)(const struct dentry *, struct qstr *); int (*d_compare)(const struct dentry *, @@ -1284,6 +1285,14 @@ defined: they can change and, in d_inode case, even become NULL under us). + Filesystems shouldn't rely on the name under lookup, unless + there are particular filename encoding semantics to be handled + during revalidation. Note the name under lookup can change from + under d_revalidate, so it must be protected with ->d_lock before + accessing. The exception is when revalidating negative dentries + for creation, in which case the parent inode prevents it from + changing. + If a situation is encountered that rcu-walk cannot handle, return -ECHILD and it will be called again in ref-walk mode. diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c index 0c6fa1f53530..de679d9505db 100644 --- a/fs/9p/vfs_dentry.c +++ b/fs/9p/vfs_dentry.c @@ -56,7 +56,8 @@ static void v9fs_dentry_release(struct dentry *dentry) dentry->d_fsdata = NULL; } -static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags) +static int v9fs_lookup_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { struct p9_fid *fid; struct inode *inode; @@ -97,7 +98,7 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags) static int v9fs_lookup_weak_revalidate(struct dentry *dentry, unsigned int flags) { - return v9fs_lookup_revalidate(dentry, flags); + return v9fs_lookup_revalidate(dentry, NULL, flags); } const struct dentry_operations v9fs_cached_dentry_operations = { diff --git a/fs/afs/dir.c b/fs/afs/dir.c index 5219182e52e1..e3ba14512715 100644 --- a/fs/afs/dir.c +++ b/fs/afs/dir.c @@ -21,7 +21,8 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); static int afs_dir_open(struct inode *inode, struct file *file); static int afs_readdir(struct file *file, struct dir_context *ctx); -static int afs_d_revalidate(struct dentry *dentry, unsigned int flags); +static int afs_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags); static int afs_d_delete(const struct dentry *dentry); static void afs_d_iput(struct dentry *dentry, struct inode *inode); static bool afs_lookup_one_filldir(struct dir_context *ctx, const char *name, int nlen, @@ -1084,7 +1085,8 @@ static int afs_d_revalidate_rcu(struct dentry *dentry) * - NOTE! the hit can be a negative hit too, so we can't assume we have an * inode */ -static int afs_d_revalidate(struct dentry *dentry, unsigned int flags) +static int afs_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { struct afs_vnode *vnode, *dir; struct afs_fid fid; diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c index d7d9402ff718..44a8f736eaf8 100644 --- a/fs/afs/dynroot.c +++ b/fs/afs/dynroot.c @@ -247,7 +247,9 @@ const struct inode_operations afs_dynroot_inode_operations = { /* * Dirs in the dynamic root don't need revalidation. */ -static int afs_dynroot_d_revalidate(struct dentry *dentry, unsigned int flags) +static int afs_dynroot_d_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) { return 1; } diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 4a2b39d9a61a..dffc115adae0 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1758,7 +1758,8 @@ static int dir_lease_is_valid(struct inode *dir, struct dentry *dentry, /* * Check if cached dentry can be trusted. */ -static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags) +static int ceph_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { int valid = 0; struct dentry *parent; diff --git a/fs/coda/dir.c b/fs/coda/dir.c index 8450b1bd354b..bb2ecac4a7e7 100644 --- a/fs/coda/dir.c +++ b/fs/coda/dir.c @@ -452,7 +452,8 @@ static int coda_readdir(struct file *coda_file, struct dir_context *ctx) } /* called when a cache lookup succeeds */ -static int coda_dentry_revalidate(struct dentry *de, unsigned int flags) +static int coda_dentry_revalidate(struct dentry *de, const struct qstr *name, + unsigned int flags) { struct inode *inode; struct coda_inode_info *cii; diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 6eae3f12ad50..d543e4648a0f 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -580,7 +580,8 @@ EXPORT_SYMBOL_GPL(fscrypt_fname_siphash); * Validate dentries in encrypted directories to make sure we aren't potentially * caching stale dentries after a key has been added. */ -int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags) +int fscrypt_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { struct dentry *dir; int err; diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c index acaa0825e9bb..56093648d838 100644 --- a/fs/ecryptfs/dentry.c +++ b/fs/ecryptfs/dentry.c @@ -28,7 +28,8 @@ * Returns 1 if valid, 0 otherwise. * */ -static int ecryptfs_d_revalidate(struct dentry *dentry, unsigned int flags) +static int ecryptfs_d_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry); int rc = 1; @@ -37,7 +38,7 @@ static int ecryptfs_d_revalidate(struct dentry *dentry, unsigned int flags) return -ECHILD; if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE) - rc = lower_dentry->d_op->d_revalidate(lower_dentry, flags); + rc = lower_dentry->d_op->d_revalidate(lower_dentry, name, flags); if (d_really_is_positive(dentry)) { struct inode *inode = d_inode(dentry); diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index e0ff9d156f6f..6220046a687b 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -31,7 +31,8 @@ static inline void exfat_d_version_set(struct dentry *dentry, * If it happened, the negative dentry isn't actually negative anymore. So, * drop it. */ -static int exfat_d_revalidate(struct dentry *dentry, unsigned int flags) +static int exfat_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { int ret; diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c index c4d00999a433..73981b0e4ea7 100644 --- a/fs/fat/namei_vfat.c +++ b/fs/fat/namei_vfat.c @@ -53,7 +53,8 @@ static int vfat_revalidate_shortname(struct dentry *dentry) return ret; } -static int vfat_revalidate(struct dentry *dentry, unsigned int flags) +static int vfat_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { if (flags & LOOKUP_RCU) return -ECHILD; @@ -64,7 +65,8 @@ static int vfat_revalidate(struct dentry *dentry, unsigned int flags) return vfat_revalidate_shortname(dentry); } -static int vfat_revalidate_ci(struct dentry *dentry, unsigned int flags) +static int vfat_revalidate_ci(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { if (flags & LOOKUP_RCU) return -ECHILD; diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 35bc174f9ba2..948bbfc1aae4 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -202,7 +202,8 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args, * the lookup once more. If the lookup results in the same inode, * then refresh the attributes, timeouts and mark the dentry valid. */ -static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags) +static int fuse_dentry_revalidate(struct dentry *entry, + const struct qstr *name, unsigned int flags) { struct inode *inode; struct dentry *parent; diff --git a/fs/gfs2/dentry.c b/fs/gfs2/dentry.c index 2e215e8c3c88..3dd93d36aaf2 100644 --- a/fs/gfs2/dentry.c +++ b/fs/gfs2/dentry.c @@ -30,7 +30,8 @@ * Returns: 1 if the dentry is ok, 0 if it isn't */ -static int gfs2_drevalidate(struct dentry *dentry, unsigned int flags) +static int gfs2_drevalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { struct dentry *parent; struct gfs2_sbd *sdp; diff --git a/fs/hfs/sysdep.c b/fs/hfs/sysdep.c index 2875961fdc10..68fb32f4fbb8 100644 --- a/fs/hfs/sysdep.c +++ b/fs/hfs/sysdep.c @@ -13,7 +13,8 @@ /* dentry case-handling: just lowercase everything */ -static int hfs_revalidate_dentry(struct dentry *dentry, unsigned int flags) +static int hfs_revalidate_dentry(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { struct inode *inode; int diff; diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c index 9b030297aa64..0d2b5b54e2d8 100644 --- a/fs/jfs/namei.c +++ b/fs/jfs/namei.c @@ -1573,7 +1573,8 @@ static int jfs_ci_compare(const struct dentry *dentry, return result; } -static int jfs_ci_revalidate(struct dentry *dentry, unsigned int flags) +static int jfs_ci_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { /* * This is not negative dentry. Always valid. diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 5a1a4af9d3d2..820988710ce5 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -1084,7 +1084,8 @@ struct kernfs_node *kernfs_create_empty_dir(struct kernfs_node *parent, return ERR_PTR(rc); } -static int kernfs_dop_revalidate(struct dentry *dentry, unsigned int flags) +static int kernfs_dop_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { struct kernfs_node *kn; struct kernfs_root *root; diff --git a/fs/namei.c b/fs/namei.c index e56ff39a79bc..7631c762217a 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -853,10 +853,12 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry) return false; } -static inline int d_revalidate(struct dentry *dentry, unsigned int flags) +static inline int d_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) { if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) - return dentry->d_op->d_revalidate(dentry, flags); + return dentry->d_op->d_revalidate(dentry, name, flags); else return 1; } @@ -1565,7 +1567,7 @@ static struct dentry *lookup_dcache(const struct qstr *name, { struct dentry *dentry = d_lookup(dir, name); if (dentry) { - int error = d_revalidate(dentry, flags); + int error = d_revalidate(dentry, name, flags); if (unlikely(error <= 0)) { if (!error) d_invalidate(dentry); @@ -1636,19 +1638,19 @@ static struct dentry *lookup_fast(struct nameidata *nd) if (read_seqcount_retry(&parent->d_seq, nd->seq)) return ERR_PTR(-ECHILD); - status = d_revalidate(dentry, nd->flags); + status = d_revalidate(dentry, &nd->last, nd->flags); if (likely(status > 0)) return dentry; if (!try_to_unlazy_next(nd, dentry)) return ERR_PTR(-ECHILD); if (status == -ECHILD) /* we'd been told to redo it in non-rcu mode */ - status = d_revalidate(dentry, nd->flags); + status = d_revalidate(dentry, &nd->last, nd->flags); } else { dentry = __d_lookup(parent, &nd->last); if (unlikely(!dentry)) return NULL; - status = d_revalidate(dentry, nd->flags); + status = d_revalidate(dentry, &nd->last, nd->flags); } if (unlikely(status <= 0)) { if (!status) @@ -1676,7 +1678,7 @@ static struct dentry *__lookup_slow(const struct qstr *name, if (IS_ERR(dentry)) return dentry; if (unlikely(!d_in_lookup(dentry))) { - int error = d_revalidate(dentry, flags); + int error = d_revalidate(dentry, name, flags); if (unlikely(error <= 0)) { if (!error) { d_invalidate(dentry); @@ -3421,7 +3423,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, if (d_in_lookup(dentry)) break; - error = d_revalidate(dentry, nd->flags); + error = d_revalidate(dentry, &nd->last, nd->flags); if (likely(error > 0)) break; if (error) diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 8f3112e71a6a..be162ef6a24e 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1805,7 +1805,8 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags, return ret; } -static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) +static int nfs_lookup_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { return __nfs_lookup_revalidate(dentry, flags, nfs_do_lookup_revalidate); } @@ -1993,7 +1994,8 @@ void nfs_d_prune_case_insensitive_aliases(struct inode *inode) EXPORT_SYMBOL_GPL(nfs_d_prune_case_insensitive_aliases); #if IS_ENABLED(CONFIG_NFS_V4) -static int nfs4_lookup_revalidate(struct dentry *, unsigned int); +static int nfs4_lookup_revalidate(struct dentry *, const struct qstr *name, + unsigned int); const struct dentry_operations nfs4_dentry_operations = { .d_revalidate = nfs4_lookup_revalidate, @@ -2226,7 +2228,8 @@ nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, return nfs_do_lookup_revalidate(dir, dentry, flags); } -static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags) +static int nfs4_lookup_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { return __nfs_lookup_revalidate(dentry, flags, nfs4_do_lookup_revalidate); diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c index 04fc8344063a..277757f4fd2d 100644 --- a/fs/ocfs2/dcache.c +++ b/fs/ocfs2/dcache.c @@ -32,7 +32,9 @@ void ocfs2_dentry_attach_gen(struct dentry *dentry) } -static int ocfs2_dentry_revalidate(struct dentry *dentry, unsigned int flags) +static int ocfs2_dentry_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) { struct inode *inode; int ret = 0; /* if all else fails, just return false */ diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c index 8bbe9486e3a6..435b88007809 100644 --- a/fs/orangefs/dcache.c +++ b/fs/orangefs/dcache.c @@ -94,7 +94,8 @@ static int orangefs_revalidate_lookup(struct dentry *dentry) * * Should return 1 if dentry can still be trusted, else 0. */ -static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags) +static int orangefs_d_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { int ret; unsigned long time = (unsigned long) dentry->d_fsdata; diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 5b069f1a1e44..1233e38d029d 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -77,7 +77,8 @@ static struct dentry *ovl_d_real(struct dentry *dentry, return dentry; } -static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak) +static int ovl_revalidate_real(struct dentry *d, const struct qstr *name, + unsigned int flags, bool weak) { int ret = 1; @@ -88,7 +89,7 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak) if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE) ret = d->d_op->d_weak_revalidate(d, flags); } else if (d->d_flags & DCACHE_OP_REVALIDATE) { - ret = d->d_op->d_revalidate(d, flags); + ret = d->d_op->d_revalidate(d, name, flags); if (!ret) { if (!(flags & LOOKUP_RCU)) d_invalidate(d); @@ -99,6 +100,7 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak) } static int ovl_dentry_revalidate_common(struct dentry *dentry, + const struct qstr *name, unsigned int flags, bool weak) { struct ovl_entry *oe = OVL_E(dentry); @@ -114,22 +116,24 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry, upper = ovl_i_dentry_upper(inode); if (upper) - ret = ovl_revalidate_real(upper, flags, weak); + ret = ovl_revalidate_real(upper, name, flags, weak); for (i = 0; ret > 0 && i < ovl_numlower(oe); i++) - ret = ovl_revalidate_real(lowerstack[i].dentry, flags, weak); + ret = ovl_revalidate_real(lowerstack[i].dentry, name, flags, weak); return ret; } -static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags) +static int ovl_dentry_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { - return ovl_dentry_revalidate_common(dentry, flags, false); + return ovl_dentry_revalidate_common(dentry, name, flags, false); } -static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags) +static int ovl_dentry_weak_revalidate(struct dentry *dentry, + unsigned int flags) { - return ovl_dentry_revalidate_common(dentry, flags, true); + return ovl_dentry_revalidate_common(dentry, NULL, flags, true); } static const struct dentry_operations ovl_dentry_operations = { diff --git a/fs/proc/base.c b/fs/proc/base.c index 05452c3b9872..bdf212c52c8f 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2005,7 +2005,8 @@ void pid_update_inode(struct task_struct *task, struct inode *inode) * performed a setuid(), etc. * */ -static int pid_revalidate(struct dentry *dentry, unsigned int flags) +static int pid_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { struct inode *inode; struct task_struct *task; @@ -2138,7 +2139,8 @@ static int dname_to_vma_addr(struct dentry *dentry, return 0; } -static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags) +static int map_files_d_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { unsigned long vm_start, vm_end; bool exact_vma_exists = false; diff --git a/fs/proc/fd.c b/fs/proc/fd.c index b3140deebbbf..efd604fe8d82 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -136,7 +136,8 @@ static void tid_fd_update_inode(struct task_struct *task, struct inode *inode, security_task_to_inode(task, inode); } -static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) +static int tid_fd_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { struct task_struct *task; struct inode *inode; diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 42ae38ff6e7e..7cb15ab01a5a 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -216,7 +216,8 @@ void proc_free_inum(unsigned int inum) ida_simple_remove(&proc_inum_ida, inum - PROC_DYNAMIC_FIRST); } -static int proc_misc_d_revalidate(struct dentry *dentry, unsigned int flags) +static int proc_misc_d_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { if (flags & LOOKUP_RCU) return -ECHILD; @@ -343,7 +344,8 @@ static const struct file_operations proc_dir_operations = { .iterate_shared = proc_readdir, }; -static int proc_net_d_revalidate(struct dentry *dentry, unsigned int flags) +static int proc_net_d_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { return 0; } diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 5ea42653126e..d067ebff1c74 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -886,7 +886,8 @@ static const struct inode_operations proc_sys_dir_operations = { .getattr = proc_sys_getattr, }; -static int proc_sys_revalidate(struct dentry *dentry, unsigned int flags) +static int proc_sys_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { if (flags & LOOKUP_RCU) return -ECHILD; diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c index 651027967159..2d09e4cdedd1 100644 --- a/fs/reiserfs/xattr.c +++ b/fs/reiserfs/xattr.c @@ -957,7 +957,8 @@ int reiserfs_permission(struct mnt_idmap *idmap, struct inode *inode, return generic_permission(&nop_mnt_idmap, inode, mask); } -static int xattr_hide_revalidate(struct dentry *dentry, unsigned int flags) +static int xattr_hide_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { return -EPERM; } diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c index 30b1e1bfd204..0ced1a98de9f 100644 --- a/fs/smb/client/dir.c +++ b/fs/smb/client/dir.c @@ -714,7 +714,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, } static int -cifs_d_revalidate(struct dentry *direntry, unsigned int flags) +cifs_d_revalidate(struct dentry *direntry, const struct qstr *name, + unsigned int flags) { struct inode *inode; int rc; diff --git a/fs/vboxsf/dir.c b/fs/vboxsf/dir.c index 075f15c43c78..81a03a7331a4 100644 --- a/fs/vboxsf/dir.c +++ b/fs/vboxsf/dir.c @@ -191,7 +191,9 @@ const struct file_operations vboxsf_dir_fops = { * This is called during name resolution/lookup to check if the @dentry in * the cache is still valid. the job is handled by vboxsf_inode_revalidate. */ -static int vboxsf_dentry_revalidate(struct dentry *dentry, unsigned int flags) +static int vboxsf_dentry_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) { if (flags & LOOKUP_RCU) return -ECHILD; diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 6b351e009f59..9362e4ef0bad 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -126,7 +126,7 @@ enum dentry_d_lock_class }; struct dentry_operations { - int (*d_revalidate)(struct dentry *, unsigned int); + int (*d_revalidate)(struct dentry *, const struct qstr *, unsigned int); int (*d_weak_revalidate)(struct dentry *, unsigned int); int (*d_hash)(const struct dentry *, struct qstr *); int (*d_compare)(const struct dentry *, diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index c895b12737a1..d8c68a366a2b 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -353,7 +353,8 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode, bool fscrypt_match_name(const struct fscrypt_name *fname, const u8 *de_name, u32 de_name_len); u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name); -int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags); +int fscrypt_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags); /* bio.c */ bool fscrypt_decrypt_bio(struct bio *bio); @@ -647,6 +648,7 @@ static inline u64 fscrypt_fname_siphash(const struct inode *dir, } static inline int fscrypt_d_revalidate(struct dentry *dentry, + const struct qstr *name, unsigned int flags) { return 1; -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi @ 2023-08-12 2:15 ` Eric Biggers 2023-08-17 7:00 ` kernel test robot 2023-08-17 9:12 ` kernel test robot 2 siblings, 0 replies; 32+ messages in thread From: Eric Biggers @ 2023-08-12 2:15 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi On Fri, Aug 11, 2023 at 08:41:40PM -0400, Gabriel Krisman Bertazi wrote: > diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst > index cb2a97e49872..ddd542c2a722 100644 > --- a/Documentation/filesystems/vfs.rst > +++ b/Documentation/filesystems/vfs.rst > @@ -1251,7 +1251,8 @@ defined: > .. code-block:: c > > struct dentry_operations { > - int (*d_revalidate)(struct dentry *, unsigned int); > + int (*d_revalidate)(struct dentry *, const struct qstr *, > + unsigned int); > int (*d_weak_revalidate)(struct dentry *, unsigned int); > int (*d_hash)(const struct dentry *, struct qstr *); > int (*d_compare)(const struct dentry *, > @@ -1284,6 +1285,14 @@ defined: > they can change and, in d_inode case, even become NULL under > us). > > + Filesystems shouldn't rely on the name under lookup, unless > + there are particular filename encoding semantics to be handled > + during revalidation. Note the name under lookup can change from > + under d_revalidate, so it must be protected with ->d_lock before > + accessing. The exception is when revalidating negative dentries > + for creation, in which case the parent inode prevents it from > + changing. Actually, the "name under lookup" can never change. It's passed as the 'name' argument, newly added by this patch. What this paragraph is actually about is the ->d_name of the dentry being revalidated. The documentation should make it clear when it means ->d_name and when it means name, how they differ from each other, and what the purpose of each is. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi 2023-08-12 2:15 ` Eric Biggers @ 2023-08-17 7:00 ` kernel test robot 2023-08-17 9:12 ` kernel test robot 2 siblings, 0 replies; 32+ messages in thread From: kernel test robot @ 2023-08-17 7:00 UTC (permalink / raw) To: Gabriel Krisman Bertazi, viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, linux-f2fs-devel, oe-kbuild-all Hi Gabriel, kernel test robot noticed the following build warnings: [auto build test WARNING on tytso-ext4/dev] [also build test WARNING on linus/master] [cannot apply to tyhicks-ecryptfs/next ericvh-v9fs/for-next] [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/Gabriel-Krisman-Bertazi/fs-Expose-helper-to-check-if-a-directory-needs-casefolding/20230812-084506 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/20230812004146.30980-5-krisman%40suse.de patch subject: [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks config: mips-randconfig-r002-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171453.3HpCqtib-lkp@intel.com/config) compiler: mipsel-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171453.3HpCqtib-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/202308171453.3HpCqtib-lkp@intel.com/ All warnings (new ones prefixed by >>): >> fs/ecryptfs/dentry.c:33: warning: Function parameter or member 'name' not described in 'ecryptfs_d_revalidate' vim +33 fs/ecryptfs/dentry.c 237fead619984c Michael Halcrow 2006-10-04 17 237fead619984c Michael Halcrow 2006-10-04 18 /** 237fead619984c Michael Halcrow 2006-10-04 19 * ecryptfs_d_revalidate - revalidate an ecryptfs dentry 237fead619984c Michael Halcrow 2006-10-04 20 * @dentry: The ecryptfs dentry 0b728e1911cbe6 Al Viro 2012-06-10 21 * @flags: lookup flags 237fead619984c Michael Halcrow 2006-10-04 22 * 237fead619984c Michael Halcrow 2006-10-04 23 * Called when the VFS needs to revalidate a dentry. This 237fead619984c Michael Halcrow 2006-10-04 24 * is called whenever a name lookup finds a dentry in the 237fead619984c Michael Halcrow 2006-10-04 25 * dcache. Most filesystems leave this as NULL, because all their 237fead619984c Michael Halcrow 2006-10-04 26 * dentries in the dcache are valid. 237fead619984c Michael Halcrow 2006-10-04 27 * 237fead619984c Michael Halcrow 2006-10-04 28 * Returns 1 if valid, 0 otherwise. 237fead619984c Michael Halcrow 2006-10-04 29 * 237fead619984c Michael Halcrow 2006-10-04 30 */ 0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11 31 static int ecryptfs_d_revalidate(struct dentry *dentry, 0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11 32 const struct qstr *name, unsigned int flags) 237fead619984c Michael Halcrow 2006-10-04 @33 { 2edbfbf1c1ab0a Al Viro 2013-09-15 34 struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry); 5556e7e6d30e8e Tyler Hicks 2015-08-05 35 int rc = 1; 237fead619984c Michael Halcrow 2006-10-04 36 0b728e1911cbe6 Al Viro 2012-06-10 37 if (flags & LOOKUP_RCU) 34286d6662308d Nicholas Piggin 2011-01-07 38 return -ECHILD; 34286d6662308d Nicholas Piggin 2011-01-07 39 5556e7e6d30e8e Tyler Hicks 2015-08-05 40 if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE) 0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11 41 rc = lower_dentry->d_op->d_revalidate(lower_dentry, name, flags); 5556e7e6d30e8e Tyler Hicks 2015-08-05 42 2b0143b5c986be David Howells 2015-03-17 43 if (d_really_is_positive(dentry)) { 5556e7e6d30e8e Tyler Hicks 2015-08-05 44 struct inode *inode = d_inode(dentry); ae56fb16337c88 Michael Halcrow 2006-11-16 45 5556e7e6d30e8e Tyler Hicks 2015-08-05 46 fsstack_copy_attr_all(inode, ecryptfs_inode_to_lower(inode)); 5556e7e6d30e8e Tyler Hicks 2015-08-05 47 if (!inode->i_nlink) 5556e7e6d30e8e Tyler Hicks 2015-08-05 48 return 0; ae56fb16337c88 Michael Halcrow 2006-11-16 49 } 237fead619984c Michael Halcrow 2006-10-04 50 return rc; 237fead619984c Michael Halcrow 2006-10-04 51 } 237fead619984c Michael Halcrow 2006-10-04 52 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi 2023-08-12 2:15 ` Eric Biggers 2023-08-17 7:00 ` kernel test robot @ 2023-08-17 9:12 ` kernel test robot 2 siblings, 0 replies; 32+ messages in thread From: kernel test robot @ 2023-08-17 9:12 UTC (permalink / raw) To: Gabriel Krisman Bertazi, viro, brauner, tytso, ebiggers, jaegeuk Cc: Gabriel Krisman Bertazi, llvm, linux-f2fs-devel, oe-kbuild-all, linux-fsdevel, linux-ext4 Hi Gabriel, kernel test robot noticed the following build warnings: [auto build test WARNING on tytso-ext4/dev] [also build test WARNING on linus/master] [cannot apply to tyhicks-ecryptfs/next ericvh-v9fs/for-next viro-vfs/for-next] [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/Gabriel-Krisman-Bertazi/fs-Expose-helper-to-check-if-a-directory-needs-casefolding/20230812-084506 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/20230812004146.30980-5-krisman%40suse.de patch subject: [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks config: x86_64-randconfig-x012-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171740.0u9DuWtr-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171740.0u9DuWtr-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/202308171740.0u9DuWtr-lkp@intel.com/ All warnings (new ones prefixed by >>): >> fs/ecryptfs/dentry.c:33: warning: Function parameter or member 'name' not described in 'ecryptfs_d_revalidate' vim +33 fs/ecryptfs/dentry.c 237fead619984c Michael Halcrow 2006-10-04 17 237fead619984c Michael Halcrow 2006-10-04 18 /** 237fead619984c Michael Halcrow 2006-10-04 19 * ecryptfs_d_revalidate - revalidate an ecryptfs dentry 237fead619984c Michael Halcrow 2006-10-04 20 * @dentry: The ecryptfs dentry 0b728e1911cbe6 Al Viro 2012-06-10 21 * @flags: lookup flags 237fead619984c Michael Halcrow 2006-10-04 22 * 237fead619984c Michael Halcrow 2006-10-04 23 * Called when the VFS needs to revalidate a dentry. This 237fead619984c Michael Halcrow 2006-10-04 24 * is called whenever a name lookup finds a dentry in the 237fead619984c Michael Halcrow 2006-10-04 25 * dcache. Most filesystems leave this as NULL, because all their 237fead619984c Michael Halcrow 2006-10-04 26 * dentries in the dcache are valid. 237fead619984c Michael Halcrow 2006-10-04 27 * 237fead619984c Michael Halcrow 2006-10-04 28 * Returns 1 if valid, 0 otherwise. 237fead619984c Michael Halcrow 2006-10-04 29 * 237fead619984c Michael Halcrow 2006-10-04 30 */ 0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11 31 static int ecryptfs_d_revalidate(struct dentry *dentry, 0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11 32 const struct qstr *name, unsigned int flags) 237fead619984c Michael Halcrow 2006-10-04 @33 { 2edbfbf1c1ab0a Al Viro 2013-09-15 34 struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry); 5556e7e6d30e8e Tyler Hicks 2015-08-05 35 int rc = 1; 237fead619984c Michael Halcrow 2006-10-04 36 0b728e1911cbe6 Al Viro 2012-06-10 37 if (flags & LOOKUP_RCU) 34286d6662308d Nicholas Piggin 2011-01-07 38 return -ECHILD; 34286d6662308d Nicholas Piggin 2011-01-07 39 5556e7e6d30e8e Tyler Hicks 2015-08-05 40 if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE) 0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11 41 rc = lower_dentry->d_op->d_revalidate(lower_dentry, name, flags); 5556e7e6d30e8e Tyler Hicks 2015-08-05 42 2b0143b5c986be David Howells 2015-03-17 43 if (d_really_is_positive(dentry)) { 5556e7e6d30e8e Tyler Hicks 2015-08-05 44 struct inode *inode = d_inode(dentry); ae56fb16337c88 Michael Halcrow 2006-11-16 45 5556e7e6d30e8e Tyler Hicks 2015-08-05 46 fsstack_copy_attr_all(inode, ecryptfs_inode_to_lower(inode)); 5556e7e6d30e8e Tyler Hicks 2015-08-05 47 if (!inode->i_nlink) 5556e7e6d30e8e Tyler Hicks 2015-08-05 48 return 0; ae56fb16337c88 Michael Halcrow 2006-11-16 49 } 237fead619984c Michael Halcrow 2006-10-04 50 return rc; 237fead619984c Michael Halcrow 2006-10-04 51 } 237fead619984c Michael Halcrow 2006-10-04 52 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (3 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 2:17 ` Eric Biggers 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories Gabriel Krisman Bertazi ` (4 subsequent siblings) 9 siblings, 1 reply; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> This flag marks a negative or positive dentry as being created after a case-insensitive lookup operation. It is useful to differentiate dentries this way to detect whether the negative dentry can be trusted during a case-insensitive lookup. Reviewed-by: Theodore Ts'o <tytso@mit.edu> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v4: - Fixup names of functions to reflect flag name change (Eric) Changes since v2: - Rename DCACHE_CASEFOLD_LOOKUP -> DCACHE_CASEFOLDED_NAME (Eric) --- fs/dcache.c | 8 ++++++++ include/linux/dcache.h | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/fs/dcache.c b/fs/dcache.c index 52e6d5fdab6b..269367c1a86c 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1958,6 +1958,14 @@ void d_set_fallthru(struct dentry *dentry) } EXPORT_SYMBOL(d_set_fallthru); +void d_set_casefolded_name(struct dentry *dentry) +{ + spin_lock(&dentry->d_lock); + dentry->d_flags |= DCACHE_CASEFOLDED_NAME; + spin_unlock(&dentry->d_lock); +} +EXPORT_SYMBOL(d_set_casefold_lookup); + static unsigned d_flags_for_inode(struct inode *inode) { unsigned add_flags = DCACHE_REGULAR_TYPE; diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 9362e4ef0bad..ccbb5c4db7ce 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -208,6 +208,7 @@ struct dentry_operations { #define DCACHE_FALLTHRU 0x01000000 /* Fall through to lower layer */ #define DCACHE_NOKEY_NAME 0x02000000 /* Encrypted name encoded without key */ #define DCACHE_OP_REAL 0x04000000 +#define DCACHE_CASEFOLDED_NAME 0x08000000 /* Dentry comes from a casefold directory */ #define DCACHE_PAR_LOOKUP 0x10000000 /* being looked up (with parent locked shared) */ #define DCACHE_DENTRY_CURSOR 0x20000000 @@ -496,6 +497,13 @@ static inline bool d_is_fallthru(const struct dentry *dentry) return dentry->d_flags & DCACHE_FALLTHRU; } +extern void d_set_casefolded_name(struct dentry *dentry); + +static inline bool d_is_casefolded_name(const struct dentry *dentry) +{ + return dentry->d_flags & DCACHE_CASEFOLDED_NAME; +} + extern int sysctl_vfs_cache_pressure; -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag Gabriel Krisman Bertazi @ 2023-08-12 2:17 ` Eric Biggers 2023-08-14 15:03 ` Gabriel Krisman Bertazi 0 siblings, 1 reply; 32+ messages in thread From: Eric Biggers @ 2023-08-12 2:17 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi On Fri, Aug 11, 2023 at 08:41:41PM -0400, Gabriel Krisman Bertazi wrote: > +void d_set_casefolded_name(struct dentry *dentry) > +{ > + spin_lock(&dentry->d_lock); > + dentry->d_flags |= DCACHE_CASEFOLDED_NAME; > + spin_unlock(&dentry->d_lock); > +} > +EXPORT_SYMBOL(d_set_casefold_lookup); s/d_set_casefold_lookup/d_set_casefolded_name/ - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag 2023-08-12 2:17 ` Eric Biggers @ 2023-08-14 15:03 ` Gabriel Krisman Bertazi 0 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-14 15:03 UTC (permalink / raw) To: Eric Biggers Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi Eric Biggers <ebiggers@kernel.org> writes: > On Fri, Aug 11, 2023 at 08:41:41PM -0400, Gabriel Krisman Bertazi wrote: >> +void d_set_casefolded_name(struct dentry *dentry) >> +{ >> + spin_lock(&dentry->d_lock); >> + dentry->d_flags |= DCACHE_CASEFOLDED_NAME; >> + spin_unlock(&dentry->d_lock); >> +} >> +EXPORT_SYMBOL(d_set_casefold_lookup); > > s/d_set_casefold_lookup/d_set_casefolded_name/ My apologies for this error again. It sucks there is no compile-time warning for EXPORT_SYMBOL, but I should have caught it in the past two iterations. Will fix. -- Gabriel Krisman Bertazi _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (4 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 2:41 ` Eric Biggers 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 07/10] libfs: Chain encryption checks after case-insensitive revalidation Gabriel Krisman Bertazi ` (3 subsequent siblings) 9 siblings, 1 reply; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> Introduce a dentry revalidation helper to be used by case-insensitive filesystems to check if it is safe to reuse a negative dentry. A negative dentry is safe to be reused on a case-insensitive lookup if it was created during a case-insensitive lookup and this is not a lookup that will instantiate a dentry. If this is a creation lookup, we also need to make sure the name matches sensitively the name under lookup in order to assure the name preserving semantics. dentry->d_name is only checked by the case-insensitive d_revalidate hook in the LOOKUP_CREATE/LOOKUP_RENAME_TARGET case since, for these cases, d_revalidate is always called with the parent inode at least read-locked, and therefore the name cannot change from under us. d_revalidate is only called in 4 places: lookup_dcache, __lookup_slow, lookup_open and lookup_fast: - lookup_dcache always calls it with zeroed flags, with the exception of when coming from __lookup_hash, which needs the parent locked already, for instance in the open/creation path, which is locked in open_last_lookups. - In __lookup_slow, either the parent inode is read-locked by the caller (lookup_slow), or it is called with no flags (lookup_one*). The read lock suffices to prevent ->d_name modifications, with the exception of one case: __d_unalias, will call __d_move to fix a directory accessible from multiple dentries, which effectively swaps ->d_name while holding only the shared read lock. This happens through this flow: lookup_slow() //LOOKUP_CREATE d_lookup() ->d_lookup() d_splice_alias() __d_unalias() __d_move() Nevertheless, this case is not a problem because negative dentries are not allowed to be moved with __d_move. In addition, d_instantiate shouldn't race with this case because it's callers also acquire the parent inode lock, preventing it from racing with lookup creation, so the dentry cannot become positive and be moved while inside d_revalidate, which would be a problem if a parallel lookup did d_splice_alias. - lookup_open also requires the parent to be locked in the creation case, which is done in open_last_lookups. - lookup_fast will indeed be called with the parent unlocked, but it shouldn't be called with LOOKUP_CREATE. Either it is called in the link_path_walk, where nd->flags doesn't have LOOKUP_CREATE yet or in open_last_lookups. But, in this case, it also never has LOOKUP_CREATE, because it is only called on the !O_CREAT case, which means op->intent doesn't have LOOKUP_CREAT (set in build_open_flags only if O_CREAT is set). Finally, for the LOOKUP_RENAME_TARGET, we are doing a rename, so the parents inodes are also locked. Reviewed-by: Theodore Ts'o <tytso@mit.edu> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v4: - Drop useless inline declaration (eric) - Refactor to drop extra identation (Christian) - Discuss d_instantiate Changes since v3: - Add comment regarding creation (Eric) - Reorder checks to clarify !flags meaning (Eric) - Add commit message explanaton of the inode read lock wrt. __d_move. (Eric) Changes since v2: - Add comments to all rejection cases (Eric) - safeguard against filesystem creating dentries without LOOKUP flags --- fs/libfs.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/fs/libfs.c b/fs/libfs.c index 8d0b64cfd5da..cb98c4721327 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1452,9 +1452,66 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str) return 0; } +static int generic_ci_d_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) +{ + const struct dentry *parent; + const struct inode *dir; + + if (!d_is_negative(dentry)) + return 1; + + parent = READ_ONCE(dentry->d_parent); + dir = READ_ONCE(parent->d_inode); + + if (!dir || !dir_is_casefolded(dir)) + return 1; + + /* + * Negative dentries created prior to turning the directory + * case-insensitive cannot be trusted, since they don't ensure + * any possible case version of the filename doesn't exist. + */ + if (!d_is_casefolded_name(dentry)) + return 0; + + /* + * Filesystems will call into d_revalidate without setting + * LOOKUP_ flags even for file creation (see lookup_one* + * variants). Reject negative dentries in this case, since we + * can't know for sure it won't be used for creation. + */ + if (!flags) + return 0; + + /* + * If the lookup is for creation, then a negative dentry can + * only be reused if it's a case-sensitive match, not just a + * case-insensitive one. This is needed to make the new file be + * created with the name the user specified, preserving case. + */ + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { + /* + * ->d_name won't change from under us in the creation + * path only, since d_revalidate during creation and + * renames is always called with the parent inode + * locked. It isn't the case for all lookup callpaths, + * so ->d_name must not be touched outside + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context. + */ + if (dentry->d_name.len != name->len || + memcmp(dentry->d_name.name, name->name, name->len)) + return 0; + } + + return 1; +} + static const struct dentry_operations generic_ci_dentry_ops = { .d_hash = generic_ci_d_hash, .d_compare = generic_ci_d_compare, + .d_revalidate = generic_ci_d_revalidate, }; #endif -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories Gabriel Krisman Bertazi @ 2023-08-12 2:41 ` Eric Biggers 2023-08-14 14:50 ` Gabriel Krisman Bertazi 0 siblings, 1 reply; 32+ messages in thread From: Eric Biggers @ 2023-08-12 2:41 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote: > + /* > + * Filesystems will call into d_revalidate without setting > + * LOOKUP_ flags even for file creation (see lookup_one* > + * variants). Reject negative dentries in this case, since we > + * can't know for sure it won't be used for creation. > + */ > + if (!flags) > + return 0; > + > + /* > + * If the lookup is for creation, then a negative dentry can > + * only be reused if it's a case-sensitive match, not just a > + * case-insensitive one. This is needed to make the new file be > + * created with the name the user specified, preserving case. > + */ > + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { > + /* > + * ->d_name won't change from under us in the creation > + * path only, since d_revalidate during creation and > + * renames is always called with the parent inode > + * locked. It isn't the case for all lookup callpaths, > + * so ->d_name must not be touched outside > + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context. > + */ > + if (dentry->d_name.len != name->len || > + memcmp(dentry->d_name.name, name->name, name->len)) > + return 0; > + } This is still really confusing to me. Can you consider the below? The code is the same except for the reordering, but the explanation is reworked to be much clearer (IMO). Anything I am misunderstanding? /* * If the lookup is for creation, then a negative dentry can only be * reused if it's a case-sensitive match, not just a case-insensitive * one. This is needed to make the new file be created with the name * the user specified, preserving case. * * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these * cases, ->d_name is stable and can be compared to 'name' without * taking ->d_lock because the caller holds dir->i_rwsem for write. * (This is because the directory lock blocks the dentry from being * concurrently instantiated, and negative dentries are never moved.) * * All other creations actually use flags==0. These come from the edge * case of filesystems calling functions like lookup_one() that do a * lookup without setting the lookup flags at all. Such lookups might * or might not be for creation, and if not don't guarantee stable * ->d_name. Therefore, invalidate all negative dentries when flags==0. */ if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { if (dentry->d_name.len != name->len || memcmp(dentry->d_name.name, name->name, name->len)) return 0; } if (!flags) return 0; _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories 2023-08-12 2:41 ` Eric Biggers @ 2023-08-14 14:50 ` Gabriel Krisman Bertazi 2023-08-14 18:42 ` Eric Biggers 0 siblings, 1 reply; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-14 14:50 UTC (permalink / raw) To: Eric Biggers Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi Eric Biggers <ebiggers@kernel.org> writes: > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote: >> + /* >> + * Filesystems will call into d_revalidate without setting >> + * LOOKUP_ flags even for file creation (see lookup_one* >> + * variants). Reject negative dentries in this case, since we >> + * can't know for sure it won't be used for creation. >> + */ >> + if (!flags) >> + return 0; >> + >> + /* >> + * If the lookup is for creation, then a negative dentry can >> + * only be reused if it's a case-sensitive match, not just a >> + * case-insensitive one. This is needed to make the new file be >> + * created with the name the user specified, preserving case. >> + */ >> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { >> + /* >> + * ->d_name won't change from under us in the creation >> + * path only, since d_revalidate during creation and >> + * renames is always called with the parent inode >> + * locked. It isn't the case for all lookup callpaths, >> + * so ->d_name must not be touched outside >> + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context. >> + */ >> + if (dentry->d_name.len != name->len || >> + memcmp(dentry->d_name.name, name->name, name->len)) >> + return 0; >> + } > > This is still really confusing to me. Can you consider the below? The code is > the same except for the reordering, but the explanation is reworked to be much > clearer (IMO). Anything I am misunderstanding? > > /* > * If the lookup is for creation, then a negative dentry can only be > * reused if it's a case-sensitive match, not just a case-insensitive > * one. This is needed to make the new file be created with the name > * the user specified, preserving case. > * > * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these > * cases, ->d_name is stable and can be compared to 'name' without > * taking ->d_lock because the caller holds dir->i_rwsem for write. > * (This is because the directory lock blocks the dentry from being > * concurrently instantiated, and negative dentries are never moved.) > * > * All other creations actually use flags==0. These come from the edge > * case of filesystems calling functions like lookup_one() that do a > * lookup without setting the lookup flags at all. Such lookups might > * or might not be for creation, and if not don't guarantee stable > * ->d_name. Therefore, invalidate all negative dentries when flags==0. > */ > if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { > if (dentry->d_name.len != name->len || > memcmp(dentry->d_name.name, name->name, name->len)) > return 0; > } > if (!flags) > return 0; I don't see it as particularly better or less confusing than the original. but I also don't mind taking it into the next iteration. -- Gabriel Krisman Bertazi _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories 2023-08-14 14:50 ` Gabriel Krisman Bertazi @ 2023-08-14 18:42 ` Eric Biggers 2023-08-14 19:21 ` Gabriel Krisman Bertazi 0 siblings, 1 reply; 32+ messages in thread From: Eric Biggers @ 2023-08-14 18:42 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote: > Eric Biggers <ebiggers@kernel.org> writes: > > > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote: > >> + /* > >> + * Filesystems will call into d_revalidate without setting > >> + * LOOKUP_ flags even for file creation (see lookup_one* > >> + * variants). Reject negative dentries in this case, since we > >> + * can't know for sure it won't be used for creation. > >> + */ > >> + if (!flags) > >> + return 0; > >> + > >> + /* > >> + * If the lookup is for creation, then a negative dentry can > >> + * only be reused if it's a case-sensitive match, not just a > >> + * case-insensitive one. This is needed to make the new file be > >> + * created with the name the user specified, preserving case. > >> + */ > >> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { > >> + /* > >> + * ->d_name won't change from under us in the creation > >> + * path only, since d_revalidate during creation and > >> + * renames is always called with the parent inode > >> + * locked. It isn't the case for all lookup callpaths, > >> + * so ->d_name must not be touched outside > >> + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context. > >> + */ > >> + if (dentry->d_name.len != name->len || > >> + memcmp(dentry->d_name.name, name->name, name->len)) > >> + return 0; > >> + } > > > > This is still really confusing to me. Can you consider the below? The code is > > the same except for the reordering, but the explanation is reworked to be much > > clearer (IMO). Anything I am misunderstanding? > > > > /* > > * If the lookup is for creation, then a negative dentry can only be > > * reused if it's a case-sensitive match, not just a case-insensitive > > * one. This is needed to make the new file be created with the name > > * the user specified, preserving case. > > * > > * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these > > * cases, ->d_name is stable and can be compared to 'name' without > > * taking ->d_lock because the caller holds dir->i_rwsem for write. > > * (This is because the directory lock blocks the dentry from being > > * concurrently instantiated, and negative dentries are never moved.) > > * > > * All other creations actually use flags==0. These come from the edge > > * case of filesystems calling functions like lookup_one() that do a > > * lookup without setting the lookup flags at all. Such lookups might > > * or might not be for creation, and if not don't guarantee stable > > * ->d_name. Therefore, invalidate all negative dentries when flags==0. > > */ > > if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { > > if (dentry->d_name.len != name->len || > > memcmp(dentry->d_name.name, name->name, name->len)) > > return 0; > > } > > if (!flags) > > return 0; > > I don't see it as particularly better or less confusing than the > original. but I also don't mind taking it into the next iteration. > Your commit message is still much longer and covers some quite different details which seem irrelevant to me. So if you don't see my explanation as being much different, I think we're still not on the same page. I hope that I'm not misunderstanding anything, in which I believe that what I wrote above is a good explanation and your commit message should be substantially simplified. Remember, longer != better. Keep things as simple as possible. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories 2023-08-14 18:42 ` Eric Biggers @ 2023-08-14 19:21 ` Gabriel Krisman Bertazi 2023-08-14 19:26 ` Eric Biggers 0 siblings, 1 reply; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-14 19:21 UTC (permalink / raw) To: Eric Biggers Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi Eric Biggers <ebiggers@kernel.org> writes: > On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote: >> Eric Biggers <ebiggers@kernel.org> writes: >> >> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote: >> >> + /* >> >> + * Filesystems will call into d_revalidate without setting >> >> + * LOOKUP_ flags even for file creation (see lookup_one* >> >> + * variants). Reject negative dentries in this case, since we >> >> + * can't know for sure it won't be used for creation. >> >> + */ >> >> + if (!flags) >> >> + return 0; >> >> + >> >> + /* >> >> + * If the lookup is for creation, then a negative dentry can >> >> + * only be reused if it's a case-sensitive match, not just a >> >> + * case-insensitive one. This is needed to make the new file be >> >> + * created with the name the user specified, preserving case. >> >> + */ >> >> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { >> >> + /* >> >> + * ->d_name won't change from under us in the creation >> >> + * path only, since d_revalidate during creation and >> >> + * renames is always called with the parent inode >> >> + * locked. It isn't the case for all lookup callpaths, >> >> + * so ->d_name must not be touched outside >> >> + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context. >> >> + */ >> >> + if (dentry->d_name.len != name->len || >> >> + memcmp(dentry->d_name.name, name->name, name->len)) >> >> + return 0; >> >> + } >> > >> > This is still really confusing to me. Can you consider the below? The code is >> > the same except for the reordering, but the explanation is reworked to be much >> > clearer (IMO). Anything I am misunderstanding? >> > >> > /* >> > * If the lookup is for creation, then a negative dentry can only be >> > * reused if it's a case-sensitive match, not just a case-insensitive >> > * one. This is needed to make the new file be created with the name >> > * the user specified, preserving case. >> > * >> > * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these >> > * cases, ->d_name is stable and can be compared to 'name' without >> > * taking ->d_lock because the caller holds dir->i_rwsem for write. >> > * (This is because the directory lock blocks the dentry from being >> > * concurrently instantiated, and negative dentries are never moved.) >> > * >> > * All other creations actually use flags==0. These come from the edge >> > * case of filesystems calling functions like lookup_one() that do a >> > * lookup without setting the lookup flags at all. Such lookups might >> > * or might not be for creation, and if not don't guarantee stable >> > * ->d_name. Therefore, invalidate all negative dentries when flags==0. >> > */ >> > if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { >> > if (dentry->d_name.len != name->len || >> > memcmp(dentry->d_name.name, name->name, name->len)) >> > return 0; >> > } >> > if (!flags) >> > return 0; >> >> I don't see it as particularly better or less confusing than the >> original. but I also don't mind taking it into the next iteration. >> > > Your commit message is still much longer and covers some quite different details > which seem irrelevant to me. So if you don't see my explanation as being much > different, I think we're still not on the same page. I hope that I'm not > misunderstanding anything, in which I believe that what I wrote above is a good > explanation and your commit message should be substantially simplified. > Remember, longer != better. Keep things as simple as possible. I think we are on the same page regarding the code. I was under the impression your suggestion was regarding the *code comment* you proposed to replace, and not the commit message. I don't see your code comment to be much different than the original. The commit message has information accumulated on previous discussions, including the conclusions from the locking discussion Viro requested. I'll reword it too for the next iteration to see if I can make it more concise. Thx -- Gabriel Krisman Bertazi _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories 2023-08-14 19:21 ` Gabriel Krisman Bertazi @ 2023-08-14 19:26 ` Eric Biggers 0 siblings, 0 replies; 32+ messages in thread From: Eric Biggers @ 2023-08-14 19:26 UTC (permalink / raw) To: Gabriel Krisman Bertazi Cc: brauner, tytso, linux-f2fs-devel, viro, linux-fsdevel, jaegeuk, linux-ext4, Gabriel Krisman Bertazi On Mon, Aug 14, 2023 at 03:21:33PM -0400, Gabriel Krisman Bertazi wrote: > Eric Biggers <ebiggers@kernel.org> writes: > > > On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote: > >> Eric Biggers <ebiggers@kernel.org> writes: > >> > >> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote: > >> >> + /* > >> >> + * Filesystems will call into d_revalidate without setting > >> >> + * LOOKUP_ flags even for file creation (see lookup_one* > >> >> + * variants). Reject negative dentries in this case, since we > >> >> + * can't know for sure it won't be used for creation. > >> >> + */ > >> >> + if (!flags) > >> >> + return 0; > >> >> + > >> >> + /* > >> >> + * If the lookup is for creation, then a negative dentry can > >> >> + * only be reused if it's a case-sensitive match, not just a > >> >> + * case-insensitive one. This is needed to make the new file be > >> >> + * created with the name the user specified, preserving case. > >> >> + */ > >> >> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { > >> >> + /* > >> >> + * ->d_name won't change from under us in the creation > >> >> + * path only, since d_revalidate during creation and > >> >> + * renames is always called with the parent inode > >> >> + * locked. It isn't the case for all lookup callpaths, > >> >> + * so ->d_name must not be touched outside > >> >> + * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context. > >> >> + */ > >> >> + if (dentry->d_name.len != name->len || > >> >> + memcmp(dentry->d_name.name, name->name, name->len)) > >> >> + return 0; > >> >> + } > >> > > >> > This is still really confusing to me. Can you consider the below? The code is > >> > the same except for the reordering, but the explanation is reworked to be much > >> > clearer (IMO). Anything I am misunderstanding? > >> > > >> > /* > >> > * If the lookup is for creation, then a negative dentry can only be > >> > * reused if it's a case-sensitive match, not just a case-insensitive > >> > * one. This is needed to make the new file be created with the name > >> > * the user specified, preserving case. > >> > * > >> > * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations. In these > >> > * cases, ->d_name is stable and can be compared to 'name' without > >> > * taking ->d_lock because the caller holds dir->i_rwsem for write. > >> > * (This is because the directory lock blocks the dentry from being > >> > * concurrently instantiated, and negative dentries are never moved.) > >> > * > >> > * All other creations actually use flags==0. These come from the edge > >> > * case of filesystems calling functions like lookup_one() that do a > >> > * lookup without setting the lookup flags at all. Such lookups might > >> > * or might not be for creation, and if not don't guarantee stable > >> > * ->d_name. Therefore, invalidate all negative dentries when flags==0. > >> > */ > >> > if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) { > >> > if (dentry->d_name.len != name->len || > >> > memcmp(dentry->d_name.name, name->name, name->len)) > >> > return 0; > >> > } > >> > if (!flags) > >> > return 0; > >> > >> I don't see it as particularly better or less confusing than the > >> original. but I also don't mind taking it into the next iteration. > >> > > > > Your commit message is still much longer and covers some quite different details > > which seem irrelevant to me. So if you don't see my explanation as being much > > different, I think we're still not on the same page. I hope that I'm not > > misunderstanding anything, in which I believe that what I wrote above is a good > > explanation and your commit message should be substantially simplified. > > Remember, longer != better. Keep things as simple as possible. > > I think we are on the same page regarding the code. I was under the > impression your suggestion was regarding the *code comment* you proposed > to replace, and not the commit message. I don't see your code comment > to be much different than the original. > > The commit message has information accumulated on previous discussions, > including the conclusions from the locking discussion Viro requested. > I'll reword it too for the next iteration to see if I can make it more > concise. > Yes, I was talking about the code comment, but the commit message is explaining the same thing so it needs to be consistent (or have the commit message just reference the code). As-is they seem to be in contradiction. - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 07/10] libfs: Chain encryption checks after case-insensitive revalidation 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (5 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 08/10] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops Gabriel Krisman Bertazi ` (2 subsequent siblings) 9 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> Support encrypted dentries in generic_ci_d_revalidate by chaining fscrypt_d_revalidate at the tail of the d_revalidate. This allows filesystem to just call generic_ci_d_revalidate and let it handle any case-insensitive dentry (encrypted or not). Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v2: - Enable negative dentries of encrypted filesystems (Eric) --- fs/libfs.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/fs/libfs.c b/fs/libfs.c index cb98c4721327..efb245118d10 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1452,9 +1452,8 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str) return 0; } -static int generic_ci_d_revalidate(struct dentry *dentry, - const struct qstr *name, - unsigned int flags) +static int ci_d_revalidate(struct dentry *dentry, const struct qstr *name, + unsigned int flags) { const struct dentry *parent; const struct inode *dir; @@ -1508,6 +1507,15 @@ static int generic_ci_d_revalidate(struct dentry *dentry, return 1; } +static int generic_ci_d_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) +{ + if (!ci_d_revalidate(dentry, name, flags)) + return 0; + return fscrypt_d_revalidate(dentry, name, flags); +} + static const struct dentry_operations generic_ci_dentry_ops = { .d_hash = generic_ci_d_hash, .d_compare = generic_ci_d_compare, @@ -1525,7 +1533,7 @@ static const struct dentry_operations generic_encrypted_dentry_ops = { 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, + .d_revalidate = generic_ci_d_revalidate, }; #endif -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 08/10] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (6 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 07/10] libfs: Chain encryption checks after case-insensitive revalidation Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 09/10] ext4: Enable negative dentries on case-insensitive lookup Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 10/10] f2fs: " Gabriel Krisman Bertazi 9 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> Now that casefold needs d_revalidate and calls fscrypt_d_revalidate itself, generic_encrypt_ci_dentry_ops and generic_ci_dentry_ops are now equivalent. Merge them together and simplify the setup code. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- changes since v2: - reword comment for clarity (Eric) --- fs/libfs.c | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/fs/libfs.c b/fs/libfs.c index efb245118d10..6b15a4f0312f 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1516,7 +1516,7 @@ static int generic_ci_d_revalidate(struct dentry *dentry, return fscrypt_d_revalidate(dentry, name, flags); } -static const struct dentry_operations generic_ci_dentry_ops = { +static const struct dentry_operations generic_encrypted_ci_dentry_ops = { .d_hash = generic_ci_d_hash, .d_compare = generic_ci_d_compare, .d_revalidate = generic_ci_d_revalidate, @@ -1529,26 +1529,19 @@ static const struct dentry_operations generic_encrypted_dentry_ops = { }; #endif -#if defined(CONFIG_FS_ENCRYPTION) && IS_ENABLED(CONFIG_UNICODE) -static const struct dentry_operations generic_encrypted_ci_dentry_ops = { - .d_hash = generic_ci_d_hash, - .d_compare = generic_ci_d_compare, - .d_revalidate = generic_ci_d_revalidate, -}; -#endif - /** * 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 - * while the casefolding flag can be toggled on and off on an empty directory, - * 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). + * Casefolded directories need some dentry_operations set, so that the dentries + * contained in them are handled case-insensitively. Note that d_hash and + * d_compare are needed on the parent directory rather than on the dentries in + * it, and while the casefolding flag can be toggled on and off on an empty + * directory, 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. @@ -1557,34 +1550,22 @@ static const struct dentry_operations generic_encrypted_ci_dentry_ops = { * 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 than always installing all operations. + * combination of operations rather than always installing them. */ 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 #if IS_ENABLED(CONFIG_UNICODE) - bool needs_ci_ops = dentry->d_sb->s_encoding; -#endif -#if defined(CONFIG_FS_ENCRYPTION) && IS_ENABLED(CONFIG_UNICODE) - if (needs_encrypt_ops && needs_ci_ops) { + if (dentry->d_sb->s_encoding) { d_set_d_op(dentry, &generic_encrypted_ci_dentry_ops); return; } #endif #ifdef CONFIG_FS_ENCRYPTION - if (needs_encrypt_ops) { + if (dentry->d_flags & DCACHE_NOKEY_NAME) { d_set_d_op(dentry, &generic_encrypted_dentry_ops); return; } #endif -#if IS_ENABLED(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); -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 09/10] ext4: Enable negative dentries on case-insensitive lookup 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (7 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 08/10] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 10/10] f2fs: " Gabriel Krisman Bertazi 9 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> Instead of invalidating negative dentries during case-insensitive lookups, mark them as such and let them be added to the dcache. d_ci_revalidate is able to properly filter them out if necessary based on the dentry casefold flag. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v4: - Use helper to decide if should set dentry flag. Changes since v2: - Move dentry flag set closer to fscrypt code (Eric) --- fs/ext4/namei.c | 35 ++++------------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 0caf6c730ce3..8d33a74bcc95 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1759,6 +1759,10 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir, err = ext4_fname_prepare_lookup(dir, dentry, &fname); generic_set_encrypted_ci_d_ops(dentry); + + if (dir_is_casefolded(dir)) + d_set_casefolded_name(dentry); + if (err == -ENOENT) return NULL; if (err) @@ -1866,16 +1870,6 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi } } -#if IS_ENABLED(CONFIG_UNICODE) - if (!inode && IS_CASEFOLDED(dir)) { - /* Eventually we want to call d_add_ci(dentry, NULL) - * for negative dentries in the encoding case as - * well. For now, prevent the negative dentry - * from being cached. - */ - return NULL; - } -#endif return d_splice_alias(inode, dentry); } @@ -3206,17 +3200,6 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry) ext4_fc_track_unlink(handle, dentry); retval = ext4_mark_inode_dirty(handle, dir); -#if IS_ENABLED(CONFIG_UNICODE) - /* VFS negative dentries are incompatible with Encoding and - * Case-insensitiveness. Eventually we'll want avoid - * invalidating the dentries here, alongside with returning the - * negative dentries at ext4_lookup(), when it is better - * supported by the VFS for the CI case. - */ - if (IS_CASEFOLDED(dir)) - d_invalidate(dentry); -#endif - end_rmdir: brelse(bh); if (handle) @@ -3317,16 +3300,6 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) goto out_trace; retval = __ext4_unlink(dir, &dentry->d_name, d_inode(dentry), dentry); -#if IS_ENABLED(CONFIG_UNICODE) - /* VFS negative dentries are incompatible with Encoding and - * Case-insensitiveness. Eventually we'll want avoid - * invalidating the dentries here, alongside with returning the - * negative dentries at ext4_lookup(), when it is better - * supported by the VFS for the CI case. - */ - if (IS_CASEFOLDED(dir)) - d_invalidate(dentry); -#endif out_trace: trace_ext4_unlink_exit(dentry, retval); -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [f2fs-dev] [PATCH v5 10/10] f2fs: Enable negative dentries on case-insensitive lookup 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi ` (8 preceding siblings ...) 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 09/10] ext4: Enable negative dentries on case-insensitive lookup Gabriel Krisman Bertazi @ 2023-08-12 0:41 ` Gabriel Krisman Bertazi 9 siblings, 0 replies; 32+ messages in thread From: Gabriel Krisman Bertazi @ 2023-08-12 0:41 UTC (permalink / raw) To: viro, brauner, tytso, ebiggers, jaegeuk Cc: linux-fsdevel, Gabriel Krisman Bertazi, linux-ext4, Gabriel Krisman Bertazi, linux-f2fs-devel From: Gabriel Krisman Bertazi <krisman@collabora.com> Instead of invalidating negative dentries during case-insensitive lookups, mark them as such and let them be added to the dcache. d_ci_revalidate is able to properly filter them out if necessary based on the dentry casefold flag. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v4: - Use helper to decide if should set dentry flag. Changes since v2: - Move dentry flag set closer to fscrypt code (Eric) --- fs/f2fs/namei.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c index bee0568888da..8b8fd4cdf62d 100644 --- a/fs/f2fs/namei.c +++ b/fs/f2fs/namei.c @@ -533,6 +533,10 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, err = f2fs_prepare_lookup(dir, dentry, &fname); generic_set_encrypted_ci_d_ops(dentry); + + if (dir_is_casefolded(dir)) + d_set_casefolded_name(dentry); + if (err == -ENOENT) goto out_splice; if (err) @@ -578,17 +582,6 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, goto out_iput; } out_splice: -#if IS_ENABLED(CONFIG_UNICODE) - if (!inode && IS_CASEFOLDED(dir)) { - /* Eventually we want to call d_add_ci(dentry, NULL) - * for negative dentries in the encoding case as - * well. For now, prevent the negative dentry - * from being cached. - */ - trace_f2fs_lookup_end(dir, dentry, ino, err); - return NULL; - } -#endif new = d_splice_alias(inode, dentry); trace_f2fs_lookup_end(dir, !IS_ERR_OR_NULL(new) ? new : dentry, ino, IS_ERR(new) ? PTR_ERR(new) : err); @@ -641,16 +634,6 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry) f2fs_delete_entry(de, page, dir, inode); f2fs_unlock_op(sbi); -#if IS_ENABLED(CONFIG_UNICODE) - /* VFS negative dentries are incompatible with Encoding and - * Case-insensitiveness. Eventually we'll want avoid - * invalidating the dentries here, alongside with returning the - * negative dentries at f2fs_lookup(), when it is better - * supported by the VFS for the CI case. - */ - if (IS_CASEFOLDED(dir)) - d_invalidate(dentry); -#endif if (IS_DIRSYNC(dir)) f2fs_sync_fs(sbi->sb, 1); fail: -- 2.41.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
end of thread, other threads:[~2023-08-17 9:13 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-12 0:41 [f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding Gabriel Krisman Bertazi 2023-08-12 1:59 ` Eric Biggers 2023-08-12 23:06 ` Theodore Ts'o 2023-08-13 0:12 ` Eric Biggers 2023-08-13 3:08 ` Matthew Wilcox 2023-08-13 4:30 ` Eric Biggers 2023-08-14 11:38 ` Theodore Ts'o 2023-08-14 17:22 ` Eric Biggers 2023-08-15 3:59 ` Theodore Ts'o 2023-08-14 15:02 ` Gabriel Krisman Bertazi 2023-08-14 19:14 ` Eric Biggers 2023-08-14 19:26 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 02/10] ecryptfs: Reject casefold directory inodes Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 03/10] 9p: Split ->weak_revalidate from ->revalidate Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi 2023-08-12 2:15 ` Eric Biggers 2023-08-17 7:00 ` kernel test robot 2023-08-17 9:12 ` kernel test robot 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag Gabriel Krisman Bertazi 2023-08-12 2:17 ` Eric Biggers 2023-08-14 15:03 ` Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories Gabriel Krisman Bertazi 2023-08-12 2:41 ` Eric Biggers 2023-08-14 14:50 ` Gabriel Krisman Bertazi 2023-08-14 18:42 ` Eric Biggers 2023-08-14 19:21 ` Gabriel Krisman Bertazi 2023-08-14 19:26 ` Eric Biggers 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 07/10] libfs: Chain encryption checks after case-insensitive revalidation Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 08/10] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 09/10] ext4: Enable negative dentries on case-insensitive lookup Gabriel Krisman Bertazi 2023-08-12 0:41 ` [f2fs-dev] [PATCH v5 10/10] f2fs: " Gabriel Krisman Bertazi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).