Skip to content

Commit 7515631

Browse files
Al Virobwhacks
authored andcommitted
fix mntput/mntput race
commit 9ea0a46 upstream. mntput_no_expire() does the calculation of total refcount under mount_lock; unfortunately, the decrement (as well as all increments) are done outside of it, leading to false positives in the "are we dropping the last reference" test. Consider the following situation: * mnt is a lazy-umounted mount, kept alive by two opened files. One of those files gets closed. Total refcount of mnt is 2. On CPU 42 mntput(mnt) (called from __fput()) drops one reference, decrementing component * After it has looked at component #0, the process on CPU 0 does mntget(), incrementing component #0, gets preempted and gets to run again - on CPU 69. There it does mntput(), which drops the reference (component #69) and proceeds to spin on mount_lock. * On CPU 42 our first mntput() finishes counting. It observes the decrement of component #69, but not the increment of component #0. As the result, the total it gets is not 1 as it should've been - it's 0. At which point we decide that vfsmount needs to be killed and proceed to free it and shut the filesystem down. However, there's still another opened file on that filesystem, with reference to (now freed) vfsmount, etc. and we are screwed. It's not a wide race, but it can be reproduced with artificial slowdown of the mnt_get_count() loop, and it should be easier to hit on SMP KVM setups. Fix consists of moving the refcount decrement under mount_lock; the tricky part is that we want (and can) keep the fast case (i.e. mount that still has non-NULL ->mnt_ns) entirely out of mount_lock. All places that zero mnt->mnt_ns are dropping some reference to mnt and they call synchronize_rcu() before that mntput(). IOW, if mntput() observes (under rcu_read_lock()) a non-NULL ->mnt_ns, it is guaranteed that there is another reference yet to be dropped. Reported-by: Jann Horn <jannh@google.com> Tested-by: Jann Horn <jannh@google.com> Fixes: 48a066e ("RCU'd vsfmounts") Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> [bwh: Backported to 3.16: Use ACCESS_ONCE() instead of READ_ONCE()] Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
1 parent 074c5db commit 7515631

File tree

1 file changed

+12
-2
lines changed

1 file changed

+12
-2
lines changed

fs/namespace.c

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -969,12 +969,22 @@ static void mntput_no_expire(struct mount *mnt)
969969
{
970970
put_again:
971971
rcu_read_lock();
972-
mnt_add_count(mnt, -1);
973-
if (likely(mnt->mnt_ns)) { /* shouldn't be the last one */
972+
if (likely(ACCESS_ONCE(mnt->mnt_ns))) {
973+
/*
974+
* Since we don't do lock_mount_hash() here,
975+
* ->mnt_ns can change under us. However, if it's
976+
* non-NULL, then there's a reference that won't
977+
* be dropped until after an RCU delay done after
978+
* turning ->mnt_ns NULL. So if we observe it
979+
* non-NULL under rcu_read_lock(), the reference
980+
* we are dropping is not the final one.
981+
*/
982+
mnt_add_count(mnt, -1);
974983
rcu_read_unlock();
975984
return;
976985
}
977986
lock_mount_hash();
987+
mnt_add_count(mnt, -1);
978988
if (mnt_get_count(mnt)) {
979989
rcu_read_unlock();
980990
unlock_mount_hash();

0 commit comments

Comments
 (0)