- fs/namespace.c::copy_namespace()

Hi!

This patch is a little clean up, maybe it is the kind of
thing which Randy could say: "is better work on important things". But it is impossible to read this code without patch it. :-)

put_namespace() is called four in different places, and the
goto is a little useless in that context. This patch puts the things toghether.

 fs/namespace.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff -X dontdiff -Nparu a/fs/namespace.c a~/fs/namespace.c
@@ -868,23 +868,26 @@ int copy_namespace(int flags, struct tas
 	struct namespace *new_ns;
 	struct vfsmount *rootmnt = NULL, *pwdmnt = NULL, *altrootmnt = NULL;
 	struct fs_struct *fs = tsk->fs;
+	int rc = 0;
 
 	if (!namespace)
-		return 0;
+		return rc;
 
 	get_namespace(namespace);
 
 	if (!(flags & CLONE_NEWNS))
-		return 0;
+		return rc;
 
 	if (!capable(CAP_SYS_ADMIN)) {
-		put_namespace(namespace);
-		return -EPERM;
+		rc = -EPERM;
+		goto out;
 	}
 
 	new_ns = kmalloc(sizeof(struct namespace), GFP_KERNEL);
-	if (!new_ns)
+	if (!new_ns) {
+		rc = -ENOMEM;
 		goto out;
+	}
 
 	atomic_set(&new_ns->count, 1);
 	init_rwsem(&new_ns->sem);

@@ -896,6 +899,7 @@ int copy_namespace(int flags, struct tas
 	if (!new_ns->root) {
 		up_write(&tsk->namespace->sem);
 		kfree(new_ns);
+		rc = -ENOMEM;
 		goto out;
 	}
 	spin_lock(&vfsmount_lock);

@@ -938,12 +942,9 @@ int copy_namespace(int flags, struct tas
 	if (altrootmnt)
 		mntput(altrootmnt);
 
-	put_namespace(namespace);
-	return 0;
-
 out:
 	put_namespace(namespace);
-	return -ENOMEM;
+	return rc;
 }
 
 asmlinkage long sys_mount(char __user * dev_name, char __user * dir_name,

Luiz Fernando N. Capitulino
<http://www.telecentros.sp.gov.br>

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

I personally find the first version more readable -- now I need to look up to the top of the function to see the value of rc which is being returned.

Mikal

Michael Still ([email protected]) | "All my life I've had one dream,
http://www.stillhq.com            |  to achieve my many goals"
UTC + 10                          |    -- Homer Simpson

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

On Mon, May 17, 2004 at 08:23:35PM -0300, Luiz Fernando N. Capitulino wrote:

Might work a bit better like this:

	int rc;

	rc = -EPERM;
	if (!capable(CAP_SYS_ADMIN))
		goto out;

	new_ns = kmalloc(sizeof(struct namespace), GFP_KERNEL);
	rc = -ENOMEM;
	if (!new_ns)
		goto out;

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Because copy_namespace doesn't need CAP_SYS_ADMIN if CLONE_NEWNS is clear. Anyway, what information is leaked? Here's the beginning of the function:

int copy_namespace(int flags, struct task_struct *tsk)

{
        struct namespace *namespace = tsk->namespace;
        struct namespace *new_ns;
        struct vfsmount *rootmnt = NULL, *pwdmnt = NULL, *altrootmnt = NULL;
        struct fs_struct *fs = tsk->fs;

        if (!namespace)
                return 0;

        get_namespace(namespace);

        if (!(flags & CLONE_NEWNS))
                return 0;

        if (!capable(CAP_SYS_ADMIN)) {
                put_namespace(namespace);
                return -EPERM;
        }

get_namespace and put_namespace handle refcounting. Without refcounting, this is what it looks like:

        if (!namespace) 
                return 0;
        if (!(flags & CLONE_NEWNS))
                return 0;
        if (!capable(CAP_SYS_ADMIN))
		return -EPERM;

Perhaps the conceptual problem here is that copy_namespace does different things depending on the flags. Maybe this should be:

Index: fs/namespace.c
===================================================================
RCS file: /var/cvs/linux-2.6/fs/namespace.c,v
retrieving revision 1.8

diff -u -p -r1.8 namespace.c
@@ -803,25 +803,13 @@ dput_out:
        return retval;
 }
 
-int copy_namespace(int flags, struct task_struct *tsk)
+static int new_namespace(int flags, struct task_struct *tsk)
 {
        struct namespace *namespace = tsk->namespace;
        struct namespace *new_ns;
        struct vfsmount *rootmnt = NULL, *pwdmnt = NULL, *altrootmnt = NULL;
        struct fs_struct *fs = tsk->fs;
-
-       if (!namespace)
-               return 0;
-
-       get_namespace(namespace);
-
-       if (!(flags & CLONE_NEWNS))
-               return 0;
-
-       if (!capable(CAP_SYS_ADMIN)) {
-               put_namespace(namespace);
-               return -EPERM;
-       }
+       int result = -ENOMEM;
 
        new_ns = kmalloc(sizeof(struct namespace), GFP_KERNEL);
        if (!new_ns)

@@ -879,12 +867,32 @@ int copy_namespace(int flags, struct tas
        if (altrootmnt)
                mntput(altrootmnt);
 
-       put_namespace(namespace);
-       return 0;
+       result = 0;
 
-out:
+ out:
        put_namespace(namespace);
-       return -ENOMEM;
+       return result;
+}
+
+int copy_namespace(int flags, struct task_struct *tsk)
+{
+       struct namespace *namespace = tsk->namespace;
+       int result;
+
+       if (!namespace)
+               return 0;
+
+       get_namespace(namespace);
+
+       if (!(flags & CLONE_NEWNS))
+               return 0;
+
+       if (!capable(CAP_SYS_ADMIN)) {
+               put_namespace(namespace);
+               return -EPERM;
+       }
+
+       return new_namespace(flags, tsk);
 }
 
 asmlinkage long sys_mount(char __user * dev_name, char __user * dir_name,

Untested, but should hopefully convey better that copy_namespace does two different things, and to do the second one (create a new namespace), you have to be root.

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Em Tue, May 18, 2004 at 02:03:30PM +0100, Matthew Wilcox escreveu:

| To be honest, this doesn't really need to be touched, it's pretty | readable as-is.

Ok, lets forget it.

Luiz Fernando N. Capitulino
<http://www.telecentros.sp.gov.br>

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors