mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Sudip Mukherjee (Codethink)]

Hi All,

The latest mainline kernel branch fails to build allmodconfig for every
ARCH with gcc-11 with the error:

drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c: In function 'dc_stream_remove_writeback':
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:527:83: error: array subscript [0, 0] is outside array bounds of 'struct dc_writeback_info[1]' [-Werror=array-bounds]
  527 |                                 stream->writeback_info[j] = stream->writeback_info[i];
      |                                                             ~~~~~~~~~~~~~~~~~~~~~~^~~
In file included from ./drivers/gpu/drm/amd/amdgpu/../display/dc/dc.h:1269,
                 from ./drivers/gpu/drm/amd/amdgpu/../display/dc/inc/core_types.h:29,
                 from ./drivers/gpu/drm/amd/amdgpu/../display/dc/basics/dc_common.h:29,
                 from drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_stream.c:27:
./drivers/gpu/drm/amd/amdgpu/../display/dc/dc_stream.h:241:34: note: while referencing 'writeback_info'
  241 |         struct dc_writeback_info writeback_info[MAX_DWB_PIPES];
      |                                  ^~~~~~~~~~~~~~


git bisect pointed to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

I will be happy to test any patch or provide any extra log if needed.

Note:
This is only seen with gcc-11, gcc-12 builds are ok.


-- 
Regards
Sudip

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Linus Torvalds]

On Thu, Oct 6, 2022 at 1:51 AM Sudip Mukherjee (Codethink)
<sudipm.mukherjee@gmail.com> wrote:
>
> This is only seen with gcc-11, gcc-12 builds are ok.

Hmm. This seems to be some odd gcc issue.

I *think* that what is going on is that the test

        j = 0 ; j < MAX_DWB_PIPES

makes gcc decide that "hey, j is in the range [0,MAX_DWB_PIPES[", and
then since MAX_DWB_PIPES is 1, that simplifies to "j must be zero".
Good range analysis so far.

But for 'i', we start off with that lower bound of 0, but the upper
bound is not fixed (the test for "i" is: "i < stream->num_wb_info").

And then that "if (i != j)", so now gcc decides that it can simplify
that to "if (i != 0)", and then simplifies *that* to "oh, the lower
bound of 'i' in that code is actually 1.

So then it decides that "stream->writeback_info[i]" must be out of bounds.

Of course, the *reality* is that stream->num_wb_info should be <=
MAX_DWB_PIPES, and as such (with the current MAX_DWB_PIPES value of 1)
it's not that 'i' can be 1, it's that the code in question cannot be
reached at all.

What confuses me is that error message ("array subscript [0, 0] is
outside array bounds of 'struct dc_writeback_info[1]') which seems to
be aware that the value is actually 0.

So this seems to be some gcc-11 range analysis bug, but I don't know
what the fix is. I suspect some random code change would magically
just make gcc realize it's ok after all, but since it all depends on
random gcc confusion, I don't know what the random code change would
be.

The fix *MAY* be to just add a '&& i < MAX_DWB_PIPES' to that loop
too, and then gcc will see that both i and j are always 0, and that
the code is unreachable and not warn about it. Hmm? Can you test that?

And the reason gcc-12 builds are ok probably isn't that gcc-12 gets
this right, it's simply that gcc-12 gets so many *opther* things wrong
that we already disabled -Warray-bounds with gcc-12 entirely.

If somebody cannot come up with a fix, I suspect the solution is "gcc
array bounds analysis is terminally buggy" and we just need to disable
it for gcc-11 too.

Kees, any idea? Who else might be interested in fixing a -Warray-bounds issue?

                 Linus

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Hamza Mahfooz]

Hey Linus,

On 2022-10-06 15:39, Linus Torvalds wrote:
> On Thu, Oct 6, 2022 at 1:51 AM Sudip Mukherjee (Codethink)
> <sudipm.mukherjee@gmail.com> wrote:
>>
>> This is only seen with gcc-11, gcc-12 builds are ok.
> 
> Hmm. This seems to be some odd gcc issue.
> 
> I *think* that what is going on is that the test
> 
>          j = 0 ; j < MAX_DWB_PIPES
> 
> makes gcc decide that "hey, j is in the range [0,MAX_DWB_PIPES[", and
> then since MAX_DWB_PIPES is 1, that simplifies to "j must be zero".
> Good range analysis so far.
> 
> But for 'i', we start off with that lower bound of 0, but the upper
> bound is not fixed (the test for "i" is: "i < stream->num_wb_info").
> 
> And then that "if (i != j)", so now gcc decides that it can simplify
> that to "if (i != 0)", and then simplifies *that* to "oh, the lower
> bound of 'i' in that code is actually 1.
> 
> So then it decides that "stream->writeback_info[i]" must be out of bounds.
> 
> Of course, the *reality* is that stream->num_wb_info should be <=
> MAX_DWB_PIPES, and as such (with the current MAX_DWB_PIPES value of 1)
> it's not that 'i' can be 1, it's that the code in question cannot be
> reached at all.
> 
> What confuses me is that error message ("array subscript [0, 0] is
> outside array bounds of 'struct dc_writeback_info[1]') which seems to
> be aware that the value is actually 0.
> 
> So this seems to be some gcc-11 range analysis bug, but I don't know
> what the fix is. I suspect some random code change would magically
> just make gcc realize it's ok after all, but since it all depends on
> random gcc confusion, I don't know what the random code change would
> be.
> 
> The fix *MAY* be to just add a '&& i < MAX_DWB_PIPES' to that loop
> too, and then gcc will see that both i and j are always 0, and that
> the code is unreachable and not warn about it. Hmm? Can you test that?
> 
> And the reason gcc-12 builds are ok probably isn't that gcc-12 gets
> this right, it's simply that gcc-12 gets so many *opther* things wrong
> that we already disabled -Warray-bounds with gcc-12 entirely.
> 
> If somebody cannot come up with a fix, I suspect the solution is "gcc
> array bounds analysis is terminally buggy" and we just need to disable
> it for gcc-11 too.

It seems that Stephen has a fix for this that works for multiple 
versions of GCC, see: 
https://lore.kernel.org/all/20221006191245.11bb0e2c@canb.auug.org.au/

> 
> Kees, any idea? Who else might be interested in fixing a -Warray-bounds issue?
> 
>                   Linus

-- 
Hamza

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Sudip Mukherjee]

On Thu, Oct 6, 2022 at 8:39 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Oct 6, 2022 at 1:51 AM Sudip Mukherjee (Codethink)
> <sudipm.mukherjee@gmail.com> wrote:
> >
> > This is only seen with gcc-11, gcc-12 builds are ok.
>
> Hmm. This seems to be some odd gcc issue.
>

<snip>

>
> The fix *MAY* be to just add a '&& i < MAX_DWB_PIPES' to that loop
> too, and then gcc will see that both i and j are always 0, and that
> the code is unreachable and not warn about it. Hmm? Can you test that?

Builds fine with the change you suggested. Here is the diff.


diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
index ae13887756bf..51571245c49a 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
@@ -520,7 +520,7 @@ bool dc_stream_remove_writeback(struct dc *dc,
        }

        /* remove writeback info for disabled writeback pipes from stream */
-       for (i = 0, j = 0; i < stream->num_wb_info && j < MAX_DWB_PIPES; i++) {
+       for (i = 0, j = 0; i < stream->num_wb_info && j <
MAX_DWB_PIPES && i < MAX_DWB_PIPES; i++) {
                if (stream->writeback_info[i].wb_enabled) {
                        if (i != j)
                                /* trim the array */



-- 
Regards
Sudip

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Kees Cook]

On Thu, Oct 06, 2022 at 12:39:40PM -0700, Linus Torvalds wrote:
> What confuses me is that error message ("array subscript [0, 0] is
> outside array bounds of 'struct dc_writeback_info[1]') which seems to
> be aware that the value is actually 0.

I've seen bugs in the tracker where the reporting is broken but the
range checker is working "correctly", which seems to be the case here.

> If somebody cannot come up with a fix, I suspect the solution is "gcc
> array bounds analysis is terminally buggy" and we just need to disable
> it for gcc-11 too.

It does continue to find bugs, so I'd rather keep it on. GCC has fixed
all the issues we've run into so far (though not all have been back
ported to GCC 12 yet, so yes, let's keep -Warray-bounds disabled there).

Specifically, I've been tracking:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679	Fixed 13+
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578	Fixed 12+, 11.3

And it looks like Sudip's proposed fix for this particular code is
additionally fixing unsigned vs signed as well. I think -Warray-bounds
did its job (though, with quite a confusing index range in the report).

-Kees

-- 
Kees Cook

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Sudip Mukherjee]

On Thu, Oct 6, 2022 at 9:37 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 06, 2022 at 12:39:40PM -0700, Linus Torvalds wrote:
> > What confuses me is that error message ("array subscript [0, 0] is
> > outside array bounds of 'struct dc_writeback_info[1]') which seems to
> > be aware that the value is actually 0.
>
> I've seen bugs in the tracker where the reporting is broken but the
> range checker is working "correctly", which seems to be the case here.
>
> > If somebody cannot come up with a fix, I suspect the solution is "gcc
> > array bounds analysis is terminally buggy" and we just need to disable
> > it for gcc-11 too.
>
> It does continue to find bugs, so I'd rather keep it on. GCC has fixed
> all the issues we've run into so far (though not all have been back
> ported to GCC 12 yet, so yes, let's keep -Warray-bounds disabled there).
>
> Specifically, I've been tracking:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679     Fixed 13+
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578      Fixed 12+, 11.3

Thats odd, the bug report says its fixed but I am using:
gcc version 11.3.1 20220925 (GCC)

>
> And it looks like Sudip's proposed fix for this particular code is
> additionally fixing unsigned vs signed as well. I think -Warray-bounds
> did its job (though, with quite a confusing index range in the report).

Not my. Linus's. I just tested. :)


-- 
Regards
Sudip

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Linus Torvalds]

On Thu, Oct 6, 2022 at 1:50 PM Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
>
> > And it looks like Sudip's proposed fix for this particular code is
> > additionally fixing unsigned vs signed as well. I think -Warray-bounds
> > did its job (though, with quite a confusing index range in the report).
>
> Not my. Linus's. I just tested. :)

I suspect Kees meant Stephen's other patch that Hamza pointed at, and
that is perhaps the cleaner version.

That said, I hate how this forces us to write random code changes just
to make a compiler just randomly _happen_ to not complain about it.

                   Linus