[PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[Lukas Czerner]

Currently when someone calls __mnt_make_shortterm() and the mnt_longterm
is already zero, it will come negative which is exactly opposite of what
the function should to. So fix this by decrementing mnt_longterm only in
case that it's not zero. Note that mnt_longterm should not be touched
directly, but rather via __mnt_make_shortterm() and mnt_make_shortterm()
functions - the former does not have this problem.

Moreover this will fix very nasty bug which might cause file systems not
being properly cleaned up while unmounting. Specifically it might cause
deactivate_super() not being called at all, hence the file system would
be still up and running even after successful unmount without user even
noticing it.

The following scenario leads to a bug. While mounting propagate_mnt()
would walk through propagation tree cloning the mount for every every
mount the root propagates to. Even for mounts which parents are not
parents of the directory you're trying to mount to. So it will bump up
the s_active counter.

However before the propagate_mnt() is left it will clean the unneeded
cloned mounts (those which parent root is not parent of your new mount
point), but it will _NOT_ decrement the s_active because umount_tree()
would decrement mnt_longterm causing it to underflow, hence
mntput_no_expire() would bail out before actually calling mntfree() and
releasing the super block (decreasing s_active). At this point super
block will never get released.

User can not directly observe this (aside from seeing running file system
processes even after unmount), but it can be easily reproduced with loop
device.

mount --bind /tmp/one /tmp/two
losetup /dev/loop0 /mnt/test/file0
mount /dev/loop0 /mnt/test1
umount /mnt/test1
losetup -d /dev/loop0
^^^^^^^^^^^^^^^^^^^^^
This will fail because the device is still busy (super block is still
active and s_active is non zero).

This commit fixes this issue.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Milan Broz <mbroz@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index e608199..e72fec7 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -632,7 +632,7 @@ static inline void __mnt_make_longterm(struct mount *mnt)
 static inline void __mnt_make_shortterm(struct mount *mnt)
 {
 #ifdef CONFIG_SMP
-	atomic_dec(&mnt->mnt_longterm);
+	atomic_add_unless(&mnt->mnt_longterm, -1, 0);
 #endif
 }
 
-- 
1.7.7.6

[PATCH 2/2] vfs: Do not copy_tree() unnecessarily while mounting

[Lukas Czerner]

While mounting propagate_mnt() would walk through propagation tree
cloning the mount for every every mount the root propagates to.
Even for mounts which parents are not parents of the directory you're
trying to mount to.

Then before the propagate_mnt() is left it will clean the unneeded
cloned mounts (those which parent root is not parent of your new mount
point).

I think this whole operation is unnecessary and can be avoided by
skipping the copy_tree() if the propagated mount root is not
subdirectory of the directory you're mounting to. Such cloned mount
would be cleaned up later anyway.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/pnode.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/pnode.c b/fs/pnode.c
index ab5fa9e..3ebf63d 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -237,22 +237,22 @@ int propagate_mnt(struct mount *dest_mnt, struct dentry *dest_dentry,
 
 		source =  get_source(m, prev_dest_mnt, prev_src_mnt, &type);
 
-		if (!(child = copy_tree(source, source->mnt.mnt_root, type))) {
-			ret = -ENOMEM;
-			list_splice(tree_list, tmp_list.prev);
-			goto out;
-		}
-
 		if (is_subdir(dest_dentry, m->mnt.mnt_root)) {
+			if (!(child = copy_tree(source,
+						source->mnt.mnt_root, type))) {
+				ret = -ENOMEM;
+				list_splice(tree_list, tmp_list.prev);
+				goto out;
+			}
+
 			mnt_set_mountpoint(m, dest_dentry, child);
 			list_add_tail(&child->mnt_hash, tree_list);
-		} else {
+		} else
 			/*
 			 * This can happen if the parent mount was bind mounted
 			 * on some subdirectory of a shared/slave mount.
 			 */
-			list_add_tail(&child->mnt_hash, &tmp_list);
-		}
+			continue;
 		prev_dest_mnt = m;
 		prev_src_mnt  = child;
 	}
-- 
1.7.7.6

Re: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[Lukáš Czerner]

Anyone can take a look at this ? It's pretty pressing I would say
since this bug will not only disallow to unattach the loop devices
but also make it unable to actually unmount the file system!

You can reproduce it like this:

mkdir -p /tmp/namespace/root/tmp
mkdir -p /tmp/namespace/private
mount --make-rshared /
mount --rbind /tmp/namespace/root/tmp /tmp/namespace/private/

losetup /dev/loop0 /aaa
mount /dev/loop0 /mnt/tst
umount /mnt/tst

I probably need to update description as I forgot about the
--make-shared thing though...sorry about that. Will resend patch
with updated description right away.

Thanks!
-Lukas

On Fri, 25 May 2012, Lukas Czerner wrote:

