[PATCH cgroup/for-4.8] cgroup: allow NULL return from ss->css_alloc()

[PATCH cgroup/for-4.8] cgroup: allow NULL return from ss->css_alloc()

[Tejun Heo]

cgroup core expected css_alloc to return an ERR_PTR value on failure
and caused NULL deref if it returned NULL.  It's an easy mistake to
make from an alloc function and there's no ambiguity in what's being
indicated.  Update css_create() so that it interprets NULL return from
css_alloc as -ENOMEM.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
Hello,

I'm applying this patch to cgroup/for-4.8.

Thanks.

 kernel/cgroup.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 86cb5c6..8e5470d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -5139,6 +5139,8 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
 	lockdep_assert_held(&cgroup_mutex);
 
 	css = ss->css_alloc(parent_css);
+	if (!css)
+		css = ERR_PTR(-ENOMEM);
 	if (IS_ERR(css))
 		return css;
 

Re: [PATCH cgroup/for-4.8] cgroup: allow NULL return from ss->css_alloc()

[Johannes Weiner]

On Tue, Jun 21, 2016 at 01:06:24PM -0400, Tejun Heo wrote:
> cgroup core expected css_alloc to return an ERR_PTR value on failure
> and caused NULL deref if it returned NULL.  It's an easy mistake to
> make from an alloc function and there's no ambiguity in what's being
> indicated.  Update css_create() so that it interprets NULL return from
> css_alloc as -ENOMEM.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>

Would it be better to remove the error code instead and have everybody
return NULL? AFAICS, everybody is returning either the object or the
-ENOMEM error code right now.

What error condition is there for an allocation function?

Re: [PATCH cgroup/for-4.8] cgroup: allow NULL return from ss->css_alloc()

[Tejun Heo]

Hello,

On Tue, Jun 21, 2016 at 02:14:04PM -0400, Johannes Weiner wrote:
> Would it be better to remove the error code instead and have everybody
> return NULL? AFAICS, everybody is returning either the object or the
> -ENOMEM error code right now.
> 
> What error condition is there for an allocation function?

I already changed cgroup core to interpret NULL return as -ENOMEM.
Maybe we can drop ERR_PTR handling altogether in the future but I'm
not sure.  e.g. memcg's id allocation and -ENOSPC check could be in
css_alloc.  The thing is css_online is only necessary if there are
requirements around css id or iteration, so css_alloc can easily be
the only alloc / init point and thus should be able to indicate
different failure conditions.

Thanks.

-- 
tejun