Re: New warnings with gcc-11

Re: New warnings with gcc-11

[Ronald Warsow]

may I add this ?

https://marc.info/?l=linux-kernel&m=161819303505220&w=2

--
regards

Ronald

Re: New warnings with gcc-11

[Jani Nikula]

On Sat, 08 May 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> I have heard nothing about this, and it remains the only warning from
> my allmodconfig build (I have another one for drm compiled with clang,
> but there I at least heard back that a fix exists).
>
> Since I am going to release rc1 tomorrow, and I don't want to release
> it with an ugly compiler warning, I took it upon myself to just fix
> the code:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852
>
> HOWEVER.
>
> That commit fixes the warning, and is at worst harmless. At best it
> fixes an access to random stack memory. But it does smell like
> somebody who actually knows how these arrays work should look at that
> code.
>
> IOW, maybe the code should actually have read 16 bytes from the Event
> Status Indicator? Maybe offset 10 was wrong? Maybe
> drm_dp_channel_eq_ok() should never have taken six bytes to begin
> with?
>
> It's a mystery, and I haven't heard anything otherwise, so there it is.

Fair enough. My bad for not getting this fixed.

The fix is harmless. drm_dp_channel_eq_ok() only ever accesses 3 bytes
instead of 6. I figure the DP_LINK_STATUS_SIZE (=6) is there because in
the normal case you'd read that much, and use a family of functions on
that data, some of which do access the full 6 bytes, some don't.

In our case, we use drm_dp_channel_eq_ok() to check 3 bytes of similarly
encoded data elsewhere in the DPCD address space, and the
DP_LINK_STATUS_SIZE is meaningless there.

The straightforward fix would be to replace
link_status[DP_LINK_STATUS_SIZE] with link_status[3], and that likely
needs changes in dp_link_status() and dp_get_lane_status() as well.


BR,
Jani.


>
>               Linus
>
> On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> > I've updated to Fedora 34 on one of my machines, and it causes a lot
>> > of i915 warnings like
>> >
>> >   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
>> >   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
>> > of type ‘const u16 *’ {aka ‘const short unsigned int *’}
>> >   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
>> > ‘intel_print_wm_latency’
>> >
>> > and the reason is that gcc now seems to look at the argument array
>> > size more, and notices that
>>
>> Arnd Bergmann reported some of these a while back. I think we have some
>> of them fixed in our -next already, but not all. Thanks for the
>> reminder.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New warnings with gcc-11

[Linus Torvalds]

I have heard nothing about this, and it remains the only warning from
my allmodconfig build (I have another one for drm compiled with clang,
but there I at least heard back that a fix exists).

Since I am going to release rc1 tomorrow, and I don't want to release
it with an ugly compiler warning, I took it upon myself to just fix
the code:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852

HOWEVER.

That commit fixes the warning, and is at worst harmless. At best it
fixes an access to random stack memory. But it does smell like
somebody who actually knows how these arrays work should look at that
code.

IOW, maybe the code should actually have read 16 bytes from the Event
Status Indicator? Maybe offset 10 was wrong? Maybe
drm_dp_channel_eq_ok() should never have taken six bytes to begin
with?

It's a mystery, and I haven't heard anything otherwise, so there it is.

              Linus

On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > I've updated to Fedora 34 on one of my machines, and it causes a lot
> > of i915 warnings like
> >
> >   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> >   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
> > of type ‘const u16 *’ {aka ‘const short unsigned int *’}
> >   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> > ‘intel_print_wm_latency’
> >
> > and the reason is that gcc now seems to look at the argument array
> > size more, and notices that
>
> Arnd Bergmann reported some of these a while back. I think we have some
> of them fixed in our -next already, but not all. Thanks for the
> reminder.

Re: New warnings with gcc-11

[Jani Nikula]

On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> I've updated to Fedora 34 on one of my machines, and it causes a lot
> of i915 warnings like
>
>   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
>   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
> of type ‘const u16 *’ {aka ‘const short unsigned int *’}
>   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> ‘intel_print_wm_latency’
>
> and the reason is that gcc now seems to look at the argument array
> size more, and notices that

Arnd Bergmann reported some of these a while back. I think we have some
of them fixed in our -next already, but not all. Thanks for the
reminder.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New warnings with gcc-11

[Linus Torvalds]

On Tue, Apr 27, 2021 at 4:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think I will make the argument type to intel_print_wm_latency() be
> just "const u16 wm[]" for now, just to avoid seeing a ton of silly
> warnings.

After fixing the trivial ones, this one remains:

  drivers/gpu/drm/i915/display/intel_dp.c: In function
‘intel_dp_check_mst_status’:
  drivers/gpu/drm/i915/display/intel_dp.c:4554:22: warning:
‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4
[-Wstringop-overread]
   4554 |                     !drm_dp_channel_eq_ok(&esi[10],
intel_dp->lane_count)) {
        |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/gpu/drm/i915/display/intel_dp.c:4554:22: note: referencing
argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’}
  In file included from drivers/gpu/drm/i915/display/intel_dp.c:38:
  ./include/drm/drm_dp_helper.h:1459:6: note: in a call to function
‘drm_dp_channel_eq_ok’
   1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
        |      ^~~~~~~~~~~~~~~~~~~~

and I'm not fixing that one, because it actually looks like a valid
warning, and doesn't have an obvious fix.

That "esi[]" array is 14 bytes in size (DP_DPRX_ESI_LEN). So when it
does that "&esi[10]" and passes it in as an argument, then only 4
bytes remain of the array.

And drm_dp_channel_eq_ok() supposedly takes a "const u8
link_status[DP_LINK_STATUS_SIZE]", which is 6 bytes.

There may be some reason this is ok, but it does look a bit fishy, and
the compiler warning is appropriate.

            Linus

New warnings with gcc-11

[Linus Torvalds]

I've updated to Fedora 34 on one of my machines, and it causes a lot
of i915 warnings like

  drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
  drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
of type ‘const u16 *’ {aka ‘const short unsigned int *’}
  drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
‘intel_print_wm_latency’

and the reason is that gcc now seems to look at the argument array
size more, and notices that

 (a) intel_print_wm_latency() takes a "const u16 wm[8]" argument

but

 (b) most of the arrays passed in tend to look like 'u16 pri_latency[5]'

I think I will make the argument type to intel_print_wm_latency() be
just "const u16 wm[]" for now, just to avoid seeing a ton of silly
warnings.

I'm not sure if there is a better solution (like making all of those
latency arrays be 8 entries in size), so I'm just letting you know
about my change in this area in case anybody has a better idea.

             Linus