> Date: Fri, 25 May 2012 17:29:35 +0200
> From: Lukas Czerner <lczerner@redhat.com>
> To: linux-kernel@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org, mbroz@redhat.com,
>     Lukas Czerner <lczerner@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>
> Subject: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative
> 
> Currently when someone calls __mnt_make_shortterm() and the mnt_longterm
> is already zero, it will come negative which is exactly opposite of what
> the function should to. So fix this by decrementing mnt_longterm only in
> case that it's not zero. Note that mnt_longterm should not be touched
> directly, but rather via __mnt_make_shortterm() and mnt_make_shortterm()
> functions - the former does not have this problem.
> 
> Moreover this will fix very nasty bug which might cause file systems not
> being properly cleaned up while unmounting. Specifically it might cause
> deactivate_super() not being called at all, hence the file system would
> be still up and running even after successful unmount without user even
> noticing it.
> 
> The following scenario leads to a bug. While mounting propagate_mnt()
> would walk through propagation tree cloning the mount for every every
> mount the root propagates to. Even for mounts which parents are not
> parents of the directory you're trying to mount to. So it will bump up
> the s_active counter.
> 
> However before the propagate_mnt() is left it will clean the unneeded
> cloned mounts (those which parent root is not parent of your new mount
> point), but it will _NOT_ decrement the s_active because umount_tree()
> would decrement mnt_longterm causing it to underflow, hence
> mntput_no_expire() would bail out before actually calling mntfree() and
> releasing the super block (decreasing s_active). At this point super
> block will never get released.
> 
> User can not directly observe this (aside from seeing running file system
> processes even after unmount), but it can be easily reproduced with loop
> device.
> 
> mount --bind /tmp/one /tmp/two
> losetup /dev/loop0 /mnt/test/file0
> mount /dev/loop0 /mnt/test1
> umount /mnt/test1
> losetup -d /dev/loop0
> ^^^^^^^^^^^^^^^^^^^^^
> This will fail because the device is still busy (super block is still
> active and s_active is non zero).
> 
> This commit fixes this issue.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Milan Broz <mbroz@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/namespace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index e608199..e72fec7 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -632,7 +632,7 @@ static inline void __mnt_make_longterm(struct mount *mnt)
>  static inline void __mnt_make_shortterm(struct mount *mnt)
>  {
>  #ifdef CONFIG_SMP
> -	atomic_dec(&mnt->mnt_longterm);
> +	atomic_add_unless(&mnt->mnt_longterm, -1, 0);
>  #endif
>  }
>  
> 

Re: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[Al Viro]

On Tue, May 29, 2012 at 12:56:44PM +0200, Luk?? Czerner wrote:
> 
> Anyone can take a look at this ? It's pretty pressing I would say
> since this bug will not only disallow to unattach the loop devices
> but also make it unable to actually unmount the file system!

I agree that it's a bug, but I don't believe that this is a proper
fix.  We can do it much simpler.  Look: making these guys longterm
happens exactly at the same points where we set their ->mnt_ns non-NULL.
Remember that "longterm" == "internal kernel mount, mount in some
namespace, someone's root or pwd".  So the problem is that umount_tree()
is that umount_tree() blindly assumes that it's killing a longterm
ref, even though it may be called when dissolving a tree that never
made it into anyone's namespace.  Fine, so let's just have that
sucker do
                if (p->mnt_ns)
                        __mnt_make_shortterm(p);
                p->mnt_ns = NULL;
instead and that's all it takes.  Alternatively, we can just turn the
code in mntput_no_expire() into
	if (likely(mnt->mnt_ns || atomic_read(...))
and get rid of longterm/shortterm logics around ->mnt_ns completely.
Short-term, pardon the bad pun, I'm going with the first variant; it's
less intrusive.  Longer term I think we want to get rid of playing with
mnt_longterm in there and just check ->mnt_ns as well when deciding to
go for the fast path in mntput_no_expire().

Fixed in my tree, will push in a couple of hours.  And that'll need to
go into -stable as well.  Thanks for catching that sucker...

Re: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[Al Viro]

On Wed, May 30, 2012 at 03:01:34AM +0100, Al Viro wrote:

> Fixed in my tree, will push in a couple of hours.  And that'll need to
> go into -stable as well.  Thanks for catching that sucker...

OK, pushed into for-next, along with a bunch of other misc stuff; _NOT_
all that will be there.  If it survives a bit of beating in -next, to
Linus it goes.

Next pile to go in there is Miklos' stuff.

Re: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[Al Viro]

On Wed, May 30, 2012 at 03:01:34AM +0100, Al Viro wrote:
> instead and that's all it takes.  Alternatively, we can just turn the
> code in mntput_no_expire() into
> 	if (likely(mnt->mnt_ns || atomic_read(...))
> and get rid of longterm/shortterm logics around ->mnt_ns completely.
> Short-term, pardon the bad pun, I'm going with the first variant; it's
> less intrusive.  Longer term I think we want to get rid of playing with
> mnt_longterm in there and just check ->mnt_ns as well when deciding to
> go for the fast path in mntput_no_expire().

Probably not, actually - mnt_make_shortterm() is called often enough (any
chdir moving you to another fs) and practically always it's done with
longterm reference from being mounted remaining there.  We want that to
be fast path.  And that's exactly what we have with the current logics;
we don't grab vfsmount_lock unless ->mnt_longterm was about to reach 0.
We need to make sure that transition to "no longterm references left"
would happen only under vfsmount_lock.  As it is (with the fix in 3.5-rc
and -stable) we have this:
	mnt->mnt_longterm == number of references to mnt in some
fs_struct->{root,pwd}.mnt + (is it a kernel-internal vfsmount ? 1 : 0) +
(mnt->mnt_ns != NULL ? 1 : 0)
and getting rid of the third part would make life consider more painful
for chdir et.al. - we can use atomic_add_unless() for lockless path
when dropping a pwd/root reference with the current scheme and get away
with that in almost all cases.  Doing an equivalent with separate checks
for mnt->mnt_ns != NULL or atomic_read(&mnt->mnt_longterm) > 0 would be
hard and it wouldn't have won us anything.

IOW, I'm an idiot.  Especially since there is a much simpler solution:
	* lose ->mnt_longterm manipulations for root/pwd
	* lose ->mnt_longterm manipulations for places where we assign
->mnt_ns now
	* turn that check on the fast path of mntput_no_expire() into
	if (likely(mnt->mnt_ns))
	* turn manipulations with ->mnt_longterm in kern_mount_data/kern_umount
into setting ->mnt_ns to something distinct (ERR_PTR(-EINVAL)) and clearing it.
	* lose ->mnt_longterm completely, along with the helpers for
modifying it.
	* new helper - is_mounted(mnt); checks if ->mnt_ns points to some
mnt_namespace (i.e. what used to be ->mnt_ns != NULL; now !IS_ERR_OR_NULL())
	* switch prepend_path() to use of that helper instead of direct
check of ->mnt_ns != NULl.

Somebody with pwd on lazy-umounted vfsmount will have costlier mntput() of
that pwd.  Everything else loses a bunch of atomic operations on fairly
hot paths, not to mention the loss of extra atomic_t in struct vfsmount
on SMP kernels.  BTW, looking at that thing... I really wonder if int is
enough for mnt_count.  On boxen with a lot of RAM it might be possible to
overflow; you are probably fucked anyway if that happens (things that
hold struct vfsmount * are not particulary small), but still...  I suspect
that it, and probably mnt_writers as well, should be long.  Anyway, that's
a separate story; how about the following for ->mnt_longterm?

diff --git a/fs/dcache.c b/fs/dcache.c
index 4046904..44acb5b 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2622,7 +2622,7 @@ global_root:
 	if (!slash)
 		error = prepend(buffer, buflen, "/", 1);
 	if (!error)
-		error = real_mount(vfsmnt)->mnt_ns ? 1 : 2;
+		error = is_mounted(vfsmnt) ? 1 : 2;
 	goto out;
 }
 
diff --git a/fs/fs_struct.c b/fs/fs_struct.c
index e159e68..5df4775 100644
--- a/fs/fs_struct.c
+++ b/fs/fs_struct.c
@@ -6,18 +6,6 @@
 #include <linux/fs_struct.h>
 #include "internal.h"
 
-static inline void path_get_longterm(struct path *path)
-{
-	path_get(path);
-	mnt_make_longterm(path->mnt);
-}
-
-static inline void path_put_longterm(struct path *path)
-{
-	mnt_make_shortterm(path->mnt);
-	path_put(path);
-}
-
 /*
  * Replace the fs->{rootmnt,root} with {mnt,dentry}. Put the old values.
  * It can block.
@@ -26,7 +14,7 @@ void set_fs_root(struct fs_struct *fs, struct path *path)
 {
 	struct path old_root;
 
-	path_get_longterm(path);
+	path_get(path);
 	spin_lock(&fs->lock);
 	write_seqcount_begin(&fs->seq);
 	old_root = fs->root;
@@ -34,7 +22,7 @@ void set_fs_root(struct fs_struct *fs, struct path *path)
 	write_seqcount_end(&fs->seq);
 	spin_unlock(&fs->lock);
 	if (old_root.dentry)
-		path_put_longterm(&old_root);
+		path_put(&old_root);
 }
 
 /*
@@ -45,7 +33,7 @@ void set_fs_pwd(struct fs_struct *fs, struct path *path)
 {
 	struct path old_pwd;
 
-	path_get_longterm(path);
+	path_get(path);
 	spin_lock(&fs->lock);
 	write_seqcount_begin(&fs->seq);
 	old_pwd = fs->pwd;
@@ -54,7 +42,7 @@ void set_fs_pwd(struct fs_struct *fs, struct path *path)
 	spin_unlock(&fs->lock);
 
 	if (old_pwd.dentry)
-		path_put_longterm(&old_pwd);
+		path_put(&old_pwd);
 }
 
 static inline int replace_path(struct path *p, const struct path *old, const struct path *new)
@@ -84,7 +72,7 @@ void chroot_fs_refs(struct path *old_root, struct path *new_root)
 			write_seqcount_end(&fs->seq);
 			while (hits--) {
 				count++;
-				path_get_longterm(new_root);
+				path_get(new_root);
 			}
 			spin_unlock(&fs->lock);
 		}
@@ -92,13 +80,13 @@ void chroot_fs_refs(struct path *old_root, struct path *new_root)
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 	while (count--)
-		path_put_longterm(old_root);
+		path_put(old_root);
 }
 
 void free_fs_struct(struct fs_struct *fs)
 {
-	path_put_longterm(&fs->root);
-	path_put_longterm(&fs->pwd);
+	path_put(&fs->root);
+	path_put(&fs->pwd);
 	kmem_cache_free(fs_cachep, fs);
 }
 
@@ -132,9 +120,9 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)
 
 		spin_lock(&old->lock);
 		fs->root = old->root;
-		path_get_longterm(&fs->root);
+		path_get(&fs->root);
 		fs->pwd = old->pwd;
-		path_get_longterm(&fs->pwd);
+		path_get(&fs->pwd);
 		spin_unlock(&old->lock);
 	}
 	return fs;
diff --git a/fs/internal.h b/fs/internal.h
index 18bc216..d2a23ff6 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -50,8 +50,6 @@ extern int copy_mount_string(const void __user *, char **);
 extern struct vfsmount *lookup_mnt(struct path *);
 extern int finish_automount(struct vfsmount *, struct path *);
 
-extern void mnt_make_longterm(struct vfsmount *);
-extern void mnt_make_shortterm(struct vfsmount *);
 extern int sb_prepare_remount_readonly(struct super_block *);
 
 extern void __init mnt_init(void);
diff --git a/fs/mount.h b/fs/mount.h
index 4ef36d9..05a2a11 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -22,7 +22,6 @@ struct mount {
 	struct vfsmount mnt;
 #ifdef CONFIG_SMP
 	struct mnt_pcp __percpu *mnt_pcp;
-	atomic_t mnt_longterm;		/* how many of the refs are longterm */
 #else
 	int mnt_count;
 	int mnt_writers;
@@ -49,6 +48,8 @@ struct mount {
 	int mnt_ghosts;
 };
 
+#define MNT_NS_INTERNAL ERR_PTR(-EINVAL) /* distinct from any mnt_namespace */
+
 static inline struct mount *real_mount(struct vfsmount *mnt)
 {
 	return container_of(mnt, struct mount, mnt);
@@ -59,6 +60,12 @@ static inline int mnt_has_parent(struct mount *mnt)
 	return mnt != mnt->mnt_parent;
 }
 
+static inline int is_mounted(struct vfsmount *mnt)
+{
+	/* neither detached nor internal? */
+	return !IS_ERR_OR_NULL(real_mount(mnt));
+}
+
 extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *, int);
 
 static inline void get_mnt_ns(struct mnt_namespace *ns)
diff --git a/fs/namespace.c b/fs/namespace.c
index 1e4a5fe..a524ea4 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -621,21 +621,6 @@ static void attach_mnt(struct mount *mnt, struct path *path)
 	list_add_tail(&mnt->mnt_child, &real_mount(path->mnt)->mnt_mounts);
 }
 
-static inline void __mnt_make_longterm(struct mount *mnt)
-{
-#ifdef CONFIG_SMP
-	atomic_inc(&mnt->mnt_longterm);
-#endif
-}
-
-/* needs vfsmount lock for write */
-static inline void __mnt_make_shortterm(struct mount *mnt)
-{
-#ifdef CONFIG_SMP
-	atomic_dec(&mnt->mnt_longterm);
-#endif
-}
-
 /*
  * vfsmount lock must be held for write
  */
@@ -649,10 +634,8 @@ static void commit_tree(struct mount *mnt)
 	BUG_ON(parent == mnt);
 
 	list_add_tail(&head, &mnt->mnt_list);
-	list_for_each_entry(m, &head, mnt_list) {
+	list_for_each_entry(m, &head, mnt_list)
 		m->mnt_ns = n;
-		__mnt_make_longterm(m);
-	}
 
 	list_splice(&head, n->list.prev);
 
@@ -804,7 +787,8 @@ static void mntput_no_expire(struct mount *mnt)
 put_again:
 #ifdef CONFIG_SMP
 	br_read_lock(&vfsmount_lock);
