lkml.org 
[lkml]   [2013]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 04/11] vfs: check unlinked ancestors before mount
    From
    On Thu, Sep 5, 2013 at 1:18 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
    > On Thu, Sep 05, 2013 at 11:44:37AM +0200, Miklos Szeredi wrote:
    >> +static bool __has_unlinked_ancestor(struct dentry *dentry)
    >> +{
    >> + struct dentry *this;
    >> +
    >> + for (this = dentry; !IS_ROOT(this); this = this->d_parent) {
    >> + int is_unhashed;
    >> +
    >> + /* Need exclusion wrt. check_submounts_and_drop() */
    >> + spin_lock(&this->d_lock);
    >> + is_unhashed = d_unhashed(this);
    >> + spin_unlock(&this->d_lock);
    >> +
    >> + if (is_unhashed)
    >> + return true;
    >> + }
    >> + return false;
    >> +}
    >> +
    >> +/*
    >> + * Called by mount code to check if the mountpoint is reachable (e.g. NFS can
    >> + * unhash a directory dentry and then the complete subtree can become
    >> + * unreachable).
    >> + */
    >> +bool has_unlinked_ancestor(struct dentry *dentry)
    >> +{
    >> + bool found;
    >> +
    >> + /* Need exclusion wrt. check_submounts_and_drop() */
    >> + write_seqlock(&rename_lock);
    >> + found = __has_unlinked_ancestor(dentry);
    >> + write_sequnlock(&rename_lock);
    >> +
    >> + return found;
    >> +}
    >> +
    >> /*
    >> * Search the dentry child list of the specified parent,
    >> * and move any unused dentries to the end of the unused
    >> diff --git a/fs/internal.h b/fs/internal.h
    >> index 7c5f01c..d232355 100644
    >> --- a/fs/internal.h
    >> +++ b/fs/internal.h
    >> @@ -126,6 +126,7 @@ extern int invalidate_inodes(struct super_block *, bool);
    >> * dcache.c
    >> */
    >> extern struct dentry *__d_alloc(struct super_block *, const struct qstr *);
    >> +extern bool has_unlinked_ancestor(struct dentry *dentry);
    >>
    >> /*
    >> * read_write.c
    >> diff --git a/fs/namespace.c b/fs/namespace.c
    >> index a45ba4f..91b1c39 100644
    >> --- a/fs/namespace.c
    >> +++ b/fs/namespace.c
    >> @@ -634,6 +634,15 @@ static struct mountpoint *new_mountpoint(struct dentry *dentry)
    >> }
    >> dentry->d_flags |= DCACHE_MOUNTED;
    >> spin_unlock(&dentry->d_lock);
    >> +
    >> + if (has_unlinked_ancestor(dentry)) {
    >> + spin_lock(&dentry->d_lock);
    >> + dentry->d_flags &= ~DCACHE_MOUNTED;
    >> + spin_unlock(&dentry->d_lock);
    >> + kfree(mp);
    >> + return ERR_PTR(-ENOENT);
    >> + }
    >
    > Something's really odd with locking here. You are take d_lock, do one
    > check, set flag, drop d_lock, grab rename_lock, do another check (taking
    > and dropping d_lock in process), and, in case that check fails, grab
    > d_lock again to clear the flag.
    >
    > At the very least it's a massive overkill. Just grab rename_lock, then
    > d_lock, then do the damn check and set the flag only on success. Moreover,
    > with rename_lock held, do you need d_lock on ancestors to mess with in
    > has_unlinked_ancestor()?

    Yes, we need hard exclusion for the __d_drop() part. rename_lock can
    provide one if we always take it for write in
    check_submounts_and_drop(). But if we only take it for read then
    that's not enough.

    And we do in fact also need DCACHE_MOUNTED set *before* checking
    ancestors. Otherwise check_submounts_and_drop() could succeed and
    has_unlinked_ancestor() return false, resulting in a dropped dentry
    and a mount below it. Though this is mostly theoretical at this
    point.

    Thanks,
    Miklos


    \
     
     \ /
      Last update: 2013-09-05 13:41    [W:4.869 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site