Re: [danielt-linux:kgdb/for-next 4/4] kernel/debug/debug_core.c:452:17: warning: array subscript is outside array bounds

[Doug Anderson]

Hi,

On Mon, Oct 14, 2019 at 9:28 AM Daniel Thompson
<daniel.thompson@xxxxxxxxxx> wrote:
>
> Hi Doug
>
> On Fri, Oct 11, 2019 at 12:41:31AM +0800, kbuild test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/danielt/linux.git kgdb/for-next
> > head:   2277b492582d5525244519f60da6f9daea5ef41a
> > commit: 2277b492582d5525244519f60da6f9daea5ef41a [4/4] kdb: Fix stack crawling on 'running' CPUs that aren't the master
> > config: sh-allyesconfig (attached as .config)
> > compiler: sh4-linux-gcc (GCC) 7.4.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         git checkout 2277b492582d5525244519f60da6f9daea5ef41a
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.4.0 make.cross ARCH=sh
> >
> > If you fix the issue, kindly add following tag
> > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> >
> > Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> >
> > All warnings (new ones prefixed by >>):
> >
> >    kernel/debug/debug_core.c: In function 'kdb_dump_stack_on_cpu':
> > >> kernel/debug/debug_core.c:452:17: warning: array subscript is outside array bounds [-Warray-bounds]
> >      if (!(kgdb_info[cpu].exception_state & DCPU_IS_SLAVE)) {
> >            ~~~~~~~~~^~~~~
> >    kernel/debug/debug_core.c:469:33: warning: array subscript is outside array bounds [-Warray-bounds]
> >      kgdb_info[cpu].exception_state |= DCPU_WANT_BT;
> >    kernel/debug/debug_core.c:470:18: warning: array subscript is outside array bounds [-Warray-bounds]
> >      while (kgdb_info[cpu].exception_state & DCPU_WANT_BT)
> >             ~~~~~~~~~^~~~~
>
> Thoughts on the following?
>
> From 9e0114bc9ae504c3a7e837c977d64f84b2010d8e Mon Sep 17 00:00:00 2001
> From: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> Date: Fri, 11 Oct 2019 08:49:29 +0100
> Subject: [PATCH] kdb: Avoid array subscript warnings on non-SMP builds
>
> Recent versions of gcc (reported on gcc-7.4) issue array subscript
> warnings for builds where SMP is not enabled.
>
> There is no bug here but there is scope to improve the code
> generation for non-SMP systems (whilst also silencing the warning).
>
> Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> Fixes: 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs that aren't the master")
> Signed-off-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> ---
>  kernel/debug/debug_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 70e86b4b4932..eccb7298a0b5 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -449,7 +449,8 @@ void kdb_dump_stack_on_cpu(int cpu)
>                 return;
>         }
>
> -       if (!(kgdb_info[cpu].exception_state & DCPU_IS_SLAVE)) {
> +       if (!IS_ENABLED(CONFIG_SMP) ||
> +           !(kgdb_info[cpu].exception_state & DCPU_IS_SLAVE)) {

Thanks for sending this!  I saw the alert but I was on vacation last
Friday and today has been a trainwreck.

I guess my first thought is that this fix is slightly confusing to
read.  Reading it makes you think that if we don't have SMP that we'll
print:

  ERROR: Task on cpu %d didn't stop in the debugger

It took me a second to realize that of course this would never print
because if we're ot on SMP then the first "if" test would trip and
we'd return.  Which makes me wonder why we couldn't instead change
that "if" test to:

 if (!IS_ENABLED(CONFIG_SMP) || cpu == raw_smp_processor_id()) {

...and be done with it.

---

As far as the "there is no bug here", I actually wonder.  We are
always called with a cpu that we got from "kdb_process_cpu(p)", right?
 That function sets cpu to 0 if "cpu > num_possible_cpus()".  ...but
shouldn't it be >=?  I guess task_cpu() probably returned something
sane anyway...

I also find it a little odd that kdb_process_cpu() returns a signed
int even though task_cpu() returns an unsigned one, but I guess we
don't need to worry about the case where the number of CPUs can't fit
into a signed int?

---

Hopefully that's all correct.  I'm just about outta time and I wanted
to send something off.  If something looks wrong it probably is...

-Doug