-	if (likely(atomic_read(&mnt->mnt_longterm))) {
+	if (likely(mnt->mnt_ns)) {
+		/* shouldn't be the last one */
 		mnt_add_count(mnt, -1);
 		br_read_unlock(&vfsmount_lock);
 		return;
@@ -1074,8 +1058,6 @@ void umount_tree(struct mount *mnt, int propagate, struct list_head *kill)
 		list_del_init(&p->mnt_expire);
 		list_del_init(&p->mnt_list);
 		__touch_mnt_namespace(p->mnt_ns);
-		if (p->mnt_ns)
-			__mnt_make_shortterm(p);
 		p->mnt_ns = NULL;
 		list_del_init(&p->mnt_child);
 		if (mnt_has_parent(p)) {
@@ -2209,23 +2191,6 @@ static struct mnt_namespace *alloc_mnt_ns(void)
 	return new_ns;
 }
 
-void mnt_make_longterm(struct vfsmount *mnt)
-{
-	__mnt_make_longterm(real_mount(mnt));
-}
-
-void mnt_make_shortterm(struct vfsmount *m)
-{
-#ifdef CONFIG_SMP
-	struct mount *mnt = real_mount(m);
-	if (atomic_add_unless(&mnt->mnt_longterm, -1, 1))
-		return;
-	br_write_lock(&vfsmount_lock);
-	atomic_dec(&mnt->mnt_longterm);
-	br_write_unlock(&vfsmount_lock);
-#endif
-}
-
 /*
  * Allocate a new namespace structure and populate it with contents
  * copied from the namespace of the passed in task structure.
@@ -2265,18 +2230,13 @@ static struct mnt_namespace *dup_mnt_ns(struct mnt_namespace *mnt_ns,
 	q = new;
 	while (p) {
 		q->mnt_ns = new_ns;
-		__mnt_make_longterm(q);
 		if (fs) {
 			if (&p->mnt == fs->root.mnt) {
 				fs->root.mnt = mntget(&q->mnt);
-				__mnt_make_longterm(q);
-				mnt_make_shortterm(&p->mnt);
 				rootmnt = &p->mnt;
 			}
 			if (&p->mnt == fs->pwd.mnt) {
 				fs->pwd.mnt = mntget(&q->mnt);
-				__mnt_make_longterm(q);
-				mnt_make_shortterm(&p->mnt);
 				pwdmnt = &p->mnt;
 			}
 		}
@@ -2320,7 +2280,6 @@ static struct mnt_namespace *create_mnt_ns(struct vfsmount *m)
 	if (!IS_ERR(new_ns)) {
 		struct mount *mnt = real_mount(m);
 		mnt->mnt_ns = new_ns;
-		__mnt_make_longterm(mnt);
 		new_ns->root = mnt;
 		list_add(&new_ns->list, &mnt->mnt_list);
 	} else {
@@ -2615,7 +2574,7 @@ struct vfsmount *kern_mount_data(struct file_system_type *type, void *data)
 		 * it is a longterm mount, don't release mnt until
 		 * we unmount before file sys is unregistered
 		*/
-		mnt_make_longterm(mnt);
+		real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL;
 	}
 	return mnt;
 }
@@ -2625,7 +2584,9 @@ void kern_unmount(struct vfsmount *mnt)
 {
 	/* release long term mount so mount point can be released */
 	if (!IS_ERR_OR_NULL(mnt)) {
-		mnt_make_shortterm(mnt);
+		br_write_lock(&vfsmount_lock);
+		real_mount(mnt)->mnt_ns = NULL;
+		br_write_unlock(&vfsmount_lock);
 		mntput(mnt);
 	}
 }

Re: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative

[Lukáš Czerner]

On Sat, 9 Jun 2012, Al Viro wrote:

