sched/fair: avoid using uninitialized variable in preferred_group_nid()

sched/fair: avoid using uninitialized variable in preferred_group_nid()

[Jan Beulich]

At least some gcc versions - validly afaict - warn about potentially
using max_group uninitialized: There's no way the compiler can prove
that the body of the conditional where it and max_faults get set/
updated gets executed; in fact, without knowing all the details of
other scheduler code, I can't prove this either.

Generally the necessary change would appear to be to clear max_group
prior to entering the inner loop, and break out of the outer loop when
it ends up being all clear after the inner one. This, however, seems
inefficient, and afaict the same effect can be achieved by exiting the
outer loop when max_faults is still zero after the inner loop. For the
compiler's sake, mark max_group uninitialized, as we're now able to
prove it's not actually being used uninitalized anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Rik van Riel <riel@redhat.com>
---
 kernel/sched/fair.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 3.19-rc5/kernel/sched/fair.c
+++ 3.19-rc5-sched-fair-preferred-group-nid/kernel/sched/fair.c
@@ -1730,7 +1730,7 @@ static int preferred_group_nid(struct ta
 	nodes = node_online_map;
 	for (dist = sched_max_numa_distance; dist > LOCAL_DISTANCE; dist--) {
 		unsigned long max_faults = 0;
-		nodemask_t max_group;
+		nodemask_t uninitialized_var(max_group);
 		int a, b;
 
 		/* Are there nodes at this distance from each other? */
@@ -1764,6 +1764,8 @@ static int preferred_group_nid(struct ta
 			}
 		}
 		/* Next round, evaluate the nodes within max_group. */
+		if (!max_faults)
+			break;
 		nodes = max_group;
 	}
 	return nid;

Re: sched/fair: avoid using uninitialized variable in preferred_group_nid()

[Peter Zijlstra]

On Fri, Jan 23, 2015 at 08:25:38AM +0000, Jan Beulich wrote:
> At least some gcc versions - validly afaict - warn about potentially
> using max_group uninitialized: There's no way the compiler can prove
> that the body of the conditional where it and max_faults get set/
> updated gets executed; in fact, without knowing all the details of
> other scheduler code, I can't prove this either.
> 
> Generally the necessary change would appear to be to clear max_group
> prior to entering the inner loop, and break out of the outer loop when
> it ends up being all clear after the inner one. This, however, seems
> inefficient, and afaict the same effect can be achieved by exiting the
> outer loop when max_faults is still zero after the inner loop. For the
> compiler's sake, mark max_group uninitialized, as we're now able to
> prove it's not actually being used uninitalized anymore.


Yes this is somewhat challenging. What compiler version in specific did
you get this warning wiht? I cannot remember seeing it with whatever it
is I use (4.7-4.9 it seems).


> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  kernel/sched/fair.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> --- 3.19-rc5/kernel/sched/fair.c
> +++ 3.19-rc5-sched-fair-preferred-group-nid/kernel/sched/fair.c
> @@ -1730,7 +1730,7 @@ static int preferred_group_nid(struct ta
>  	nodes = node_online_map;
>  	for (dist = sched_max_numa_distance; dist > LOCAL_DISTANCE; dist--) {
>  		unsigned long max_faults = 0;
> -		nodemask_t max_group;
> +		nodemask_t uninitialized_var(max_group);
>  		int a, b;
>  
>  		/* Are there nodes at this distance from each other? */
> @@ -1764,6 +1764,8 @@ static int preferred_group_nid(struct ta
>  			}
>  		}
>  		/* Next round, evaluate the nodes within max_group. */
> +		if (!max_faults)
> +			break;
>  		nodes = max_group;
>  	}
>  	return nid;

Yes I think you're right; there is no guarantee the faults sum will be
greater than 0, and therefore we might not trigger the assignment and
end up with uninitialized use.

Re: sched/fair: avoid using uninitialized variable in preferred_group_nid()

[Jan Beulich]

>>> On 23.01.15 at 10:54, <peterz@infradead.org> wrote:
> On Fri, Jan 23, 2015 at 08:25:38AM +0000, Jan Beulich wrote:
>> At least some gcc versions - validly afaict - warn about potentially
>> using max_group uninitialized: There's no way the compiler can prove
>> that the body of the conditional where it and max_faults get set/
>> updated gets executed; in fact, without knowing all the details of
>> other scheduler code, I can't prove this either.
>> 
>> Generally the necessary change would appear to be to clear max_group
>> prior to entering the inner loop, and break out of the outer loop when
>> it ends up being all clear after the inner one. This, however, seems
>> inefficient, and afaict the same effect can be achieved by exiting the
>> outer loop when max_faults is still zero after the inner loop. For the
>> compiler's sake, mark max_group uninitialized, as we're now able to
>> prove it's not actually being used uninitalized anymore.
> 
> 
> Yes this is somewhat challenging. What compiler version in specific did
> you get this warning wiht? I cannot remember seeing it with whatever it
> is I use (4.7-4.9 it seems).

SLE11 SP3's gcc (4.3.4 based).

Jan

[tip:sched/urgent] sched/fair: Avoid using uninitialized variable in preferred_group_nid()

[tip-bot for Jan Beulich]

Commit-ID:  81907478c4311a679849216abf723999184ab984
Gitweb:     http://git.kernel.org/tip/81907478c4311a679849216abf723999184ab984
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Fri, 23 Jan 2015 08:25:38 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 28 Jan 2015 13:14:12 +0100

sched/fair: Avoid using uninitialized variable in preferred_group_nid()

At least some gcc versions - validly afaict - warn about potentially
using max_group uninitialized: There's no way the compiler can prove
that the body of the conditional where it and max_faults get set/
updated gets executed; in fact, without knowing all the details of
other scheduler code, I can't prove this either.

Generally the necessary change would appear to be to clear max_group
prior to entering the inner loop, and break out of the outer loop when
it ends up being all clear after the inner one. This, however, seems
inefficient, and afaict the same effect can be achieved by exiting the
outer loop when max_faults is still zero after the inner loop.

[ mingo: changed the solution to zero initialization: uninitialized_var()
  needs to die, as it's an actively dangerous construct: if in the future
  a known-proven-good piece of code is changed to have a true, buggy
  uninitialized variable, the compiler warning is then supressed...

  The better long term solution is to clean up the code flow, so that
  even simple minded compilers (and humans!) are able to read it without
  getting a headache.  ]

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/54C2139202000078000588F7@mail.emea.novell.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 40667cb..fe331fc 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1730,7 +1730,7 @@ static int preferred_group_nid(struct task_struct *p, int nid)
 	nodes = node_online_map;
 	for (dist = sched_max_numa_distance; dist > LOCAL_DISTANCE; dist--) {
 		unsigned long max_faults = 0;
-		nodemask_t max_group;
+		nodemask_t max_group = NODE_MASK_NONE;
 		int a, b;
 
 		/* Are there nodes at this distance from each other? */

Re: [tip:sched/urgent] sched/fair: Avoid using uninitialized variable in preferred_group_nid()

[Jan Beulich]

>>> On 28.01.15 at 15:29, <tipbot@zytor.com> wrote:
> Commit-ID:  81907478c4311a679849216abf723999184ab984
> Gitweb:     
> http://git.kernel.org/tip/81907478c4311a679849216abf723999184ab984 
> Author:     Jan Beulich <JBeulich@suse.com>
> AuthorDate: Fri, 23 Jan 2015 08:25:38 +0000
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Wed, 28 Jan 2015 13:14:12 +0100
> 
> sched/fair: Avoid using uninitialized variable in preferred_group_nid()
> 
> At least some gcc versions - validly afaict - warn about potentially
> using max_group uninitialized: There's no way the compiler can prove
> that the body of the conditional where it and max_faults get set/
> updated gets executed; in fact, without knowing all the details of
> other scheduler code, I can't prove this either.
> 
> Generally the necessary change would appear to be to clear max_group
> prior to entering the inner loop, and break out of the outer loop when
> it ends up being all clear after the inner one. This, however, seems
> inefficient, and afaict the same effect can be achieved by exiting the
> outer loop when max_faults is still zero after the inner loop.
> 
> [ mingo: changed the solution to zero initialization: uninitialized_var()
>   needs to die, as it's an actively dangerous construct: if in the future
>   a known-proven-good piece of code is changed to have a true, buggy
>   uninitialized variable, the compiler warning is then supressed...

But you went farther than that: You also dropped the breaking
out of the outer loop. Yet that has - beyond the fixing of the bug
here - the desirable effect of not continuing for perhaps many
iterations when nothing new can ever be found anymore.

Jan

Re: [tip:sched/urgent] sched/fair: Avoid using uninitialized variable in preferred_group_nid()

[Peter Zijlstra]

On Wed, Jan 28, 2015 at 02:46:19PM +0000, Jan Beulich wrote:
> >>> On 28.01.15 at 15:29, <tipbot@zytor.com> wrote:
> > Commit-ID:  81907478c4311a679849216abf723999184ab984
> > Gitweb:     
> > http://git.kernel.org/tip/81907478c4311a679849216abf723999184ab984 
> > Author:     Jan Beulich <JBeulich@suse.com>
> > AuthorDate: Fri, 23 Jan 2015 08:25:38 +0000
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Wed, 28 Jan 2015 13:14:12 +0100
> > 
> > sched/fair: Avoid using uninitialized variable in preferred_group_nid()
> > 
> > At least some gcc versions - validly afaict - warn about potentially
> > using max_group uninitialized: There's no way the compiler can prove
> > that the body of the conditional where it and max_faults get set/
> > updated gets executed; in fact, without knowing all the details of
> > other scheduler code, I can't prove this either.
> > 
> > Generally the necessary change would appear to be to clear max_group
> > prior to entering the inner loop, and break out of the outer loop when
> > it ends up being all clear after the inner one. This, however, seems
> > inefficient, and afaict the same effect can be achieved by exiting the
> > outer loop when max_faults is still zero after the inner loop.
> > 
> > [ mingo: changed the solution to zero initialization: uninitialized_var()
> >   needs to die, as it's an actively dangerous construct: if in the future
> >   a known-proven-good piece of code is changed to have a true, buggy
> >   uninitialized variable, the compiler warning is then supressed...
> 
> But you went farther than that: You also dropped the breaking
> out of the outer loop. Yet that has - beyond the fixing of the bug
> here - the desirable effect of not continuing for perhaps many
> iterations when nothing new can ever be found anymore.

That break is indeed desired. The 'problem' it fixes is that when
group_faults() returns 0, faults will be 0, which will not > max_faults,
and therefore we will not set max_group.

Without that break, we'll now set nodes to NODE_MASK_NONE, which will
mean the for_each_node(a, nodes) loop will NOP and our dist loop will
iterate pointlessly.

Re: [tip:sched/urgent] sched/fair: Avoid using uninitialized variable in preferred_group_nid()

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Jan 28, 2015 at 02:46:19PM +0000, Jan Beulich wrote:
> > >>> On 28.01.15 at 15:29, <tipbot@zytor.com> wrote:
> > > Commit-ID:  81907478c4311a679849216abf723999184ab984
> > > Gitweb:     
> > > http://git.kernel.org/tip/81907478c4311a679849216abf723999184ab984 
> > > Author:     Jan Beulich <JBeulich@suse.com>
> > > AuthorDate: Fri, 23 Jan 2015 08:25:38 +0000
> > > Committer:  Ingo Molnar <mingo@kernel.org>
> > > CommitDate: Wed, 28 Jan 2015 13:14:12 +0100
> > > 
> > > sched/fair: Avoid using uninitialized variable in preferred_group_nid()
> > > 
> > > At least some gcc versions - validly afaict - warn about potentially
> > > using max_group uninitialized: There's no way the compiler can prove
> > > that the body of the conditional where it and max_faults get set/
> > > updated gets executed; in fact, without knowing all the details of
> > > other scheduler code, I can't prove this either.
> > > 
> > > Generally the necessary change would appear to be to clear max_group
> > > prior to entering the inner loop, and break out of the outer loop when
> > > it ends up being all clear after the inner one. This, however, seems
> > > inefficient, and afaict the same effect can be achieved by exiting the
> > > outer loop when max_faults is still zero after the inner loop.
> > > 
> > > [ mingo: changed the solution to zero initialization: uninitialized_var()
> > >   needs to die, as it's an actively dangerous construct: if in the future
> > >   a known-proven-good piece of code is changed to have a true, buggy
> > >   uninitialized variable, the compiler warning is then supressed...
> > 
> > But you went farther than that: You also dropped the breaking
> > out of the outer loop. Yet that has - beyond the fixing of the bug
> > here - the desirable effect of not continuing for perhaps many
> > iterations when nothing new can ever be found anymore.
> 
> That break is indeed desired. The 'problem' it fixes is 
> that when group_faults() returns 0, faults will be 0, 
> which will not > max_faults, and therefore we will not 
> set max_group.
> 
> Without that break, we'll now set nodes to 
> NODE_MASK_NONE, which will mean the for_each_node(a, 
> nodes) loop will NOP and our dist loop will iterate 
> pointlessly.

Ok, agreed, please send a separate patch to fix this.

Thanks,

	Ingo

Re: [tip:sched/urgent] sched/fair: Avoid using uninitialized variable in preferred_group_nid()

[Peter Zijlstra]

On Mon, Feb 09, 2015 at 09:21:15AM +0100, Ingo Molnar wrote:
> Ok, agreed, please send a separate patch to fix this.

Jan, I didn't want to put your SoB on without feedback, holler and I'll
amend it any way you like.

---
Subject: sched/numa: Avoid some pointless iterations
From: Jan Beulich <JBeulich@suse.com>
Date: Mon Feb  9 12:30:00 CET 2015

Commit 81907478c431 ("sched/fair: Avoid using uninitialized variable
in preferred_group_nid()") unconditionally initializes max_group with
NODE_MASK_NONE, this means that when !max_faults (max_group didn't get
set), we'll now continue the iteration with an empty mask.

Which in turn makes the actual body of the loop go away, so we'll just
iterate until completion; short circuit this by breaking out of the
loop as soon as this would happen.

Maybe-Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1763,6 +1763,8 @@ static int preferred_group_nid(struct ta
 			}
 		}
 		/* Next round, evaluate the nodes within max_group. */
+		if (!max_faults)
+			break;
 		nodes = max_group;
 	}
 	return nid;

Re: [tip:sched/urgent] sched/fair: Avoid using uninitialized variable in preferred_group_nid()

[Jan Beulich]

>>> On 09.02.15 at 12:37, <peterz@infradead.org> wrote:
> On Mon, Feb 09, 2015 at 09:21:15AM +0100, Ingo Molnar wrote:
>> Ok, agreed, please send a separate patch to fix this.
> 
> Jan, I didn't want to put your SoB on without feedback, holler and I'll
> amend it any way you like.

It's perfectly fine to add it.

Jan

> ---
> Subject: sched/numa: Avoid some pointless iterations
> From: Jan Beulich <JBeulich@suse.com>
> Date: Mon Feb  9 12:30:00 CET 2015
> 
> Commit 81907478c431 ("sched/fair: Avoid using uninitialized variable
> in preferred_group_nid()") unconditionally initializes max_group with
> NODE_MASK_NONE, this means that when !max_faults (max_group didn't get
> set), we'll now continue the iteration with an empty mask.
> 
> Which in turn makes the actual body of the loop go away, so we'll just
> iterate until completion; short circuit this by breaking out of the
> loop as soon as this would happen.
> 
> Maybe-Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/fair.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1763,6 +1763,8 @@ static int preferred_group_nid(struct ta
>  			}
>  		}
>  		/* Next round, evaluate the nodes within max_group. */
> +		if (!max_faults)
> +			break;
>  		nodes = max_group;
>  	}
>  	return nid;

[tip:sched/core] sched/numa: Avoid some pointless iterations

[tip-bot for Jan Beulich]

Commit-ID:  890a5409f9d0c84d75a1e16eebdfe91d8a57ef1e
Gitweb:     http://git.kernel.org/tip/890a5409f9d0c84d75a1e16eebdfe91d8a57ef1e
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Mon, 9 Feb 2015 12:30:00 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 18 Feb 2015 16:18:02 +0100

sched/numa: Avoid some pointless iterations

Commit 81907478c431 ("sched/fair: Avoid using uninitialized variable
in preferred_group_nid()") unconditionally initializes max_group with
NODE_MASK_NONE, this means that when !max_faults (max_group didn't get
set), we'll now continue the iteration with an empty mask.

Which in turn makes the actual body of the loop go away, so we'll just
iterate until completion; short circuit this by breaking out of the
loop as soon as this would happen.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20150209113727.GS5029@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 28cbaca..ee595ef 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1774,6 +1774,8 @@ static int preferred_group_nid(struct task_struct *p, int nid)
 			}
 		}
 		/* Next round, evaluate the nodes within max_group. */
+		if (!max_faults)
+			break;
 		nodes = max_group;
 	}
 	return nid;