Unionfs: lock parents' branch configuration fixes

Ensure that we lock the branch configuration of parent and child dentries in
operations which need it, and in the right order.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 68023164de7a21a2301b47c712deb742f2933f32..15f4a0ac2dba01a0f2bdad1e151de2d1a2736511 100644 (file)
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -367,8 +367,9 @@ out:
  * Revalidate the struct file
  * @file: file to revalidate
  * @willwrite: true if caller may cause changes to the file; false otherwise.
+ * Caller must lock/unlock dentry's branch configuration.
  */
-int unionfs_file_revalidate(struct file *file, bool willwrite)
+int unionfs_file_revalidate_locked(struct file *file, bool willwrite)
 {
        struct super_block *sb;
        struct dentry *dentry;
@@ -378,7 +379,6 @@ int unionfs_file_revalidate(struct file *file, bool willwrite)
        int err = 0;
 
        dentry = file->f_path.dentry;
-       unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
        sb = dentry->d_sb;
 
        /*
@@ -483,7 +483,17 @@ out:
 out_nofree:
        if (!err)
                unionfs_check_file(file);
-       unionfs_unlock_dentry(dentry);
+       return err;
+}
+
+int unionfs_file_revalidate(struct file *file, bool willwrite)
+{
+       int err;
+
+       unionfs_lock_dentry(file->f_path.dentry, UNIONFS_DMUTEX_CHILD);
+       err = unionfs_file_revalidate_locked(file, willwrite);
+       unionfs_unlock_dentry(file->f_path.dentry);
+
        return err;
 }
 
@@ -606,9 +616,18 @@ int unionfs_open(struct inode *inode, struct file *file)
        struct dentry *dentry = file->f_path.dentry;
        int bindex = 0, bstart = 0, bend = 0;
        int size;
+       int valid = 0;
 
        unionfs_read_lock(inode->i_sb, UNIONFS_SMUTEX_PARENT);
        unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
+       if (dentry != dentry->d_parent)
+               unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
+
+       valid = __unionfs_d_revalidate_chain(dentry->d_parent, NULL, false);
+       if (unlikely(!valid)) {
+               err = -ESTALE;
+               goto out_nofree;
+       }
 
        /*
         * With nfs exporting we cannot get a disconnected dentry here,
@@ -682,6 +701,8 @@ out_nofree:
                unionfs_check_file(file);
                unionfs_check_inode(inode);
        }
+       if (dentry != dentry->d_parent)
+               unionfs_unlock_dentry(dentry->d_parent);
        unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(inode->i_sb);
        return err;
@@ -878,8 +899,9 @@ int unionfs_flush(struct file *file, fl_owner_t id)
        int bindex, bstart, bend;
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_PARENT);
+       unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
 
-       err = unionfs_file_revalidate(file, true);
+       err = unionfs_file_revalidate_locked(file, true);
        if (unlikely(err))
                goto out;
        unionfs_check_file(file);
@@ -905,6 +927,7 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 
 out:
        unionfs_check_file(file);
+       unionfs_unlock_dentry(file->f_path.dentry);
        unionfs_read_unlock(dentry->d_sb);
        return err;
 }

diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 418945dd298b8fac0bc5f62fcba8dd0d36f10f09..5a7fca4eb2373fdba653b25278a872cba84c71f2 100644 (file)
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -381,6 +381,7 @@ bool __unionfs_d_revalidate_chain(struct dentry *dentry, struct nameidata *nd,
        chain_len = 0;
        sbgen = atomic_read(&UNIONFS_SB(dentry->d_sb)->generation);
        dtmp = dentry->d_parent;
+       verify_locked(dentry);
        if (dentry != dtmp)
                unionfs_lock_dentry(dtmp, UNIONFS_DMUTEX_REVAL_PARENT);
        dgen = atomic_read(&UNIONFS_D(dtmp)->generation);
@@ -471,7 +472,7 @@ bool __unionfs_d_revalidate_chain(struct dentry *dentry, struct nameidata *nd,
 
 out_this:
        /* finally, lock this dentry and revalidate it */
-       verify_locked(dentry);
+       verify_locked(dentry);  /* verify child is locked */
        if (dentry != dentry->d_parent)
                unionfs_lock_dentry(dentry->d_parent,
                                    UNIONFS_DMUTEX_REVAL_PARENT);
@@ -509,24 +510,20 @@ static int unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
        return err;
 }
 
