2007-08-06 05:57:39

by Bharata B Rao

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

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 <[email protected]>

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 <[email protected]>
---
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)