| 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 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <E1P1FnW-0001yC-Ts@pomaz-ex.szeredi.hu>
Date: Thu, 30 Sep 2010 11:51:30 +0200
From: Miklos Szeredi <miklos@...redi.hu>
To: Valerie Aurora <vaurora@...hat.com>
CC: viro@...iv.linux.org.uk, miklos@...redi.hu, hch@...radead.org,
agruen@...e.de, npiggin@...nel.dk, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, vaurora@...hat.com
Subject: Re: [PATCH 01/34] VFS: Make clone_mnt() and copy_tree() return error codes
On Thu, 16 Sep 2010, Valerie Aurora wrote:
> copy_tree() can theoretically fail in a case other than ENOMEM, but
> always returns NULL which is interpreted by callers as -ENOMEM.
> Convert to return an explicit error. Convert clone_mnt() for
> consistency and because union mounts will add new error cases.
I think it makes sense to push this fix to 2.6.37 independently of the
other patches.
Acked-by: Miklos Szeredi <mszeredi@...e.cz>
>
> Signed-off-by: Valerie Aurora <vaurora@...hat.com>
> ---
> fs/namespace.c | 111 ++++++++++++++++++++++++++++++--------------------------
> fs/pnode.c | 5 ++-
> 2 files changed, 63 insertions(+), 53 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index e1ea335..5566524 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -559,53 +559,57 @@ static struct vfsmount *clone_mnt(struct vfsmount *old, struct dentry *root,
> int flag)
> {
> struct super_block *sb = old->mnt_sb;
> - struct vfsmount *mnt = alloc_vfsmnt(old->mnt_devname);
> + struct vfsmount *mnt;
> + int err;
>
> - if (mnt) {
> - if (flag & (CL_SLAVE | CL_PRIVATE))
> - mnt->mnt_group_id = 0; /* not a peer of original */
> - else
> - mnt->mnt_group_id = old->mnt_group_id;
> -
> - if ((flag & CL_MAKE_SHARED) && !mnt->mnt_group_id) {
> - int err = mnt_alloc_group_id(mnt);
> - if (err)
> - goto out_free;
> - }
> + mnt = alloc_vfsmnt(old->mnt_devname);
> + if (!mnt)
> + return ERR_PTR(-ENOMEM);
>
> - 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_PRIVATE)) {
> - if ((flag & CL_MAKE_SHARED) || 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) {
> - if (!list_empty(&old->mnt_expire))
> - list_add(&mnt->mnt_expire, &old->mnt_expire);
> - }
> + if (flag & (CL_SLAVE | CL_PRIVATE))
> + mnt->mnt_group_id = 0; /* not a peer of original */
> + else
> + mnt->mnt_group_id = old->mnt_group_id;
> +
> + if ((flag & CL_MAKE_SHARED) && !mnt->mnt_group_id) {
> + err = mnt_alloc_group_id(mnt);
> + if (err)
> + goto out_free;
> }
> +
> + 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_PRIVATE)) {
> + if ((flag & CL_MAKE_SHARED) || 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) {
> + if (!list_empty(&old->mnt_expire))
> + list_add(&mnt->mnt_expire, &old->mnt_expire);
> + }
> +
> return mnt;
>
> out_free:
> free_vfsmnt(mnt);
> - return NULL;
> + return ERR_PTR(err);
> }
>
> static inline void __mntput(struct vfsmount *mnt)
> @@ -1212,11 +1216,12 @@ struct vfsmount *copy_tree(struct vfsmount *mnt, struct dentry *dentry,
> struct path path;
>
> if (!(flag & CL_COPY_ALL) && IS_MNT_UNBINDABLE(mnt))
> - return NULL;
> + return ERR_PTR(-EINVAL);
>
> res = q = clone_mnt(mnt, dentry, flag);
> - if (!q)
> - goto Enomem;
> + if (IS_ERR(q))
> + return q;
> +
> q->mnt_mountpoint = mnt->mnt_mountpoint;
>
> p = mnt;
> @@ -1237,8 +1242,8 @@ struct vfsmount *copy_tree(struct vfsmount *mnt, struct dentry *dentry,
> path.mnt = q;
> path.dentry = p->mnt_mountpoint;
> q = clone_mnt(p, p->mnt_root, flag);
> - if (!q)
> - goto Enomem;
> + if (IS_ERR(q))
> + goto out;
> spin_lock(&vfsmount_lock);
> list_add_tail(&q->mnt_list, &res->mnt_list);
> attach_mnt(q, &path);
> @@ -1246,7 +1251,7 @@ struct vfsmount *copy_tree(struct vfsmount *mnt, struct dentry *dentry,
> }
> }
> return res;
> -Enomem:
> +out:
> if (res) {
> LIST_HEAD(umount_list);
> spin_lock(&vfsmount_lock);
> @@ -1254,9 +1259,11 @@ Enomem:
> spin_unlock(&vfsmount_lock);
> release_mounts(&umount_list);
> }
> - return NULL;
> + return q;
> }
>
> +/* Caller should check returned pointer for errors */
> +
> struct vfsmount *collect_mounts(struct path *path)
> {
> struct vfsmount *tree;
> @@ -1529,14 +1536,15 @@ static int do_loopback(struct path *path, char *old_name,
> if (!check_mnt(path->mnt) || !check_mnt(old_path.mnt))
> goto out;
>
> - err = -ENOMEM;
> if (recurse)
> mnt = copy_tree(old_path.mnt, old_path.dentry, 0);
> else
> mnt = clone_mnt(old_path.mnt, old_path.dentry, 0);
>
> - if (!mnt)
> + if (IS_ERR(mnt)) {
> + err = PTR_ERR(mnt);
> goto out;
> + }
>
> err = graft_tree(mnt, path);
> if (err) {
> @@ -2071,10 +2079,11 @@ static struct mnt_namespace *dup_mnt_ns(struct mnt_namespace *mnt_ns,
> /* First pass: copy the tree topology */
> new_ns->root = copy_tree(mnt_ns->root, mnt_ns->root->mnt_root,
> CL_COPY_ALL | CL_EXPIRE);
> - if (!new_ns->root) {
> + if (IS_ERR(new_ns->root)) {
> + int err = PTR_ERR(new_ns->root);
> up_write(&namespace_sem);
> kfree(new_ns);
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(err);
> }
> spin_lock(&vfsmount_lock);
> list_add_tail(&new_ns->list, &new_ns->root->mnt_list);
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 5cc564a..c4358d2 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -250,8 +250,9 @@ int propagate_mnt(struct vfsmount *dest_mnt, struct dentry *dest_dentry,
>
> source = get_source(m, prev_dest_mnt, prev_src_mnt, &type);
>
> - if (!(child = copy_tree(source, source->mnt_root, type))) {
> - ret = -ENOMEM;
> + child = copy_tree(source, source->mnt_root, type);
> + if (IS_ERR(child)) {
> + ret = PTR_ERR(child);
> list_splice(tree_list, tmp_list.prev);
> goto out;
> }
> --
> 1.6.3.3
>
>
--
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