-/*
- * At this point no one can reference this dentry, so we don't have to be
- * careful about concurrent access.
- */
 static void unionfs_d_release(struct dentry *dentry)
 {
        int bindex, bstart, bend;
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
+       /* must lock our branch configuration here */
+       unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
 
        unionfs_check_dentry(dentry);
        /* this could be a negative dentry, so check first */
-       if (unlikely(!UNIONFS_D(dentry))) {
-               printk(KERN_ERR "unionfs: dentry without private data: %.*s\n",
-                      dentry->d_name.len, dentry->d_name.name);
-               goto out;
-       } else if (dbstart(dentry) < 0)
-               goto out_free;  /* due to a (normal) failed lookup */
+       if (unlikely(!UNIONFS_D(dentry) || dbstart(dentry) < 0)) {
+               unionfs_unlock_dentry(dentry);
+               goto out;       /* due to a (normal) failed lookup */
+       }
 
        /* Release all the lower dentries */
        bstart = dbstart(dentry);
@@ -544,11 +541,10 @@ static void unionfs_d_release(struct dentry *dentry)
        kfree(UNIONFS_D(dentry)->lower_paths);
        UNIONFS_D(dentry)->lower_paths = NULL;
 
-out_free:
-       /* No need to unlock it, because it is disappeared. */
-       free_dentry_private_data(dentry);
+       unionfs_unlock_dentry(dentry);
 
 out:
+       free_dentry_private_data(dentry);
        unionfs_read_unlock(dentry->d_sb);
        return;
 }
@@ -563,6 +559,7 @@ static void unionfs_d_iput(struct dentry *dentry, struct inode *inode)
 
        BUG_ON(!dentry);
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
+       unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
 
        if (!UNIONFS_D(dentry) || dbstart(dentry) < 0)
                goto drop_lower_inodes;
@@ -592,6 +589,7 @@ drop_lower_inodes:
 
        iput(inode);
 
+       unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(dentry->d_sb);
 }
 

diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index c456f300107376f0c43e88cca357b11090a55e88..f800d43edfac1eba332c964ca49038b91ebed4ac 100644 (file)
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -110,16 +110,16 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
        struct nameidata lower_nd;
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
+       unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
        unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
+
        valid = __unionfs_d_revalidate_chain(dentry->d_parent, nd, false);
-       unionfs_unlock_dentry(dentry->d_parent);
        if (unlikely(!valid)) {
                err = -ESTALE;  /* same as what real_lookup does */
                goto out;
        }
-       unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
 
