lkml.org 
[lkml]   [2005]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Subject[PATCH] fix race in mark_mounts_for_expiry()
    From
    Date
    > > > > 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/

    \
     
     \ /
      Last update: 2005-05-18 11:56    [W:0.024 / U:1.440 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site