[PATCH] cgroup: Fix uninitialized variable warning

[PATCH] cgroup: Fix uninitialized variable warning

[Ross Zwisler]

Commit 1f7dd3e5a6e4 ("cgroup: fix handling of multi-destination migration
from subtree_control enabling") introduced the following compiler warning:

mm/memcontrol.c: In function ‘mem_cgroup_can_attach’:
mm/memcontrol.c:4790:9: warning: ‘memcg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   mc.to = memcg;
         ^
Fix this by initializing 'memcg' to NULL and then verifying that it is set
to a value before dereferencing it.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---

This issue is present in v4.4-rc5 and later.

---
 mm/memcontrol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e234c21..69a28b3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4782,7 +4782,7 @@ static void mem_cgroup_clear_mc(void)
 static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
 {
 	struct cgroup_subsys_state *css;
-	struct mem_cgroup *memcg;
+	struct mem_cgroup *memcg = NULL;
 	struct mem_cgroup *from;
 	struct task_struct *leader, *p;
 	struct mm_struct *mm;
@@ -4805,7 +4805,7 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
 		p = leader;
 		memcg = mem_cgroup_from_css(css);
 	}
-	if (!p)
+	if (!p || !memcg)
 		return 0;
 
 	/*
-- 
2.6.3

Re: [PATCH] cgroup: Fix uninitialized variable warning

[Tejun Heo]

Hello, Ross.

On Wed, Dec 23, 2015 at 02:30:40PM -0700, Ross Zwisler wrote:
>  static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
>  {
>  	struct cgroup_subsys_state *css;
> -	struct mem_cgroup *memcg;
> +	struct mem_cgroup *memcg = NULL;

It's one thing to add spurious init to shut up gcc

> @@ -4805,7 +4805,7 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
>  		p = leader;
>  		memcg = mem_cgroup_from_css(css);
>  	}
> -	if (!p)
> +	if (!p || !memcg)

and to another to add an additional processing on it.

Please note which gcc version you were using in the commit description
and mark the init with a comment.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: Fix uninitialized variable warning

[Ross Zwisler]

On Wed, Dec 23, 2015 at 04:35:19PM -0500, Tejun Heo wrote:
> Hello, Ross.
> 
> On Wed, Dec 23, 2015 at 02:30:40PM -0700, Ross Zwisler wrote:
> >  static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
> >  {
> >  	struct cgroup_subsys_state *css;
> > -	struct mem_cgroup *memcg;
> > +	struct mem_cgroup *memcg = NULL;
> 
> It's one thing to add spurious init to shut up gcc
> 
> > @@ -4805,7 +4805,7 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
> >  		p = leader;
> >  		memcg = mem_cgroup_from_css(css);
> >  	}
> > -	if (!p)
> > +	if (!p || !memcg)
> 
> and to another to add an additional processing on it.

Do you believe that the additional processing is incorrect?  If somehow we
*do* get through the above loop without setting memcg, the next deref will
OOPs the kernel...

Re: [PATCH] cgroup: Fix uninitialized variable warning

[Tejun Heo]

On Wed, Dec 23, 2015 at 02:38:13PM -0700, Ross Zwisler wrote:
> On Wed, Dec 23, 2015 at 04:35:19PM -0500, Tejun Heo wrote:
> > Hello, Ross.
> > 
> > On Wed, Dec 23, 2015 at 02:30:40PM -0700, Ross Zwisler wrote:
> > >  static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
> > >  {
> > >  	struct cgroup_subsys_state *css;
> > > -	struct mem_cgroup *memcg;
> > > +	struct mem_cgroup *memcg = NULL;
> > 
> > It's one thing to add spurious init to shut up gcc
> > 
> > > @@ -4805,7 +4805,7 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
> > >  		p = leader;
> > >  		memcg = mem_cgroup_from_css(css);
> > >  	}
> > > -	if (!p)
> > > +	if (!p || !memcg)
> > 
> > and to another to add an additional processing on it.
> 
> Do you believe that the additional processing is incorrect?  If somehow we
> *do* get through the above loop without setting memcg, the next deref will
> OOPs the kernel...

That'd be a a plain kernel bug and oopsing is fine.  If such
conditions are particular (more likely, more difficult to debut,
whatever), we sometimes add WARNs for them but we don't generally go
around and add spurious checks.  It actually is deterimental to
readibility as people reading the code constantly have to go "when can
p && !memcg can happen? why is this explicitly checked?".

Thanks.

-- 
tejun

[PATCH] cgroup: Fix uninitialized variable warning

[Ross Zwisler]

Commit 1f7dd3e5a6e4 ("cgroup: fix handling of multi-destination migration
from subtree_control enabling") introduced the following compiler warning:

mm/memcontrol.c: In function ‘mem_cgroup_can_attach’:
mm/memcontrol.c:4790:9: warning: ‘memcg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   mc.to = memcg;
         ^

Fix this by initializing 'memcg' to NULL.

This was found using gcc (GCC) 4.9.2 20150212 (Red Hat 4.9.2-6).

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 48b22c3..ffd9750 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4735,7 +4735,7 @@ static void mem_cgroup_clear_mc(void)
 static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
 {
 	struct cgroup_subsys_state *css;
-	struct mem_cgroup *memcg;
+	struct mem_cgroup *memcg = NULL; /* unneeded init to make gcc happy */
 	struct mem_cgroup *from;
 	struct task_struct *leader, *p;
 	struct mm_struct *mm;
-- 
2.6.3

Re: [PATCH] cgroup: Fix uninitialized variable warning

[Tejun Heo]

On Wed, Dec 23, 2015 at 02:53:27PM -0700, Ross Zwisler wrote:
> Commit 1f7dd3e5a6e4 ("cgroup: fix handling of multi-destination migration
> from subtree_control enabling") introduced the following compiler warning:
> 
> mm/memcontrol.c: In function ‘mem_cgroup_can_attach’:
> mm/memcontrol.c:4790:9: warning: ‘memcg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    mc.to = memcg;
>          ^
> 
> Fix this by initializing 'memcg' to NULL.
> 
> This was found using gcc (GCC) 4.9.2 20150212 (Red Hat 4.9.2-6).
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Applied to cgroup/for-4.5.

Thanks.

-- 
tejun