-       valid = __unionfs_d_revalidate_chain(dentry, nd, false);
+       valid = __unionfs_d_revalidate_one_locked(dentry, nd, false);
        /*
         * It's only a bug if this dentry was not negative and couldn't be
         * revalidated (shouldn't happen).
@@ -170,14 +170,13 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
        }
 
 out:
-       if (!err)
-               unionfs_postcopyup_setmnt(dentry);
-
-       unionfs_check_inode(parent);
        if (!err) {
+               unionfs_postcopyup_setmnt(dentry);
+               unionfs_check_inode(parent);
                unionfs_check_dentry(dentry);
                unionfs_check_nd(nd);
        }
+       unionfs_unlock_dentry(dentry->d_parent);
        unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(dentry->d_sb);
        return err;
@@ -197,7 +196,7 @@ static struct dentry *unionfs_lookup(struct inode *parent,
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
        if (dentry != dentry->d_parent)
-               unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
+               unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_ROOT);
 
        /* save the dentry & vfsmnt from namei */
        if (nd) {
@@ -219,6 +218,7 @@ static struct dentry *unionfs_lookup(struct inode *parent,
        if (!IS_ERR(ret)) {
                if (ret)
                        dentry = ret;
+               unionfs_copy_attr_times(dentry->d_inode);
                /* parent times may have changed */
                unionfs_copy_attr_times(dentry->d_parent->d_inode);
        }
@@ -398,12 +398,20 @@ static int unionfs_symlink(struct inode *dir, struct dentry *dentry,
        struct dentry *lower_dentry = NULL;
        struct dentry *lower_dir_dentry = NULL;
        umode_t mode;
+       int valid;
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
        unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
+       unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
+
+       valid = __unionfs_d_revalidate_chain(dentry->d_parent, NULL, false);
+       if (unlikely(!valid)) {
+               err = -ESTALE;
+               goto out;
+       }
 
        if (unlikely(dentry->d_inode &&
-                    !__unionfs_d_revalidate_chain(dentry, NULL, false))) {
+                    !__unionfs_d_revalidate_one_locked(dentry, NULL, false))) {
                err = -ESTALE;
                goto out;
        }
@@ -447,12 +455,12 @@ static int unionfs_symlink(struct inode *dir, struct dentry *dentry,
                d_drop(dentry);
 
 out:
-       if (!err)
+       if (!err) {
                unionfs_postcopyup_setmnt(dentry);
-
-       unionfs_check_inode(dir);
-       if (!err)
+               unionfs_check_inode(dir);
                unionfs_check_dentry(dentry);
+       }
+       unionfs_unlock_dentry(dentry->d_parent);
        unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(dentry->d_sb);
 
@@ -466,12 +474,20 @@ static int unionfs_mkdir(struct inode *parent, struct dentry *dentry, int mode)
        struct dentry *lower_parent_dentry = NULL;
        char *name = NULL;
        int i, bend, bindex;
+       int valid;
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
        unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
+       unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
+
+       valid = __unionfs_d_revalidate_chain(dentry->d_parent, NULL, false);
+       if (unlikely(!valid)) {
+               err = -ESTALE;  /* same as what real_lookup does */
+               goto out;
+       }
 
        if (unlikely(dentry->d_inode &&
-                    !__unionfs_d_revalidate_chain(dentry, NULL, false))) {
+               !__unionfs_d_revalidate_one_locked(dentry, NULL, false))) {
                err = -ESTALE;
                goto out;
        }
@@ -563,6 +579,7 @@ out:
        }
        unionfs_check_inode(parent);
        unionfs_check_dentry(dentry);
+       unionfs_unlock_dentry(dentry->d_parent);
        unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(dentry->d_sb);
 
@@ -575,12 +592,20 @@ static int unionfs_mknod(struct inode *dir, struct dentry *dentry, int mode,
        int err = 0;
        struct dentry *lower_dentry = NULL;
        struct dentry *lower_parent_dentry = NULL;
+       int valid;
 
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
        unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
+       unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
+
+       valid = __unionfs_d_revalidate_chain(dentry->d_parent, NULL, false);
+       if (unlikely(!valid)) {
+               err = -ESTALE;  /* same as what real_lookup does */
+               goto out;
+       }
 
        if (unlikely(dentry->d_inode &&
-                    !__unionfs_d_revalidate_chain(dentry, NULL, false))) {
+                    !__unionfs_d_revalidate_one_locked(dentry, NULL, false))) {
                err = -ESTALE;
                goto out;
        }
@@ -630,12 +655,12 @@ out:
        if (!dentry->d_inode)
                d_drop(dentry);
 
-       if (!err)
+       if (!err) {
                unionfs_postcopyup_setmnt(dentry);
-
-       unionfs_check_inode(dir);
-       if (!err)
+               unionfs_check_inode(dir);
                unionfs_check_dentry(dentry);
+       }
+       unionfs_unlock_dentry(dentry->d_parent);
        unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(dentry->d_sb);
 

diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 97d73c1a5ad110a702640f49b992185ca2d370e6..e826427cb1b1d033210142e2eaec3d9bb65020f5 100644 (file)
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -306,11 +306,11 @@ static inline void unionfs_double_lock_dentry(struct dentry *d1,
 {
        BUG_ON(d1 == d2);
        if (d1 < d2) {
-               unionfs_lock_dentry(d1, UNIONFS_DMUTEX_PARENT);
                unionfs_lock_dentry(d2, UNIONFS_DMUTEX_CHILD);
+               unionfs_lock_dentry(d1, UNIONFS_DMUTEX_PARENT);
        } else {
-               unionfs_lock_dentry(d2, UNIONFS_DMUTEX_PARENT);
                unionfs_lock_dentry(d1, UNIONFS_DMUTEX_CHILD);
+               unionfs_lock_dentry(d2, UNIONFS_DMUTEX_PARENT);
        }
 }
 
@@ -361,6 +361,7 @@ extern int unionfs_setlk(struct file *file, int cmd, struct file_lock *fl);
 extern int unionfs_getlk(struct file *file, struct file_lock *fl);
 
 /* Common file operations. */
+extern int unionfs_file_revalidate_locked(struct file *file, bool willwrite);
 extern int unionfs_file_revalidate(struct file *file, bool willwrite);
 extern int unionfs_open(struct inode *inode, struct file *file);
 extern int unionfs_file_release(struct inode *inode, struct file *file);

diff --git a/fs/unionfs/unlink.c b/fs/unionfs/unlink.c
index d7203924de562a917de301115375c5d34a3eef5e..c77f4308f86caf361194bc383b37a4245b4e7882 100644 (file)
--- a/fs/unionfs/unlink.c
+++ b/fs/unionfs/unlink.c
@@ -102,12 +102,20 @@ int unionfs_unlink(struct inode *dir, struct dentry *dentry)
 {
        int err = 0;
        struct inode *inode = dentry->d_inode;
+       int valid;
 
        BUG_ON(S_ISDIR(inode->i_mode));
        unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
        unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
+       unionfs_lock_dentry(dentry->d_parent, UNIONFS_DMUTEX_PARENT);
 
-       if (unlikely(!__unionfs_d_revalidate_chain(dentry, NULL, false))) {
+       valid = __unionfs_d_revalidate_chain(dentry->d_parent, NULL, false);
+       if (unlikely(!valid)) {
+               err = -ESTALE;
+               goto out;
+       }
+       valid = __unionfs_d_revalidate_one_locked(dentry, NULL, false);
+       if (unlikely(!valid)) {
                err = -ESTALE;
                goto out;
        }
@@ -144,6 +152,7 @@ out:
                /* update parent dir times if unlink/whiteout succeeded */
                unionfs_copy_attr_times(dir);
        }
+       unionfs_unlock_dentry(dentry->d_parent);
        unionfs_unlock_dentry(dentry);
        unionfs_read_unlock(dentry->d_sb);
        return err;