| Message ID | 20250130043707.GT1977892@ZenIV (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [git,pull] d_revalidate pile (v2) | expand |
On Wed, 29 Jan 2025 at 20:37, Al Viro <viro@zeniv.linux.org.uk> wrote: > > ->d_revalidate() series, along with ->d_iname preliminary work. > One trivial conflict in fs/afs/dir.c - afs_do_lookup_one() has lost > one argument in mainline and switched another from dentry to qstr > in this series. Actually, I had a conflict in fs/fuse/dir.c, and it was less trivial. The d_revalidate() change means that the stable name passed in might come from the path lookup, which means that it isn't NUL-terminated. So the code that did args->in_numargs = 1; args->in_args[0].size = name->len + 1; args->in_args[0].value = name->name; in fuse_lookup_init() is no longer valid for revalidate, and instead you made it do the NUL termination as the next arg: args->in_numargs = 2; args->in_args[0].size = name->len; args->in_args[0].value = name->name; args->in_args[1].size = 1; args->in_args[1].value = ""; Fine, no problem. Except it clashes with commit 7ccd86ba3a48 ("fuse: make args->in_args[0] to be always the header"), which made in_args[0] be that empty case, and moved in_args[0] up to be arg[1]. So my resolution continues on that, and ends up with three in_args, like this: args->in_numargs = 3; fuse_set_zero_arg0(args); args->in_args[1].size = name->len; args->in_args[1].value = name->name; args->in_args[2].size = 1; args->in_args[2].value = ""; which looks straightforward enough, but I have not tested this AT ALL. Miklos, can you please check and confirm that my resolution is ok? It *looks* trivial, but there may be some reason why it causes issues. I don't know the fuse code enough to really be able to tell what implications this has (if there are people adding other args afterwards, maybe we now have too many? Things like that) Linus
The pull request you sent on Thu, 30 Jan 2025 04:37:07 +0000:
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-revalidate
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/d3d90cc2891c9cf4ecba7b85c0af716ab755c7e5
Thank you!
On Thu, 30 Jan 2025 at 18:25, Linus Torvalds <torvalds@linux-foundation.org> wrote: > So my resolution continues on that, and ends up with three in_args, like this: > > args->in_numargs = 3; > fuse_set_zero_arg0(args); > args->in_args[1].size = name->len; > args->in_args[1].value = name->name; > args->in_args[2].size = 1; > args->in_args[2].value = ""; > > which looks straightforward enough, but I have not tested this AT ALL. Yes, this works fine. Thanks, Miklos
On Thu, Jan 30, 2025 at 09:24:34AM -0800, Linus Torvalds wrote: > On Wed, 29 Jan 2025 at 20:37, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > ->d_revalidate() series, along with ->d_iname preliminary work. > > One trivial conflict in fs/afs/dir.c - afs_do_lookup_one() has lost > > one argument in mainline and switched another from dentry to qstr > > in this series. > > Actually, I had a conflict in fs/fuse/dir.c, and it was less trivial. > > The d_revalidate() change means that the stable name passed in might > come from the path lookup, which means that it isn't NUL-terminated. > > So the code that did > > args->in_numargs = 1; > args->in_args[0].size = name->len + 1; > args->in_args[0].value = name->name; > > in fuse_lookup_init() is no longer valid for revalidate, and instead > you made it do the NUL termination as the next arg: > > args->in_numargs = 2; > args->in_args[0].size = name->len; > args->in_args[0].value = name->name; > args->in_args[1].size = 1; > args->in_args[1].value = ""; > > Fine, no problem. Except it clashes with commit 7ccd86ba3a48 ("fuse: > make args->in_args[0] to be always the header"), which made in_args[0] > be that empty case, and moved in_args[0] up to be arg[1]. > > So my resolution continues on that, and ends up with three in_args, like this: > > args->in_numargs = 3; > fuse_set_zero_arg0(args); > args->in_args[1].size = name->len; > args->in_args[1].value = name->name; > args->in_args[2].size = 1; > args->in_args[2].value = ""; *nod* My apologies - that did show up in -next (obviously), with the same resolution you went for, everyone nodded and agreed that it was obviously the right fix. Should've mentioned that when updating pull request message (that wasn't an issue in the first variant, since that had been prior to fuse merge). Sorry about missing that...
->d_revalidate() series, along with ->d_iname preliminary work. One trivial conflict in fs/afs/dir.c - afs_do_lookup_one() has lost one argument in mainline and switched another from dentry to qstr in this series. Change since the previous variant: made external_name.name word-aligned and explicitly documented the layout constraints for struct external_name. The following changes since commit 40384c840ea1944d7c5a392e8975ed088ecf0b37: Linux 6.13-rc1 (2024-12-01 14:28:56 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-revalidate for you to fetch changes up to 30d61efe118cad1a73ad2ad66a3298e4abdf9f41: 9p: fix ->rename_sem exclusion (2025-01-27 19:25:24 -0500) ---------------------------------------------------------------- Provide stable parent and name to ->d_revalidate() instances Most of the filesystem methods where we care about dentry name and parent have their stability guaranteed by the callers; ->d_revalidate() is the major exception. It's easy enough for callers to supply stable values for expected name and expected parent of the dentry being validated. That kills quite a bit of boilerplate in ->d_revalidate() instances, along with a bunch of races where they used to access ->d_name without sufficient precautions. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> ---------------------------------------------------------------- Al Viro (20): make sure that DNAME_INLINE_LEN is a multiple of word size dcache: back inline names with a struct-wrapped array of unsigned long make take_dentry_name_snapshot() lockless dissolve external_name.u into separate members ext4 fast_commit: make use of name_snapshot primitives generic_ci_d_compare(): use shortname_storage Pass parent directory inode and expected name to ->d_revalidate() afs_d_revalidate(): use stable name and parent inode passed by caller ceph_d_revalidate(): use stable parent inode passed by caller ceph_d_revalidate(): propagate stable name down into request encoding fscrypt_d_revalidate(): use stable parent inode passed by caller exfat_d_revalidate(): use stable parent inode passed by caller vfat_revalidate{,_ci}(): use stable parent inode passed by caller fuse_dentry_revalidate(): use stable parent inode and name passed by caller gfs2_drevalidate(): use stable parent inode and name passed by caller nfs{,4}_lookup_validate(): use stable parent inode passed by caller nfs: fix ->d_revalidate() UAF on ->d_name accesses ocfs2_dentry_revalidate(): use stable parent inode and name passed by caller orangefs_d_revalidate(): use stable parent inode and name passed by caller 9p: fix ->rename_sem exclusion Documentation/filesystems/locking.rst | 7 +- Documentation/filesystems/porting.rst | 16 +++++ Documentation/filesystems/vfs.rst | 24 ++++++- fs/9p/v9fs.h | 2 +- fs/9p/vfs_dentry.c | 26 ++++++- fs/afs/dir.c | 40 ++++------- fs/ceph/dir.c | 25 ++----- fs/ceph/mds_client.c | 9 ++- fs/ceph/mds_client.h | 2 + fs/coda/dir.c | 3 +- fs/crypto/fname.c | 22 ++---- fs/dcache.c | 103 ++++++++++++++++----------- fs/ecryptfs/dentry.c | 18 +++-- fs/exfat/namei.c | 11 +-- fs/ext4/fast_commit.c | 29 ++------ fs/ext4/fast_commit.h | 3 +- fs/fat/namei_vfat.c | 19 +++-- fs/fuse/dir.c | 20 +++--- fs/gfs2/dentry.c | 31 ++++---- fs/hfs/sysdep.c | 3 +- fs/jfs/namei.c | 3 +- fs/kernfs/dir.c | 3 +- fs/libfs.c | 15 ++-- fs/namei.c | 18 ++--- fs/nfs/dir.c | 62 +++++++--------- fs/nfs/namespace.c | 2 +- fs/nfs/nfs3proc.c | 5 +- fs/nfs/nfs4proc.c | 20 +++--- fs/nfs/proc.c | 6 +- fs/ocfs2/dcache.c | 14 ++-- fs/orangefs/dcache.c | 22 +++--- fs/overlayfs/super.c | 22 +++++- fs/proc/base.c | 6 +- fs/proc/fd.c | 3 +- fs/proc/generic.c | 6 +- fs/proc/proc_sysctl.c | 3 +- fs/smb/client/dir.c | 3 +- fs/tracefs/inode.c | 3 +- fs/vboxsf/dir.c | 3 +- include/linux/dcache.h | 23 ++++-- include/linux/fscrypt.h | 7 +- include/linux/nfs_xdr.h | 2 +- tools/testing/selftests/bpf/progs/find_vma.c | 2 +- 43 files changed, 359 insertions(+), 307 deletions(-)