[Kernel-janitors] [PATCH] - fs/namespace.c::copy_namespace()

[Kernel-janitors] [PATCH] - fs/namespace.c::copy_namespace()

[Luiz Fernando N. Capitulino]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]


 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
--- a/fs/namespace.c	2004-05-10 13:36:46.000000000 -0300
+++ a~/fs/namespace.c	2004-05-12 16:13:22.000000000 -0300
@@ -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>

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] - fs/namespace.c::copy_namespace()

[Michael Still]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

Luiz Fernando N. Capitulino wrote:

>  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. :-)

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 (mikal@stillhq.com) | "All my life I've had one dream,
http://www.stillhq.com            |  to achieve my many goals"
UTC + 10                          |    -- Homer Simpson

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] - fs/namespace.c::copy_namespace()

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]

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

Might work a bit better like this:

> diff -X dontdiff -Nparu a/fs/namespace.c a~/fs/namespace.c
> --- a/fs/namespace.c	2004-05-10 13:36:46.000000000 -0300
> +++ a~/fs/namespace.c	2004-05-12 16:13:22.000000000 -0300
> @@ -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;

	int rc;

>  
>  	if (!namespace)
> -		return 0;
> +		return rc;

Don't change.

>  
>  	get_namespace(namespace);
>  
>  	if (!(flags & CLONE_NEWNS))
> -		return 0;
> +		return rc;

Don't change.

>  	if (!capable(CAP_SYS_ADMIN)) {
> -		put_namespace(namespace);
> -		return -EPERM;
> +		rc = -EPERM;
> +		goto out;
>  	}

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

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

	new_ns = kmalloc(sizeof(struct namespace), GFP_KERNEL);
	rc = -ENOMEM;
	if (!new_ns)
		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;

... can delete.

>  		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;
>  }

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

-- 
"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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: Re: [Kernel-janitors] [PATCH] - fs/namespace.c::copy_namesp

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 3863 bytes --]

On Tue, May 18, 2004 at 04:54:32PM +0200, Walter Harms wrote:
> Hi Willy,
> first i would do 
>           if (!capable(CAP_SYS_ADMIN)) 
> 
> So its harder to leak any information 
> (if later programmers fail somewhere)	
> 
> there i would change. any reason not to do ?

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
--- a/fs/namespace.c    28 Apr 2004 19:12:44 -0000      1.8
+++ b/fs/namespace.c    18 May 2004 15:43:51 -0000
@@ -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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] - fs/namespace.c::copy_namespace()

[Luiz Fernando N. Capitulino]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

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>

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors