Re: [RFC 16/26] union-mount: Introduce union_mount structure

[Bharata B Rao]

On Mon, Jul 30, 2007 at 06:13:39PM +0200, Jan Blunck wrote:
> +
> +int append_to_union(struct vfsmount *mnt, struct dentry *dentry,
> +		    struct vfsmount *dest_mnt, struct dentry *dest_dentry)
> +{
> +	struct union_mount *this, *um;
> +
> +	BUG_ON(!IS_MNT_UNION(mnt));
> +
> +	this = union_alloc(dentry, mnt, dest_dentry, dest_mnt);
> +	if (!this)
> +		return -ENOMEM;
> +
> +	spin_lock(&union_lock);
> +	um = union_lookup(dentry, mnt);
> +	if (um) {
> +		BUG_ON((um->u_next.dentry != dest_dentry) ||
> +		       (um->u_next.mnt != dest_mnt));
> +		spin_unlock(&union_lock);
> +		union_put(this);
> +		return 0;
> +	}
> +	__union_hash(this);
> +	spin_unlock(&union_lock);
> +	return 0;
> +}

This breaks if we append to union stack from outside of the union.
A particular case I hit is with a 3 layer union with a subdir union
between topmost and bottom layer. Now if you create the same-named
directory in the middle layer from outside of this union, you hit the
above BUG_ON. The below patch fixes this and it applies on top of all of
your patches.

From: Bharata B Rao <bharata@xxxxxxxxxxxxxxxxxx>

Direct additions to union stack from outside of the union is resulting in
BUG_ON. But this is a valid case and hence needs to be supported. Modify
append_to_union() to correctly handle this case.

Signed-off-by: Bharata B Rao <bharata@xxxxxxxxxxxxxxxxxx>
---
 fs/namei.c            |    8 +++++---
 fs/union.c            |   32 ++++++++++++++++++++++++--------
 include/linux/union.h |    4 ++--
 3 files changed, 31 insertions(+), 13 deletions(-)

--- a/fs/namei.c
+++ b/fs/namei.c
@@ -512,7 +512,7 @@ static int __cache_lookup_union(struct n
 		}
 
 		/* now we know we found something "real"  */
-		append_to_union(last.mnt, last.dentry, nd->mnt, dentry);
+		append_to_union(last.mnt, last.dentry, nd->mnt, dentry, 1);
 
 		if (last.dentry != path->dentry)
 			pathput(&last);
@@ -789,7 +789,8 @@ static int __real_lookup_union(struct na
 		}
 
 		/* now we know we found something "real" */
-		append_to_union(last.mnt, last.dentry, next.mnt, next.dentry);
+		append_to_union(last.mnt, last.dentry,
+				next.mnt, next.dentry, 1);
 
 		if (last.dentry != path->dentry)
 			pathput(&last);
@@ -1775,7 +1776,8 @@ static int __hash_lookup_union(struct na
 		}
 
 		/* now we know we found something "real" */
-		append_to_union(last.mnt, last.dentry, next.mnt, next.dentry);
+		append_to_union(last.mnt, last.dentry,
+				next.mnt, next.dentry, 1);
 
 		if (last.dentry != path->dentry)
 			pathput(&last);
--- a/fs/union.c
+++ b/fs/union.c
@@ -248,7 +248,8 @@ int is_unionized(struct dentry *dentry, 
 }
 
 int append_to_union(struct vfsmount *mnt, struct dentry *dentry,
-		    struct vfsmount *dest_mnt, struct dentry *dest_dentry)
+		    struct vfsmount *dest_mnt, struct dentry *dest_dentry,
+		    int from_lookup)
 {
 	struct union_mount *this, *um;
 
@@ -264,11 +265,26 @@ int append_to_union(struct vfsmount *mnt
 	spin_lock(&union_lock);
 	um = union_lookup(dentry, mnt);
 	if (um) {
-		BUG_ON((um->u_next.dentry != dest_dentry) ||
-		       (um->u_next.mnt != dest_mnt));
-		spin_unlock(&union_lock);
-		union_put(this);
-		return 0;
+		if (um->u_next.dentry == dest_dentry &&
+				um->u_next.mnt == dest_mnt) {
+			spin_unlock(&union_lock);
+			union_put(this);
+			return 0;
+		}
+		if (from_lookup) {
+			__union_unhash(um);
+			list_del(&um->u_list);
+			list_del(&um->u_unions);
+			um->u_next.dentry->d_unionized--;
+			spin_unlock(&union_lock);
+			union_put(um);
+			spin_lock(&union_lock);
+		} else {
+			BUG();
+			spin_unlock(&union_lock);
+			union_put(this);
+			return 0;
+		}
 	}
 	list_add(&this->u_list, &mnt->mnt_unions);
 	list_add(&this->u_unions, &dentry->d_unions);
@@ -451,7 +467,7 @@ int attach_mnt_union(struct vfsmount *mn
 	if (!IS_MNT_UNION(mnt))
 		return 0;
 
-	return append_to_union(mnt, mnt->mnt_root, dest_mnt, dest_dentry);
+	return append_to_union(mnt, mnt->mnt_root, dest_mnt, dest_dentry, 0);
 }
 
 void detach_mnt_union(struct vfsmount *mnt)
@@ -941,7 +957,7 @@ struct dentry *union_create_topmost(stru
 		UM_DEBUG_DENTRY(dentry);
 
 		res = append_to_union(nd->mnt, dentry, path->mnt,
-				      path->dentry);
+				      path->dentry, 0);
 		if (res) {
 			dput(dentry);
 			dentry = ERR_PTR(res);
--- a/include/linux/union.h
+++ b/include/linux/union.h
@@ -44,7 +44,7 @@ struct union_mount {
 
 extern int is_unionized(struct dentry *, struct vfsmount *);
 extern int append_to_union(struct vfsmount *, struct dentry *,
-			   struct vfsmount *, struct dentry *);
+			   struct vfsmount *, struct dentry *, int);
 extern int follow_union_down(struct vfsmount **, struct dentry **);
 extern int follow_union_mount(struct vfsmount **, struct dentry **);
 extern void __d_drop_unions(struct dentry *);
@@ -65,7 +65,7 @@ extern int union_copyup(struct nameidata
 #define IS_UNION(x)			(0)
 #define IS_MNT_UNION(x)			(0)
 #define is_unionized(x, y)		(0)
-#define append_to_union(x1, y1, x2, y2)	({ BUG(); (0); })
+#define append_to_union(x1, y1, x2, y2, z)	({ BUG(); (0); })
 #define follow_union_down(x, y)		({ (0); })
 #define follow_union_mount(x, y)	({ (0); })
 #define __d_drop_unions(x)		do { } while (0)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/