From: "Serge E. Hallyn" Quoting Frederik Deweerdt (deweerdt@free.fr): > On Thu, Jan 04, 2007 at 12:12:15PM -0600, Serge E. Hallyn wrote: > > diff --git a/fs/namespace.c b/fs/namespace.c > > index 5da87e2..a4039a3 100644 > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -708,8 +708,9 @@ struct vfsmount *copy_tree(struct vfsmou > > return NULL; > > > > res = q = clone_mnt(mnt, dentry, flag); > > - if (!q) > > - goto Enomem; > > + if (!q || IS_ERR(q)) { > > + return q; > > + } > > q->mnt_mountpoint = mnt->mnt_mountpoint; > > > > p = mnt; > > @@ -730,8 +731,9 @@ struct vfsmount *copy_tree(struct vfsmou > > nd.mnt = q; > > nd.dentry = p->mnt_mountpoint; > > q = clone_mnt(p, p->mnt_root, flag); > > - if (!q) > > - goto Enomem; > > + if (!q || IS_ERR(q)) { > > + goto Error; > > + } > > spin_lock(&vfsmount_lock); > > list_add_tail(&q->mnt_list, &res->mnt_list); > > attach_mnt(q, &nd); > > @@ -739,7 +741,7 @@ struct vfsmount *copy_tree(struct vfsmou > > } > > } > > return res; > > -Enomem: > > +Error: > > if (res) { > ^^^^^^^^^^ > I think that this check can be safely skiped, as res is always non-NULL > when Error: is now reached, isn't it? Good point. In addition, we can clean the code up a little by not letting clone_mnt return NULL. Compile-tested patch follows. Though really it starts to look like clone_mnt should be split into two parts, with the original clone_mnt static inline, and the new clone_mnt doing the permission check and return value conversion... From: Serge E. Hallyn Subject: [PATCH 1/1] userns: cleanups with clone_mnt As Frederik Deweerdt points out, the Error: case in copy_tree() can no longer have res==NULL, so remove that check. Also make clone_mnt always return -ENOMEM in place of NULL. Signed-off-by: Serge E. Hallyn Cc: Herbert Poetzl Cc: Kirill Korotaev Cc: "Eric W. Biederman" Signed-off-by: Andrew Morton --- fs/namespace.c | 80 +++++++++++++++++++++++------------------------ 1 files changed, 40 insertions(+), 40 deletions(-) diff -puN fs/namespace.c~user-ns-prepare-copy_tree-copy_mnt-and-their-callers-to-handle-errs-fix fs/namespace.c --- a/fs/namespace.c~user-ns-prepare-copy_tree-copy_mnt-and-their-callers-to-handle-errs-fix +++ a/fs/namespace.c @@ -236,37 +236,39 @@ static struct vfsmount *clone_mnt(struct struct super_block *sb = old->mnt_sb; struct vfsmount *mnt = alloc_vfsmnt(old->mnt_devname); - if (mnt) { - mnt->mnt_flags = old->mnt_flags; - atomic_inc(&sb->s_active); - mnt->mnt_sb = sb; - mnt->mnt_root = dget(root); - mnt->mnt_mountpoint = mnt->mnt_root; - mnt->mnt_parent = mnt; - - if (flag & CL_SLAVE) { - list_add(&mnt->mnt_slave, &old->mnt_slave_list); - mnt->mnt_master = old; - CLEAR_MNT_SHARED(mnt); - } else { - if ((flag & CL_PROPAGATION) || IS_MNT_SHARED(old)) - list_add(&mnt->mnt_share, &old->mnt_share); - if (IS_MNT_SLAVE(old)) - list_add(&mnt->mnt_slave, &old->mnt_slave); - mnt->mnt_master = old->mnt_master; - } - if (flag & CL_MAKE_SHARED) - set_mnt_shared(mnt); + if (!mnt) + return ERR_PTR(-ENOMEM); - /* stick the duplicate mount on the same expiry list - * as the original if that was on one */ - if (flag & CL_EXPIRE) { - spin_lock(&vfsmount_lock); - if (!list_empty(&old->mnt_expire)) - list_add(&mnt->mnt_expire, &old->mnt_expire); - spin_unlock(&vfsmount_lock); - } + mnt->mnt_flags = old->mnt_flags; + atomic_inc(&sb->s_active); + mnt->mnt_sb = sb; + mnt->mnt_root = dget(root); + mnt->mnt_mountpoint = mnt->mnt_root; + mnt->mnt_parent = mnt; + + if (flag & CL_SLAVE) { + list_add(&mnt->mnt_slave, &old->mnt_slave_list); + mnt->mnt_master = old; + CLEAR_MNT_SHARED(mnt); + } else { + if ((flag & CL_PROPAGATION) || IS_MNT_SHARED(old)) + list_add(&mnt->mnt_share, &old->mnt_share); + if (IS_MNT_SLAVE(old)) + list_add(&mnt->mnt_slave, &old->mnt_slave); + mnt->mnt_master = old->mnt_master; + } + if (flag & CL_MAKE_SHARED) + set_mnt_shared(mnt); + + /* stick the duplicate mount on the same expiry list + * as the original if that was on one */ + if (flag & CL_EXPIRE) { + spin_lock(&vfsmount_lock); + if (!list_empty(&old->mnt_expire)) + list_add(&mnt->mnt_expire, &old->mnt_expire); + spin_unlock(&vfsmount_lock); } + return mnt; } @@ -703,14 +705,15 @@ struct vfsmount *copy_tree(struct vfsmou { struct vfsmount *res, *p, *q, *r, *s; struct nameidata nd; + LIST_HEAD(umount_list); if (!(flag & CL_COPY_ALL) && IS_MNT_UNBINDABLE(mnt)) return NULL; res = q = clone_mnt(mnt, dentry, flag); - if (!q || IS_ERR(q)) { + if (IS_ERR(q)) return q; - } + q->mnt_mountpoint = mnt->mnt_mountpoint; p = mnt; @@ -731,9 +734,8 @@ struct vfsmount *copy_tree(struct vfsmou nd.mnt = q; nd.dentry = p->mnt_mountpoint; q = clone_mnt(p, p->mnt_root, flag); - if (!q || IS_ERR(q)) { + if (IS_ERR(q)) goto Error; - } spin_lock(&vfsmount_lock); list_add_tail(&q->mnt_list, &res->mnt_list); attach_mnt(q, &nd); @@ -742,13 +744,11 @@ struct vfsmount *copy_tree(struct vfsmou } return res; Error: - if (res) { - LIST_HEAD(umount_list); - spin_lock(&vfsmount_lock); - umount_tree(res, 0, &umount_list); - spin_unlock(&vfsmount_lock); - release_mounts(&umount_list); - } + spin_lock(&vfsmount_lock); + umount_tree(res, 0, &umount_list); + spin_unlock(&vfsmount_lock); + release_mounts(&umount_list); + return q; } _