lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 16 Feb 2008 21:57:20 -0500
From:	Erez Zadok <ezk@...sunysb.edu>
To:	akpm@...ux-foundation.org
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	viro@....linux.org.uk, hch@...radead.org,
	Erez Zadok <ezk@...sunysb.edu>
Subject: [PATCH 11/17] 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@...sunysb.edu>
---
 fs/unionfs/commonfops.c |   31 +++++++++++++++++++++---
 fs/unionfs/dentry.c     |   26 +++++++++-----------
 fs/unionfs/inode.c      |   58 +++++++++++++++++++++++++++++++---------------
 fs/unionfs/union.h      |    5 ++-
 fs/unionfs/unlink.c     |   11 ++++++++-
 5 files changed, 91 insertions(+), 40 deletions(-)

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 96473c4..491e2ff 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -300,8 +300,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;
@@ -311,7 +312,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;
 
 	/*
@@ -416,7 +416,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;
 }
 
@@ -524,9 +534,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;
+	}
 
 	file->private_data =
 		kzalloc(sizeof(struct unionfs_file_info), GFP_KERNEL);
@@ -589,6 +608,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;
@@ -797,8 +818,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);
@@ -824,6 +846,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 3bd2dfb..17b297d 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -363,6 +363,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);
@@ -453,7 +454,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);
@@ -491,24 +492,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);
@@ -526,11 +523,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;
 }
@@ -545,6 +541,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;
@@ -574,6 +571,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 6377533..2e791fd 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -175,16 +175,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).
@@ -224,14 +224,13 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
 	unlock_dir(lower_parent_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;
@@ -251,7 +250,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) {
@@ -273,6 +272,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);
 	}
@@ -469,9 +469,15 @@ static int unionfs_symlink(struct inode *parent, struct dentry *dentry,
 
 	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;
 	}
@@ -514,12 +520,12 @@ out:
 	dput(wh_dentry);
 	kfree(name);
 
-	if (!err)
+	if (!err) {
 		unionfs_postcopyup_setmnt(dentry);
-
-	unionfs_check_inode(parent);
-	if (!err)
+		unionfs_check_inode(parent);
 		unionfs_check_dentry(dentry);
+	}
+	unionfs_unlock_dentry(dentry->d_parent);
 	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
@@ -534,12 +540,19 @@ static int unionfs_mkdir(struct inode *parent, struct dentry *dentry, int mode)
 	char *name = NULL;
 	int whiteout_unlinked = 0;
 	struct sioq_args args;
+	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;
 	}
@@ -673,6 +686,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);
 
@@ -691,9 +705,15 @@ static int unionfs_mknod(struct inode *parent, struct dentry *dentry, int 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);
 
+	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;
 	}
@@ -734,12 +754,12 @@ out:
 	dput(wh_dentry);
 	kfree(name);
 
-	if (!err)
+	if (!err) {
 		unionfs_postcopyup_setmnt(dentry);
-
-	unionfs_check_inode(parent);
-	if (!err)
+		unionfs_check_inode(parent);
 		unionfs_check_dentry(dentry);
+	}
+	unionfs_unlock_dentry(dentry->d_parent);
 	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 5001b07..1bf0c09 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -281,11 +281,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);
 	}
 }
 
@@ -352,6 +352,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 1e370a1..6e93da3 100644
--- a/fs/unionfs/unlink.c
+++ b/fs/unionfs/unlink.c
@@ -92,12 +92,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;
 	}
@@ -126,6 +134,7 @@ out:
 		unionfs_check_dentry(dentry);
 		unionfs_check_inode(dir);
 	}
+	unionfs_unlock_dentry(dentry->d_parent);
 	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
-- 
1.5.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists