re: cgroup: introduce cgroup namespaces

re: cgroup: introduce cgroup namespaces

[Dan Carpenter]

Hello Aditya Kali,

The patch a79a908fd2b0: "cgroup: introduce cgroup namespaces" from
Jan 29, 2016, leads to the following static checker warning:

	kernel/cgroup.c:6091 copy_cgroup_ns()
	error: 'new_ns' dereferencing possible ERR_PTR()

kernel/cgroup.c
  6085  
  6086          err = -ENOMEM;
  6087          new_ns = alloc_cgroup_ns();
  6088          if (!new_ns)
                    ^^^^^^^
This should be an IS_ERR() check.

  6089                  goto err_out;
  6090  
  6091          new_ns->user_ns = get_user_ns(user_ns);

regards,
dan carpenter

[PATCH cgroup/for-4.5-fixes] cgroup: fix alloc_cgroup_ns() error handling in copy_cgroup_ns()

[Tejun Heo]

From d22025570e2ebfc68819b35c5d457e53d9337217 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Thu, 18 Feb 2016 11:44:24 -0500

alloc_cgroup_ns() returns an ERR_PTR value on error but
copy_cgroup_ns() was checking for NULL for error.  Fix it.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
Hello, Dan.

Applied to cgroup/for-4.5-fixes.

Thanks.

 kernel/cgroup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index afb1205..d92d91a 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -6083,10 +6083,11 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
 	spin_unlock_bh(&css_set_lock);
 	mutex_unlock(&cgroup_mutex);
 
-	err = -ENOMEM;
 	new_ns = alloc_cgroup_ns();
-	if (!new_ns)
+	if (IS_ERR(new_ns)) {
+		err = PTR_ERR(new_ns);
 		goto err_out;
+	}
 
 	new_ns->user_ns = get_user_ns(user_ns);
 	new_ns->root_cset = cset;
-- 
2.5.0

Re: [PATCH cgroup/for-4.5-fixes] cgroup: fix alloc_cgroup_ns() error handling in copy_cgroup_ns()

[Aditya Kali]

Thanks for catching.

On Thu, Feb 18, 2016 at 8:46 AM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> From d22025570e2ebfc68819b35c5d457e53d9337217 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Thu, 18 Feb 2016 11:44:24 -0500
>
> alloc_cgroup_ns() returns an ERR_PTR value on error but
> copy_cgroup_ns() was checking for NULL for error.  Fix it.
>
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Acked-by: Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org)

> ---
> Hello, Dan.
>
> Applied to cgroup/for-4.5-fixes.
>
> Thanks.
>
>  kernel/cgroup.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index afb1205..d92d91a 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -6083,10 +6083,11 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
>         spin_unlock_bh(&css_set_lock);
>         mutex_unlock(&cgroup_mutex);
>
> -       err = -ENOMEM;
>         new_ns = alloc_cgroup_ns();
> -       if (!new_ns)
> +       if (IS_ERR(new_ns)) {
> +               err = PTR_ERR(new_ns);
>                 goto err_out;
> +       }
>
>         new_ns->user_ns = get_user_ns(user_ns);
>         new_ns->root_cset = cset;
> --
> 2.5.0
>



-- 
Aditya

Re: [PATCH cgroup/for-4.5-fixes] cgroup: fix alloc_cgroup_ns() error handling in copy_cgroup_ns()

[Dan Carpenter]

Sorry, I was feeling like a jerk yesterday.

I obviously could have fixed this myself but I wanted to make a point
about one error label, do-everything style "future proof" error
handling.  It is a trap for the unwary.

Now the code calls kfree(-ENOMEM).  It's a very predictable mistake.

regards,
dan carpenter

Re: [PATCH cgroup/for-4.5-fixes] cgroup: fix alloc_cgroup_ns() error handling in copy_cgroup_ns()

[Tejun Heo]

On Thu, Feb 18, 2016 at 11:21:12PM +0300, Dan Carpenter wrote:
> Sorry, I was feeling like a jerk yesterday.
> 
> I obviously could have fixed this myself but I wanted to make a point
> about one error label, do-everything style "future proof" error
> handling.  It is a trap for the unwary.
> 
> Now the code calls kfree(-ENOMEM).  It's a very predictable mistake.

lol let's take the chance and convert the NULL check in kfree() to
IS_ERR_OR_NULL().

Seriously tho, I don't think this is a good evidence against merged
error handling.  It's not like we don't make repeated mistakes with
split error labels.  Anyways, care to submit a proper patch?

Thanks.

-- 
tejun

Re: [PATCH cgroup/for-4.5-fixes] cgroup: fix alloc_cgroup_ns() error handling in copy_cgroup_ns()

[Dan Carpenter]

I'm not sure what to write the patch against now.  Can you just do this?

regards,
dan carpenter

[PATCH cgroup/for-4.6-ns] cgroup: fix and restructure error handling in copy_cgroup_ns()

[Tejun Heo]

copy_cgroup_ns()'s error handling was broken and the attempt to fix it
d22025570e2e ("cgroup: fix alloc_cgroup_ns() error handling in
copy_cgroup_ns()") was broken too in that it ended up trying an
ERR_PTR() value.

There's only one place where copy_cgroup_ns() needs to perform cleanup
after failure.  Simplify and fix the error handling by removing the
goto's.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index d92d91a..2c88149 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -6058,9 +6058,8 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
 					struct user_namespace *user_ns,
 					struct cgroup_namespace *old_ns)
 {
-	struct cgroup_namespace *new_ns = NULL;
-	struct css_set *cset = NULL;
-	int err;
+	struct cgroup_namespace *new_ns;
+	struct css_set *cset;
 
 	BUG_ON(!old_ns);
 
@@ -6070,9 +6069,8 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
 	}
 
 	/* Allow only sysadmin to create cgroup namespace. */
-	err = -EPERM;
 	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
-		goto err_out;
+		return ERR_PTR(-EPERM);
 
 	mutex_lock(&cgroup_mutex);
 	spin_lock_bh(&css_set_lock);
@@ -6085,20 +6083,14 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
 
 	new_ns = alloc_cgroup_ns();
 	if (IS_ERR(new_ns)) {
-		err = PTR_ERR(new_ns);
-		goto err_out;
+		put_css_set(cset);
+		return new_ns;
 	}
 
 	new_ns->user_ns = get_user_ns(user_ns);
 	new_ns->root_cset = cset;
 
 	return new_ns;
-
-err_out:
-	if (cset)
-		put_css_set(cset);
-	kfree(new_ns);
-	return ERR_PTR(err);
 }
 
 static inline struct cgroup_namespace *to_cg_ns(struct ns_common *ns)

Re: [PATCH cgroup/for-4.6-ns] cgroup: fix and restructure error handling in copy_cgroup_ns()

[Serge Hallyn]

Quoting Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org):
> copy_cgroup_ns()'s error handling was broken and the attempt to fix it
> d22025570e2e ("cgroup: fix alloc_cgroup_ns() error handling in
> copy_cgroup_ns()") was broken too in that it ended up trying an
> ERR_PTR() value.
> 
> There's only one place where copy_cgroup_ns() needs to perform cleanup
> after failure.  Simplify and fix the error handling by removing the
> goto's.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

It certainly looks cleaner, thanks.

Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

> ---
>  kernel/cgroup.c |   18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index d92d91a..2c88149 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -6058,9 +6058,8 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
>  					struct user_namespace *user_ns,
>  					struct cgroup_namespace *old_ns)
>  {
> -	struct cgroup_namespace *new_ns = NULL;
> -	struct css_set *cset = NULL;
> -	int err;
> +	struct cgroup_namespace *new_ns;
> +	struct css_set *cset;
>  
>  	BUG_ON(!old_ns);
>  
> @@ -6070,9 +6069,8 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
>  	}
>  
>  	/* Allow only sysadmin to create cgroup namespace. */
> -	err = -EPERM;
>  	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
> -		goto err_out;
> +		return ERR_PTR(-EPERM);
>  
>  	mutex_lock(&cgroup_mutex);
>  	spin_lock_bh(&css_set_lock);
> @@ -6085,20 +6083,14 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
>  
>  	new_ns = alloc_cgroup_ns();
>  	if (IS_ERR(new_ns)) {
> -		err = PTR_ERR(new_ns);
> -		goto err_out;
> +		put_css_set(cset);
> +		return new_ns;
>  	}
>  
>  	new_ns->user_ns = get_user_ns(user_ns);
>  	new_ns->root_cset = cset;
>  
>  	return new_ns;
> -
> -err_out:
> -	if (cset)
> -		put_css_set(cset);
> -	kfree(new_ns);
> -	return ERR_PTR(err);
>  }
>  
>  static inline struct cgroup_namespace *to_cg_ns(struct ns_common *ns)

Re: [PATCH cgroup/for-4.6-ns] cgroup: fix and restructure error handling in copy_cgroup_ns()

[Tejun Heo]

On Mon, Feb 29, 2016 at 08:50:00AM +0000, Serge Hallyn wrote:
> Quoting Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org):
> > copy_cgroup_ns()'s error handling was broken and the attempt to fix it
> > d22025570e2e ("cgroup: fix alloc_cgroup_ns() error handling in
> > copy_cgroup_ns()") was broken too in that it ended up trying an
> > ERR_PTR() value.
> > 
> > There's only one place where copy_cgroup_ns() needs to perform cleanup
> > after failure.  Simplify and fix the error handling by removing the
> > goto's.
> > 
> > Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> It certainly looks cleaner, thanks.
> 
> Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-4.6-ns.

Thanks.

-- 
tejun