> Date: Sat, 9 Jun 2012 05:54:04 +0100
> From: Al Viro <viro@ZenIV.linux.org.uk>
> To: Luk?? Czerner <lczerner@redhat.com>
> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
>     mbroz@redhat.com
> Subject: Re: [PATCH 1/2] vfs: Do not allow mnt_longterm to go negative
> 
> On Wed, May 30, 2012 at 03:01:34AM +0100, Al Viro wrote:
> > instead and that's all it takes.  Alternatively, we can just turn the
> > code in mntput_no_expire() into
> > 	if (likely(mnt->mnt_ns || atomic_read(...))
> > and get rid of longterm/shortterm logics around ->mnt_ns completely.
> > Short-term, pardon the bad pun, I'm going with the first variant; it's
> > less intrusive.  Longer term I think we want to get rid of playing with
> > mnt_longterm in there and just check ->mnt_ns as well when deciding to
> > go for the fast path in mntput_no_expire().
> 
> Probably not, actually - mnt_make_shortterm() is called often enough (any
> chdir moving you to another fs) and practically always it's done with
> longterm reference from being mounted remaining there.  We want that to
> be fast path.  And that's exactly what we have with the current logics;
> we don't grab vfsmount_lock unless ->mnt_longterm was about to reach 0.
> We need to make sure that transition to "no longterm references left"
> would happen only under vfsmount_lock.  As it is (with the fix in 3.5-rc
> and -stable) we have this:
> 	mnt->mnt_longterm == number of references to mnt in some
> fs_struct->{root,pwd}.mnt + (is it a kernel-internal vfsmount ? 1 : 0) +
> (mnt->mnt_ns != NULL ? 1 : 0)
> and getting rid of the third part would make life consider more painful
> for chdir et.al. - we can use atomic_add_unless() for lockless path
> when dropping a pwd/root reference with the current scheme and get away
> with that in almost all cases.  Doing an equivalent with separate checks
> for mnt->mnt_ns != NULL or atomic_read(&mnt->mnt_longterm) > 0 would be
> hard and it wouldn't have won us anything.
> 
> IOW, I'm an idiot.  Especially since there is a much simpler solution:
> 	* lose ->mnt_longterm manipulations for root/pwd
> 	* lose ->mnt_longterm manipulations for places where we assign
> ->mnt_ns now
> 	* turn that check on the fast path of mntput_no_expire() into
> 	if (likely(mnt->mnt_ns))
> 	* turn manipulations with ->mnt_longterm in kern_mount_data/kern_umount
> into setting ->mnt_ns to something distinct (ERR_PTR(-EINVAL)) and clearing it.
> 	* lose ->mnt_longterm completely, along with the helpers for
> modifying it.
> 	* new helper - is_mounted(mnt); checks if ->mnt_ns points to some
> mnt_namespace (i.e. what used to be ->mnt_ns != NULL; now !IS_ERR_OR_NULL())
> 	* switch prepend_path() to use of that helper instead of direct
> check of ->mnt_ns != NULl.
> 
> Somebody with pwd on lazy-umounted vfsmount will have costlier mntput() of
> that pwd.  Everything else loses a bunch of atomic operations on fairly
> hot paths, not to mention the loss of extra atomic_t in struct vfsmount
> on SMP kernels.  BTW, looking at that thing... I really wonder if int is
> enough for mnt_count.  On boxen with a lot of RAM it might be possible to
> overflow; you are probably fucked anyway if that happens (things that
> hold struct vfsmount * are not particulary small), but still...  I suspect
> that it, and probably mnt_writers as well, should be long.  Anyway, that's
> a separate story; how about the following for ->mnt_longterm?

It looks like a good idea to me and the code seems fine. Although I
have one concern.

Currently mnt_ns could contain only valid pointer or NULL, however
with this patch it can also contain ERR_PTR(-EINVAL) which might be
a problem if we hit 'if (ns)' condition followed by dereferencing the
pointer. Especially touch_mnt_namespace() and __touch_mnt_namespace()
looks rather suspicious. I am not sure it we can get into this codepath
with this mnt_ns, but I do not immediately see why not.

It seems that we should rather change those condition to use
IS_ERR_OR_NULL(). What do you think ?

Thanks!
-Lukas

> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 4046904..44acb5b 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2622,7 +2622,7 @@ global_root:
>  	if (!slash)
>  		error = prepend(buffer, buflen, "/", 1);
>  	if (!error)
> -		error = real_mount(vfsmnt)->mnt_ns ? 1 : 2;
> +		error = is_mounted(vfsmnt) ? 1 : 2;
>  	goto out;
>  }
>  
> diff --git a/fs/fs_struct.c b/fs/fs_struct.c
> index e159e68..5df4775 100644
> --- a/fs/fs_struct.c
> +++ b/fs/fs_struct.c
> @@ -6,18 +6,6 @@
>  #include <linux/fs_struct.h>
>  #include "internal.h"
>  
> -static inline void path_get_longterm(struct path *path)
> -{
> -	path_get(path);
> -	mnt_make_longterm(path->mnt);
> -}
> -
> -static inline void path_put_longterm(struct path *path)
> -{
> -	mnt_make_shortterm(path->mnt);
> -	path_put(path);
> -}
> -
>  /*
>   * Replace the fs->{rootmnt,root} with {mnt,dentry}. Put the old values.
>   * It can block.
> @@ -26,7 +14,7 @@ void set_fs_root(struct fs_struct *fs, struct path *path)
>  {
>  	struct path old_root;
>  
> -	path_get_longterm(path);
> +	path_get(path);
>  	spin_lock(&fs->lock);
>  	write_seqcount_begin(&fs->seq);
>  	old_root = fs->root;
> @@ -34,7 +22,7 @@ void set_fs_root(struct fs_struct *fs, struct path *path)
>  	write_seqcount_end(&fs->seq);
>  	spin_unlock(&fs->lock);
>  	if (old_root.dentry)
> -		path_put_longterm(&old_root);
> +		path_put(&old_root);
>  }
>  
>  /*
> @@ -45,7 +33,7 @@ void set_fs_pwd(struct fs_struct *fs, struct path *path)
>  {
>  	struct path old_pwd;
>  
> -	path_get_longterm(path);
> +	path_get(path);
>  	spin_lock(&fs->lock);
>  	write_seqcount_begin(&fs->seq);
>  	old_pwd = fs->pwd;
> @@ -54,7 +42,7 @@ void set_fs_pwd(struct fs_struct *fs, struct path *path)
>  	spin_unlock(&fs->lock);
>  
>  	if (old_pwd.dentry)
> -		path_put_longterm(&old_pwd);
> +		path_put(&old_pwd);
>  }
>  
>  static inline int replace_path(struct path *p, const struct path *old, const struct path *new)
> @@ -84,7 +72,7 @@ void chroot_fs_refs(struct path *old_root, struct path *new_root)
>  			write_seqcount_end(&fs->seq);
>  			while (hits--) {
>  				count++;
> -				path_get_longterm(new_root);
> +				path_get(new_root);
>  			}
>  			spin_unlock(&fs->lock);
>  		}
> @@ -92,13 +80,13 @@ void chroot_fs_refs(struct path *old_root, struct path *new_root)
>  	} while_each_thread(g, p);
>  	read_unlock(&tasklist_lock);
>  	while (count--)
> -		path_put_longterm(old_root);
> +		path_put(old_root);
>  }
>  
>  void free_fs_struct(struct fs_struct *fs)
>  {
> -	path_put_longterm(&fs->root);
> -	path_put_longterm(&fs->pwd);
> +	path_put(&fs->root);
> +	path_put(&fs->pwd);
>  	kmem_cache_free(fs_cachep, fs);
>  }
>  
> @@ -132,9 +120,9 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)
>  
>  		spin_lock(&old->lock);
>  		fs->root = old->root;
> -		path_get_longterm(&fs->root);
> +		path_get(&fs->root);
>  		fs->pwd = old->pwd;
> -		path_get_longterm(&fs->pwd);
> +		path_get(&fs->pwd);
>  		spin_unlock(&old->lock);
>  	}
>  	return fs;
> diff --git a/fs/internal.h b/fs/internal.h
> index 18bc216..d2a23ff6 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -50,8 +50,6 @@ extern int copy_mount_string(const void __user *, char **);
>  extern struct vfsmount *lookup_mnt(struct path *);
>  extern int finish_automount(struct vfsmount *, struct path *);
>  
> -extern void mnt_make_longterm(struct vfsmount *);
> -extern void mnt_make_shortterm(struct vfsmount *);
>  extern int sb_prepare_remount_readonly(struct super_block *);
>  
>  extern void __init mnt_init(void);
> diff --git a/fs/mount.h b/fs/mount.h
> index 4ef36d9..05a2a11 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -22,7 +22,6 @@ struct mount {
>  	struct vfsmount mnt;
>  #ifdef CONFIG_SMP
>  	struct mnt_pcp __percpu *mnt_pcp;
> -	atomic_t mnt_longterm;		/* how many of the refs are longterm */
>  #else
>  	int mnt_count;
>  	int mnt_writers;
> @@ -49,6 +48,8 @@ struct mount {
>  	int mnt_ghosts;
>  };
>  
> +#define MNT_NS_INTERNAL ERR_PTR(-EINVAL) /* distinct from any mnt_namespace */
> +
>  static inline struct mount *real_mount(struct vfsmount *mnt)
>  {
>  	return container_of(mnt, struct mount, mnt);
> @@ -59,6 +60,12 @@ static inline int mnt_has_parent(struct mount *mnt)
>  	return mnt != mnt->mnt_parent;
>  }
>  
> +static inline int is_mounted(struct vfsmount *mnt)
> +{
> +	/* neither detached nor internal? */
> +	return !IS_ERR_OR_NULL(real_mount(mnt));
> +}
> +
>  extern struct mount *__lookup_mnt(struct vfsmount *, struct dentry *, int);
>  
>  static inline void get_mnt_ns(struct mnt_namespace *ns)
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 1e4a5fe..a524ea4 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -621,21 +621,6 @@ static void attach_mnt(struct mount *mnt, struct path *path)
>  	list_add_tail(&mnt->mnt_child, &real_mount(path->mnt)->mnt_mounts);
>  }
>  
> -static inline void __mnt_make_longterm(struct mount *mnt)
> -{
> -#ifdef CONFIG_SMP
> -	atomic_inc(&mnt->mnt_longterm);
> -#endif
> -}
> -
> -/* needs vfsmount lock for write */
> -static inline void __mnt_make_shortterm(struct mount *mnt)
> -{
> -#ifdef CONFIG_SMP
> -	atomic_dec(&mnt->mnt_longterm);
> -#endif
> -}
> -
>  /*
>   * vfsmount lock must be held for write
>   */
> @@ -649,10 +634,8 @@ static void commit_tree(struct mount *mnt)
>  	BUG_ON(parent == mnt);
>  
>  	list_add_tail(&head, &mnt->mnt_list);
> -	list_for_each_entry(m, &head, mnt_list) {
> +	list_for_each_entry(m, &head, mnt_list)
>  		m->mnt_ns = n;
> -		__mnt_make_longterm(m);
> -	}
>  
>  	list_splice(&head, n->list.prev);
>  
> @@ -804,7 +787,8 @@ static void mntput_no_expire(struct mount *mnt)
>  put_again:
>  #ifdef CONFIG_SMP
>  	br_read_lock(&vfsmount_lock);
> -	if (likely(atomic_read(&mnt->mnt_longterm))) {
> +	if (likely(mnt->mnt_ns)) {
> +		/* shouldn't be the last one */
>  		mnt_add_count(mnt, -1);
>  		br_read_unlock(&vfsmount_lock);
>  		return;
> @@ -1074,8 +1058,6 @@ void umount_tree(struct mount *mnt, int propagate, struct list_head *kill)
>  		list_del_init(&p->mnt_expire);
>  		list_del_init(&p->mnt_list);
>  		__touch_mnt_namespace(p->mnt_ns);
> -		if (p->mnt_ns)
> -			__mnt_make_shortterm(p);
>  		p->mnt_ns = NULL;
>  		list_del_init(&p->mnt_child);
>  		if (mnt_has_parent(p)) {
> @@ -2209,23 +2191,6 @@ static struct mnt_namespace *alloc_mnt_ns(void)
>  	return new_ns;
>  }
>  
> -void mnt_make_longterm(struct vfsmount *mnt)
> -{
> -	__mnt_make_longterm(real_mount(mnt));
> -}
> -
> -void mnt_make_shortterm(struct vfsmount *m)
> -{
> -#ifdef CONFIG_SMP
> -	struct mount *mnt = real_mount(m);
> -	if (atomic_add_unless(&mnt->mnt_longterm, -1, 1))
> -		return;
> -	br_write_lock(&vfsmount_lock);
> -	atomic_dec(&mnt->mnt_longterm);
> -	br_write_unlock(&vfsmount_lock);
> -#endif
> -}
> -
>  /*
>   * Allocate a new namespace structure and populate it with contents
>   * copied from the namespace of the passed in task structure.
> @@ -2265,18 +2230,13 @@ static struct mnt_namespace *dup_mnt_ns(struct mnt_namespace *mnt_ns,
>  	q = new;
>  	while (p) {
>  		q->mnt_ns = new_ns;
> -		__mnt_make_longterm(q);
>  		if (fs) {
>  			if (&p->mnt == fs->root.mnt) {
>  				fs->root.mnt = mntget(&q->mnt);
> -				__mnt_make_longterm(q);
> -				mnt_make_shortterm(&p->mnt);
>  				rootmnt = &p->mnt;
>  			}
>  			if (&p->mnt == fs->pwd.mnt) {
>  				fs->pwd.mnt = mntget(&q->mnt);
> -				__mnt_make_longterm(q);
> -				mnt_make_shortterm(&p->mnt);
>  				pwdmnt = &p->mnt;
>  			}
>  		}
> @@ -2320,7 +2280,6 @@ static struct mnt_namespace *create_mnt_ns(struct vfsmount *m)
>  	if (!IS_ERR(new_ns)) {
>  		struct mount *mnt = real_mount(m);
>  		mnt->mnt_ns = new_ns;
> -		__mnt_make_longterm(mnt);
>  		new_ns->root = mnt;
>  		list_add(&new_ns->list, &mnt->mnt_list);
>  	} else {
> @@ -2615,7 +2574,7 @@ struct vfsmount *kern_mount_data(struct file_system_type *type, void *data)
>  		 * it is a longterm mount, don't release mnt until
>  		 * we unmount before file sys is unregistered
>  		*/
> -		mnt_make_longterm(mnt);
> +		real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL;
>  	}
>  	return mnt;
>  }
> @@ -2625,7 +2584,9 @@ void kern_unmount(struct vfsmount *mnt)
>  {
>  	/* release long term mount so mount point can be released */
>  	if (!IS_ERR_OR_NULL(mnt)) {
> -		mnt_make_shortterm(mnt);
> +		br_write_lock(&vfsmount_lock);
> +		real_mount(mnt)->mnt_ns = NULL;
> +		br_write_unlock(&vfsmount_lock);
>  		mntput(mnt);
>  	}
>  }
> 

[PATCH v2] vfs: Do not allow mnt_longterm to go negative

[Lukas Czerner]

Currently when someone calls __mnt_make_shortterm() and the mnt_longterm
is already zero, it will come negative which is exactly opposite of what
the function should to. So fix this by decrementing mnt_longterm only in
case that it's not zero. Note that mnt_longterm should not be touched
directly, but rather via __mnt_make_shortterm() and mnt_make_shortterm()
functions - the former does not have this problem.

Moreover this will fix very nasty bug which might cause file systems not
being properly cleaned up while unmounting. Specifically it might cause
deactivate_super() not being called at all, hence the file system would
be still up and running even after successful unmount without user even
noticing it.

The following scenario leads to a bug. While mounting propagate_mnt()
would walk through propagation tree cloning the mount for every every
mount the root propagates to. Even for mounts which parents are not
parents of the directory you're trying to mount to. So it will bump up
the s_active counter.

However before the propagate_mnt() is left it will clean the unneeded
cloned mounts (those which parent root is not parent of your new mount
point), but it will _NOT_ decrement the s_active because umount_tree()
would decrement mnt_longterm causing it to underflow, hence
mntput_no_expire() would bail out before actually calling mntfree() and
releasing the super block (decreasing s_active). At this point super
block will never get released.

User can not directly observe this (aside from seeing running file system
processes even after unmount), but it can be easily reproduced with loop
device.

mount --make-shared /
mount --bind /tmp/one /tmp/two
losetup /dev/loop0 /mnt/test/file0
mount /dev/loop0 /mnt/test1
umount /mnt/test1
losetup -d /dev/loop0
^^^^^^^^^^^^^^^^^^^^^
This will fail because the device is still busy (super block is still
active and s_active is non zero).

This commit fixes this issue.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Milan Broz <mbroz@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: stable@vger.kernel.org
---
v2: Added important step to the reproducer which I forgor previously
    --make-shared needs to be done on /

 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index e608199..e72fec7 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -632,7 +632,7 @@ static inline void __mnt_make_longterm(struct mount *mnt)
 static inline void __mnt_make_shortterm(struct mount *mnt)
 {
 #ifdef CONFIG_SMP
-	atomic_dec(&mnt->mnt_longterm);
+	atomic_add_unless(&mnt->mnt_longterm, -1, 0);
 #endif
 }
 
-- 
1.7.7.6