[PATCH] fix race in mark_mounts_for_expiry()

[Miklos Szeredi <miklos@szeredi.hu>]

> > > > I am seeing a locking issue with get_namespace() and put_namespace()
> > > > 
> > > > lets say put_namespace() is called and it finds that it is the last
> > > > user of the namespace is just about to  call __put_namespace().
> > > > 
> > > > Simultaneously another thread calls get_namespace() and increments
> > > > the count.  
> > > > 
> > > > At this point __put_namespace() goes ahead and cleans the namespace.
> > > 
> > > I think you are right. 
> > > 
> > > There's one place in the existing code, which could experience this
> > > race I think:
> > > 
> > > 
> > > 		/* don't do anything if the namespace is dead - all the
> > > 		 * vfsmounts from it are going away anyway */
> > > 		namespace = mnt->mnt_namespace;
> > > 		if (!namespace || atomic_read(&namespace->count) <= 0)
> > > 			continue;
> > > here ----> 
> > > 		get_namespace(namespace);
> > > 
> > > 		spin_unlock(&vfsmount_lock);
> > > 
> > > 
> > > Locking vfsmount_lock in put_namespace() would fix it.  Any better ideas?
> > > 
> > yes. it will.
> > 
> > However I don't think this issue will trigger currently because for this
> > to happen, a process should attempt a get_namespace() on a foreign
> > namespace. If all tasks are operating in the same
> > namespace, than the count will not go to zero for put namespace to
> > clean it up, unless it is the last task.
> > 
> > I think, it will trigger once we allow recursive binds from foreign
> > namespace.
> 
> No I am wrong. mark_mounts_for_expiry() is not called from a process
> context. Its in a timer context. So the race could happen.

How about this patch?  It tries to solve this race without additional
locking.  If refcount is already zero, it will increment and decrement
it.  So be careful to only call grab_namespace() with vfsmount_lock
held, otherwise it could race with itself.  (vfsmount_lock is also
needed in this case so that mnt->mnt_namespace, doesn't change, while
grabbing the namespace)

Compile tested only, please review carefully.

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2005-05-17 13:46:58.000000000 +0200
+++ linux/fs/namespace.c	2005-05-18 11:09:47.000000000 +0200
@@ -825,6 +825,16 @@ unlock:
 
 EXPORT_SYMBOL_GPL(do_add_mount);
 
+/* Must be called with vfsmount_lock */
+static inline struct namespace *grab_namespace(struct namespace *n)
+{
+	if (n && atomic_add_return(1, &n->count) <= 1) {
+		atomic_dec(&n->count);
+		n = NULL;
+	}
+	return n;
+}
+
 /*
  * process a list of expirable mountpoints with the intent of discarding any
  * mountpoints that aren't in use and haven't been touched since last we came
@@ -868,10 +878,9 @@ void mark_mounts_for_expiry(struct list_
 
 		/* don't do anything if the namespace is dead - all the
 		 * vfsmounts from it are going away anyway */
-		namespace = mnt->mnt_namespace;
-		if (!namespace || atomic_read(&namespace->count) <= 0)
+		namespace = grab_namespace(mnt->mnt_namespace);
+		if (!namespace)
 			continue;
-		get_namespace(namespace);
 
 		spin_unlock(&vfsmount_lock);
 		down_write(&namespace->sem);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/