[PATCH] namespace.c: fix race in mark_mounts_for_expiry()

[PATCH] namespace.c: fix race in mark_mounts_for_expiry()

[Miklos Szeredi]

This patch fixes a race found by Ram in mark_mounts_for_expiry() in
fs/namespace.c.

The bug can only be triggered with simultaneous exiting of a process
having a private namespace, and expiry of a mount from within that
namespace.  It's practically impossible to trigger, and I haven't even
tried.  But still, a bug is a bug.

The race happens when put_namespace() is called by another task, while
mark_mounts_for_expiry() is between atomic_read() and get_namespace().
In that case get_namespace() will be called on an already dead
namespace with unforeseeable results.

The solution is to make the atomic_read() and the get_namespace() into
a single atomic operation.  The patch does this in a fairly ugly way
(see comment above fix), which should be safe regardless.

This is a mininal fix, really only serving as a reminder, that this
usage of mnt_namespace is ugly and needs to be properly cleaned up.

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

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2005-05-19 15:48:21.000000000 +0200
+++ linux/fs/namespace.c	2005-05-19 16:31:35.000000000 +0200
@@ -869,9 +869,24 @@ 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)
+		if (!namespace)
 			continue;
-		get_namespace(namespace);
+		
+		/* Hack hack.  Need to get reference to namespace
+		   while ensuring that count is not already zero.
+		   There's no such atomic operation, so do the check
+		   _after_ increasing count.  If count was zero,
+		   decrease it back again, so a later instance of this
+		   will find it zero.
+
+		   This is OK, since all other uses of namespace have
+		   proper reference, so when count is zero, nobody
+		   else cares.  Race with itself is avoided by holding
+		   vfsmount_lock. */
+		if (atomic_add_return(1, &namespace->count) <= 1) {
+			atomic_dec(&namespace->count);
+			continue;
+		}
 
 		spin_unlock(&vfsmount_lock);
 		down_write(&namespace->sem);

Re: [PATCH] namespace.c: fix race in mark_mounts_for_expiry()

[Al Viro]

On Fri, May 20, 2005 at 10:41:24AM +0200, Miklos Szeredi wrote:
> This patch fixes a race found by Ram in mark_mounts_for_expiry() in
> The solution is to make the atomic_read() and the get_namespace() into
> a single atomic operation.  The patch does this in a fairly ugly way
> (see comment above fix), which should be safe regardless.

That it certainly is.  What's more, there is a trivial way to deal with
that crap - have put_namespace() do atomic_dec_and_lock() instead of
atomic_dec_and_test().  And use the same spinlock (vfsmount_lock would be
an obvious choice) to protect the atomicity here.  End of story.

Re: [PATCH] namespace.c: fix race in mark_mounts_for_expiry()

[Miklos Szeredi]

> > This patch fixes a race found by Ram in mark_mounts_for_expiry() in
> > The solution is to make the atomic_read() and the get_namespace() into
> > a single atomic operation.  The patch does this in a fairly ugly way
> > (see comment above fix), which should be safe regardless.
> 
> That it certainly is.  What's more, there is a trivial way to deal with
> that crap - have put_namespace() do atomic_dec_and_lock() instead of
> atomic_dec_and_test().  And use the same spinlock (vfsmount_lock would be
> an obvious choice) to protect the atomicity here.  End of story.

Right.

Question is, why did nobody think of that before :)

Thanks,
Miklos