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 <[email protected]>
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);
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.
> > 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