From 9419a3191dcb27f24478d288abaab697228d28e6 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Thu, 4 Apr 2019 21:04:13 -0400 Subject: acct_on(): don't mess with freeze protection What happens there is that we are replacing file->path.mnt of a file we'd just opened with a clone and we need the write count contribution to be transferred from original mount to new one. That's it. We do *NOT* want any kind of freeze protection for the duration of switchover. IOW, we should just use __mnt_{want,drop}_write() for that switchover; no need to bother with mnt_{want,drop}_write() there. Tested-by: Amir Goldstein Reported-by: syzbot+2a73a6ea9507b7112141@syzkaller.appspotmail.com Signed-off-by: Al Viro --- fs/internal.h | 2 -- include/linux/mount.h | 2 ++ kernel/acct.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/internal.h b/fs/internal.h index 6a8b71643af4..2e7362837a6e 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -89,9 +89,7 @@ extern int sb_prepare_remount_readonly(struct super_block *); extern void __init mnt_init(void); -extern int __mnt_want_write(struct vfsmount *); extern int __mnt_want_write_file(struct file *); -extern void __mnt_drop_write(struct vfsmount *); extern void __mnt_drop_write_file(struct file *); /* diff --git a/include/linux/mount.h b/include/linux/mount.h index 9197ddbf35fb..bf8cc4108b8f 100644 --- a/include/linux/mount.h +++ b/include/linux/mount.h @@ -87,6 +87,8 @@ extern bool mnt_may_suid(struct vfsmount *mnt); struct path; extern struct vfsmount *clone_private_mount(const struct path *path); +extern int __mnt_want_write(struct vfsmount *); +extern void __mnt_drop_write(struct vfsmount *); struct file_system_type; extern struct vfsmount *fc_mount(struct fs_context *fc); diff --git a/kernel/acct.c b/kernel/acct.c index addf7732fb56..81f9831a7859 100644 --- a/kernel/acct.c +++ b/kernel/acct.c @@ -227,7 +227,7 @@ static int acct_on(struct filename *pathname) filp_close(file, NULL); return PTR_ERR(internal); } - err = mnt_want_write(internal); + err = __mnt_want_write(internal); if (err) { mntput(internal); kfree(acct); @@ -252,7 +252,7 @@ static int acct_on(struct filename *pathname) old = xchg(&ns->bacct, &acct->pin); mutex_unlock(&acct->lock); pin_kill(old); - mnt_drop_write(mnt); + __mnt_drop_write(mnt); mntput(mnt); return 0; } -- cgit v1.2.3-59-g8ed1b From 5467a68cbf6884c9a9d91e2a89140afb1839c835 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Fri, 15 Mar 2019 22:23:19 -0400 Subject: dcache: sort the freeing-without-RCU-delay mess for good. For lockless accesses to dentries we don't have pinned we rely (among other things) upon having an RCU delay between dropping the last reference and actually freeing the memory. On the other hand, for things like pipes and sockets we neither do that kind of lockless access, nor want to deal with the overhead of an RCU delay every time a socket gets closed. So delay was made optional - setting DCACHE_RCUACCESS in ->d_flags made sure it would happen. We tried to avoid setting it unless we knew we need it. Unfortunately, that had led to recurring class of bugs, in which we missed the need to set it. We only really need it for dentries that are created by d_alloc_pseudo(), so let's not bother with trying to be smart - just make having an RCU delay the default. The ones that do *not* get it set the replacement flag (DCACHE_NORCU) and we'd better use that sparingly. d_alloc_pseudo() is the only such user right now. FWIW, the race that finally prompted that switch had been between __lock_parent() of immediate subdirectory of what's currently the root of a disconnected tree (e.g. from open-by-handle in progress) racing with d_splice_alias() elsewhere picking another alias for the same inode, either on outright corrupted fs image, or (in case of open-by-handle on NFS) that subdirectory having been just moved on server. It's not easy to hit, so the sky is not falling, but that's not the first race on similar missed cases and the logics for settinf DCACHE_RCUACCESS has gotten ridiculously convoluted. Cc: stable@vger.kernel.org Signed-off-by: Al Viro --- Documentation/filesystems/porting | 5 +++++ fs/dcache.c | 24 +++++++++++++----------- fs/nsfs.c | 3 +-- include/linux/dcache.h | 2 +- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/Documentation/filesystems/porting b/Documentation/filesystems/porting index cf43bc4dbf31..a60fa516d4cb 100644 --- a/Documentation/filesystems/porting +++ b/Documentation/filesystems/porting @@ -638,3 +638,8 @@ in your dentry operations instead. inode to d_splice_alias() will also do the right thing (equivalent of d_add(dentry, NULL); return NULL;), so that kind of special cases also doesn't need a separate treatment. +-- +[mandatory] + DCACHE_RCUACCESS is gone; having an RCU delay on dentry freeing is the + default. DCACHE_NORCU opts out, and only d_alloc_pseudo() has any + business doing so. diff --git a/fs/dcache.c b/fs/dcache.c index aac41adf4743..c663c602f9ef 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -344,7 +344,7 @@ static void dentry_free(struct dentry *dentry) } } /* if dentry was never visible to RCU, immediate free is OK */ - if (!(dentry->d_flags & DCACHE_RCUACCESS)) + if (dentry->d_flags & DCACHE_NORCU) __d_free(&dentry->d_u.d_rcu); else call_rcu(&dentry->d_u.d_rcu, __d_free); @@ -1701,7 +1701,6 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name) struct dentry *dentry = __d_alloc(parent->d_sb, name); if (!dentry) return NULL; - dentry->d_flags |= DCACHE_RCUACCESS; spin_lock(&parent->d_lock); /* * don't need child lock because it is not subject @@ -1726,7 +1725,7 @@ struct dentry *d_alloc_cursor(struct dentry * parent) { struct dentry *dentry = d_alloc_anon(parent->d_sb); if (dentry) { - dentry->d_flags |= DCACHE_RCUACCESS | DCACHE_DENTRY_CURSOR; + dentry->d_flags |= DCACHE_DENTRY_CURSOR; dentry->d_parent = dget(parent); } return dentry; @@ -1739,10 +1738,17 @@ struct dentry *d_alloc_cursor(struct dentry * parent) * * For a filesystem that just pins its dentries in memory and never * performs lookups at all, return an unhashed IS_ROOT dentry. + * This is used for pipes, sockets et.al. - the stuff that should + * never be anyone's children or parents. Unlike all other + * dentries, these will not have RCU delay between dropping the + * last reference and freeing them. */ struct dentry *d_alloc_pseudo(struct super_block *sb, const struct qstr *name) { - return __d_alloc(sb, name); + struct dentry *dentry = __d_alloc(sb, name); + if (likely(dentry)) + dentry->d_flags |= DCACHE_NORCU; + return dentry; } EXPORT_SYMBOL(d_alloc_pseudo); @@ -1911,12 +1917,10 @@ struct dentry *d_make_root(struct inode *root_inode) if (root_inode) { res = d_alloc_anon(root_inode->i_sb); - if (res) { - res->d_flags |= DCACHE_RCUACCESS; + if (res) d_instantiate(res, root_inode); - } else { + else iput(root_inode); - } } return res; } @@ -2781,9 +2785,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target, copy_name(dentry, target); target->d_hash.pprev = NULL; dentry->d_parent->d_lockref.count++; - if (dentry == old_parent) - dentry->d_flags |= DCACHE_RCUACCESS; - else + if (dentry != old_parent) /* wasn't IS_ROOT */ WARN_ON(!--old_parent->d_lockref.count); } else { target->d_parent = old_parent; diff --git a/fs/nsfs.c b/fs/nsfs.c index 60702d677bd4..30d150a4f0c6 100644 --- a/fs/nsfs.c +++ b/fs/nsfs.c @@ -85,13 +85,12 @@ slow: inode->i_fop = &ns_file_operations; inode->i_private = ns; - dentry = d_alloc_pseudo(mnt->mnt_sb, &empty_name); + dentry = d_alloc_anon(mnt->mnt_sb); if (!dentry) { iput(inode); return ERR_PTR(-ENOMEM); } d_instantiate(dentry, inode); - dentry->d_flags |= DCACHE_RCUACCESS; dentry->d_fsdata = (void *)ns->ops; d = atomic_long_cmpxchg(&ns->stashed, 0, (unsigned long)dentry); if (d) { diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 60996e64c579..6e1e8e6602c6 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -176,7 +176,6 @@ struct dentry_operations { * typically using d_splice_alias. */ #define DCACHE_REFERENCED 0x00000040 /* Recently used, don't discard. */ -#define DCACHE_RCUACCESS 0x00000080 /* Entry has ever been RCU-visible */ #define DCACHE_CANT_MOUNT 0x00000100 #define DCACHE_GENOCIDE 0x00000200 @@ -217,6 +216,7 @@ struct dentry_operations { #define DCACHE_PAR_LOOKUP 0x10000000 /* being looked up (with parent locked shared) */ #define DCACHE_DENTRY_CURSOR 0x20000000 +#define DCACHE_NORCU 0x40000000 /* No RCU delay for freeing */ extern seqlock_t rename_lock; -- cgit v1.2.3-59-g8ed1b From ce285c267a003acbf607f3540ff71287f82e5282 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 2 Apr 2019 15:17:34 -0400 Subject: autofs: fix use-after-free in lockless ->d_manage() autofs_d_release() can overlap with lockless ->d_manage(), ending up with autofs_dentry_ino() freed under the latter. Make freeing autofs_info instances RCU-delayed... Signed-off-by: Al Viro --- fs/autofs/autofs_i.h | 1 + fs/autofs/inode.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h index 70c132acdab1..e1091312abe1 100644 --- a/fs/autofs/autofs_i.h +++ b/fs/autofs/autofs_i.h @@ -71,6 +71,7 @@ struct autofs_info { kuid_t uid; kgid_t gid; + struct rcu_head rcu; }; #define AUTOFS_INF_EXPIRING (1<<0) /* dentry in the process of expiring */ diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c index 80597b88718b..fb0225f21c12 100644 --- a/fs/autofs/inode.c +++ b/fs/autofs/inode.c @@ -36,7 +36,7 @@ void autofs_clean_ino(struct autofs_info *ino) void autofs_free_ino(struct autofs_info *ino) { - kfree(ino); + kfree_rcu(ino, rcu); } void autofs_kill_sb(struct super_block *sb) -- cgit v1.2.3-59-g8ed1b