* [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES
@ 2022-05-20 16:58 Kees Cook
2022-05-20 17:09 ` Suren Baghdasaryan
2022-05-23 14:19 ` Johannes Weiner
0 siblings, 2 replies; 4+ messages in thread
From: Kees Cook @ 2022-05-20 16:58 UTC (permalink / raw)
To: Johannes Weiner
Cc: Kees Cook, Suren Baghdasaryan, Ingo Molnar, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, Daniel Bristot de Oliveira,
Valentin Schneider, linux-kernel, linux-hardening
GCC 12 cannot tell that "t" will be bounded by NR_PSI_STATES, which could
lead to walking off the end of the tasks array, which is NR_PSI_STATES in
size. Explicitly bounds-check "t" as part of the loop.
In file included from ../kernel/sched/build_utility.c:97:
../kernel/sched/psi.c: In function 'psi_group_change':
../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds]
730 | groupc->tasks[t]++;
| ~~~~~~~~~~~~~^~~
In file included from ../include/linux/psi.h:6,
from ../kernel/sched/build_utility.c:36:
../include/linux/psi_types.h:84:22: note: while referencing 'tasks'
84 | unsigned int tasks[NR_PSI_TASK_COUNTS];
| ^~~~~
../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds]
730 | groupc->tasks[t]++;
| ~~~~~~~~~~~~~^~~
../include/linux/psi_types.h:84:22: note: while referencing 'tasks'
84 | unsigned int tasks[NR_PSI_TASK_COUNTS];
| ^~~~~
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
Cc: Valentin Schneider <vschneid@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/sched/psi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index a337f3e35997..827f16a79936 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -725,7 +725,7 @@ static void psi_group_change(struct psi_group *group, int cpu,
}
}
- for (t = 0; set; set &= ~(1 << t), t++)
+ for (t = 0; set && t < ARRAY_SIZE(groupc->tasks); set &= ~(1 << t), t++)
if (set & (1 << t))
groupc->tasks[t]++;
--
2.32.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES 2022-05-20 16:58 [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES Kees Cook @ 2022-05-20 17:09 ` Suren Baghdasaryan 2022-05-23 14:19 ` Johannes Weiner 1 sibling, 0 replies; 4+ messages in thread From: Suren Baghdasaryan @ 2022-05-20 17:09 UTC (permalink / raw) To: Kees Cook Cc: Johannes Weiner, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, LKML, linux-hardening On Fri, May 20, 2022 at 9:58 AM Kees Cook <keescook@chromium.org> wrote: > > GCC 12 cannot tell that "t" will be bounded by NR_PSI_STATES, which could > lead to walking off the end of the tasks array, which is NR_PSI_STATES in > size. Explicitly bounds-check "t" as part of the loop. > > In file included from ../kernel/sched/build_utility.c:97: > ../kernel/sched/psi.c: In function 'psi_group_change': > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds] > 730 | groupc->tasks[t]++; > | ~~~~~~~~~~~~~^~~ > In file included from ../include/linux/psi.h:6, > from ../kernel/sched/build_utility.c:36: > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks' > 84 | unsigned int tasks[NR_PSI_TASK_COUNTS]; > | ^~~~~ > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds] > 730 | groupc->tasks[t]++; > | ~~~~~~~~~~~~~^~~ > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks' > 84 | unsigned int tasks[NR_PSI_TASK_COUNTS]; > | ^~~~~ > > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Suren Baghdasaryan <surenb@google.com> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Juri Lelli <juri.lelli@redhat.com> > Cc: Vincent Guittot <vincent.guittot@linaro.org> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ben Segall <bsegall@google.com> > Cc: Mel Gorman <mgorman@suse.de> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > Cc: Valentin Schneider <vschneid@redhat.com> > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Suren Baghdasaryan <surenb@google.com> > --- > kernel/sched/psi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index a337f3e35997..827f16a79936 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -725,7 +725,7 @@ static void psi_group_change(struct psi_group *group, int cpu, > } > } > > - for (t = 0; set; set &= ~(1 << t), t++) > + for (t = 0; set && t < ARRAY_SIZE(groupc->tasks); set &= ~(1 << t), t++) > if (set & (1 << t)) > groupc->tasks[t]++; > > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES 2022-05-20 16:58 [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES Kees Cook 2022-05-20 17:09 ` Suren Baghdasaryan @ 2022-05-23 14:19 ` Johannes Weiner 2022-05-23 20:30 ` Kees Cook 1 sibling, 1 reply; 4+ messages in thread From: Johannes Weiner @ 2022-05-23 14:19 UTC (permalink / raw) To: Kees Cook Cc: Suren Baghdasaryan, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel, linux-hardening On Fri, May 20, 2022 at 09:58:26AM -0700, Kees Cook wrote: > GCC 12 cannot tell that "t" will be bounded by NR_PSI_STATES, which could > lead to walking off the end of the tasks array, which is NR_PSI_STATES in > size. Explicitly bounds-check "t" as part of the loop. > > In file included from ../kernel/sched/build_utility.c:97: > ../kernel/sched/psi.c: In function 'psi_group_change': > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds] > 730 | groupc->tasks[t]++; > | ~~~~~~~~~~~~~^~~ > In file included from ../include/linux/psi.h:6, > from ../kernel/sched/build_utility.c:36: > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks' > 84 | unsigned int tasks[NR_PSI_TASK_COUNTS]; > | ^~~~~ > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds] > 730 | groupc->tasks[t]++; > | ~~~~~~~~~~~~~^~~ > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks' > 84 | unsigned int tasks[NR_PSI_TASK_COUNTS]; > | ^~~~~ > > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Suren Baghdasaryan <surenb@google.com> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Juri Lelli <juri.lelli@redhat.com> > Cc: Vincent Guittot <vincent.guittot@linaro.org> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ben Segall <bsegall@google.com> > Cc: Mel Gorman <mgorman@suse.de> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > Cc: Valentin Schneider <vschneid@redhat.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > kernel/sched/psi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index a337f3e35997..827f16a79936 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -725,7 +725,7 @@ static void psi_group_change(struct psi_group *group, int cpu, > } > } > > - for (t = 0; set; set &= ~(1 << t), t++) > + for (t = 0; set && t < ARRAY_SIZE(groupc->tasks); set &= ~(1 << t), t++) > if (set & (1 << t)) > groupc->tasks[t]++; This is a very hot path, it runs for every nested cgroup on every task switch, wakeup and sleep. We should avoid unnecessary instructions and branches if we can help it at all. Does the below patch address the warning for you? I can't test it myself, because I'm not getting it with gcc version 12.1.0. It's also odd that it didn't warn you about the loop over `clear' a few lines up, which ostensibly has the same "problem". --- diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h index c7fe7c089718..113861343733 100644 --- a/include/linux/psi_types.h +++ b/include/linux/psi_types.h @@ -41,6 +41,7 @@ enum psi_task_count { #define TSK_RUNNING (1 << NR_RUNNING) #define TSK_ONCPU (1 << NR_ONCPU) #define TSK_MEMSTALL_RUNNING (1 << NR_MEMSTALL_RUNNING) +#define TSK_MASK ((1U << NR_PSI_TASK_COUNTS) - 1) /* Resources that workloads could be stalled on */ enum psi_res { diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c index a4fa3aadfcba..fb7fd40af337 100644 --- a/kernel/sched/psi.c +++ b/kernel/sched/psi.c @@ -804,6 +804,8 @@ void psi_task_change(struct task_struct *task, int clear, int set) void *iter = NULL; u64 now; + WARN_ON_ONCE((clear|set) & ~TSK_MASK); + if (!task->pid) return; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES 2022-05-23 14:19 ` Johannes Weiner @ 2022-05-23 20:30 ` Kees Cook 0 siblings, 0 replies; 4+ messages in thread From: Kees Cook @ 2022-05-23 20:30 UTC (permalink / raw) To: Johannes Weiner Cc: Suren Baghdasaryan, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel, linux-hardening On Mon, May 23, 2022 at 10:19:22AM -0400, Johannes Weiner wrote: > On Fri, May 20, 2022 at 09:58:26AM -0700, Kees Cook wrote: > > GCC 12 cannot tell that "t" will be bounded by NR_PSI_STATES, which could > > lead to walking off the end of the tasks array, which is NR_PSI_STATES in > > size. Explicitly bounds-check "t" as part of the loop. > > > > In file included from ../kernel/sched/build_utility.c:97: > > ../kernel/sched/psi.c: In function 'psi_group_change': > > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds] > > 730 | groupc->tasks[t]++; > > | ~~~~~~~~~~~~~^~~ > > In file included from ../include/linux/psi.h:6, > > from ../kernel/sched/build_utility.c:36: > > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks' > > 84 | unsigned int tasks[NR_PSI_TASK_COUNTS]; > > | ^~~~~ > > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds] > > 730 | groupc->tasks[t]++; > > | ~~~~~~~~~~~~~^~~ > > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks' > > 84 | unsigned int tasks[NR_PSI_TASK_COUNTS]; > > | ^~~~~ > > > > Cc: Johannes Weiner <hannes@cmpxchg.org> > > Cc: Suren Baghdasaryan <surenb@google.com> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Juri Lelli <juri.lelli@redhat.com> > > Cc: Vincent Guittot <vincent.guittot@linaro.org> > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Ben Segall <bsegall@google.com> > > Cc: Mel Gorman <mgorman@suse.de> > > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > > Cc: Valentin Schneider <vschneid@redhat.com> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > kernel/sched/psi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > > index a337f3e35997..827f16a79936 100644 > > --- a/kernel/sched/psi.c > > +++ b/kernel/sched/psi.c > > @@ -725,7 +725,7 @@ static void psi_group_change(struct psi_group *group, int cpu, > > } > > } > > > > - for (t = 0; set; set &= ~(1 << t), t++) > > + for (t = 0; set && t < ARRAY_SIZE(groupc->tasks); set &= ~(1 << t), t++) > > if (set & (1 << t)) > > groupc->tasks[t]++; > > This is a very hot path, it runs for every nested cgroup on every task > switch, wakeup and sleep. We should avoid unnecessary instructions and > branches if we can help it at all. > > Does the below patch address the warning for you? I can't test it > myself, because I'm not getting it with gcc version 12.1.0. It's also > odd that it didn't warn you about the loop over `clear' a few lines > up, which ostensibly has the same "problem". It seems this is a GCC 12 bug. Array indexes that have been used with a shift end up miscalculated when building also with -fsanitize=shift (i.e. allmodconfig): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679 > > --- > > diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h > index c7fe7c089718..113861343733 100644 > --- a/include/linux/psi_types.h > +++ b/include/linux/psi_types.h > @@ -41,6 +41,7 @@ enum psi_task_count { > #define TSK_RUNNING (1 << NR_RUNNING) > #define TSK_ONCPU (1 << NR_ONCPU) > #define TSK_MEMSTALL_RUNNING (1 << NR_MEMSTALL_RUNNING) > +#define TSK_MASK ((1U << NR_PSI_TASK_COUNTS) - 1) > > /* Resources that workloads could be stalled on */ > enum psi_res { > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index a4fa3aadfcba..fb7fd40af337 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -804,6 +804,8 @@ void psi_task_change(struct task_struct *task, int clear, int set) > void *iter = NULL; > u64 now; > > + WARN_ON_ONCE((clear|set) & ~TSK_MASK); > + > if (!task->pid) > return; I'll give this a spin. Thanks! -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-23 20:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-20 16:58 [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES Kees Cook 2022-05-20 17:09 ` Suren Baghdasaryan 2022-05-23 14:19 ` Johannes Weiner 2022-05-23 20:30 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).