On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <[email protected]> wrote:
> > +/* The + 1 below places the pointers within the range of their array */
> > #define for_class_range(class, _from, _to) \
> > - for (class = (_from); class != (_to); class--)
> > + for (class = (_from); class + 1 != (_to) + 1; class--)
>
> Urgh, so now we get less readable code, just because GCC is being
> stupid?
>
> What's wrong with negative array indexes? memory is memory, stuff works.
What's more, C is C. Glorified assembly language in which people do odd
stuff.
But this is presumably a released gcc version and we need to do
something. And presumably, we need to do a backportable something, so
people can compile older kernels with gcc-12.
Is it possible to suppress just this warning with a gcc option? And if
so, are we confident that this warning will never be useful in other
places in the kernel?
If no||no then we'll need to add workarounds such as these?
On Thu, Apr 14, 2022 at 01:30:50PM -0700, Andrew Morton wrote:
> On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <[email protected]> wrote:
>
> > > +/* The + 1 below places the pointers within the range of their array */
> > > #define for_class_range(class, _from, _to) \
> > > - for (class = (_from); class != (_to); class--)
> > > + for (class = (_from); class + 1 != (_to) + 1; class--)
> >
> > Urgh, so now we get less readable code, just because GCC is being
> > stupid?
> >
> > What's wrong with negative array indexes? memory is memory, stuff works.
>
> What's more, C is C. Glorified assembly language in which people do odd
> stuff.
>
> But this is presumably a released gcc version and we need to do
> something. And presumably, we need to do a backportable something, so
> people can compile older kernels with gcc-12.
>
> Is it possible to suppress just this warning with a gcc option? And if
> so, are we confident that this warning will never be useful in other
> places in the kernel?
>
> If no||no then we'll need to add workarounds such as these?
-Wno-array-bounds
Is the obvious fix-all cure. The thing is, I want to hear if this new
warning has any actual use or is just crack induced madness like many of
the warnings we turn off.
On Sun, Apr 17, 2022 at 05:52:05PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 01:30:50PM -0700, Andrew Morton wrote:
> > On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <[email protected]> wrote:
> >
> > > > +/* The + 1 below places the pointers within the range of their array */
> > > > #define for_class_range(class, _from, _to) \
> > > > - for (class = (_from); class != (_to); class--)
> > > > + for (class = (_from); class + 1 != (_to) + 1; class--)
> > >
> > > Urgh, so now we get less readable code, just because GCC is being
> > > stupid?
> > >
> > > What's wrong with negative array indexes? memory is memory, stuff works.
> >
> > What's more, C is C. Glorified assembly language in which people do odd
> > stuff.
> >
> > But this is presumably a released gcc version and we need to do
> > something. And presumably, we need to do a backportable something, so
> > people can compile older kernels with gcc-12.
> >
> > Is it possible to suppress just this warning with a gcc option? And if
> > so, are we confident that this warning will never be useful in other
> > places in the kernel?
> >
> > If no||no then we'll need to add workarounds such as these?
>
> -Wno-array-bounds
Please no; we just spent two years fixing all the old non-flexible array
definitions and so many other things fixed for this to be enable because
it finds actual flaws (but we turned it off when it was introduced
because of how much sloppy old code we had).
> Is the obvious fix-all cure. The thing is, I want to hear if this new
> warning has any actual use or is just crack induced madness like many of
> the warnings we turn off.
Yes, it finds real flaws. And also yes, it is rather opinionated about
some "tricks" that have worked in C, but frankly, most of those tricks
end up being weird/accidentally-correct and aren't great for long-term
readability or robustness. Though I'm not speaking specifically to this
proposed patch; I haven't looked closely at it yet.
I quickly went back through commits; here's a handful I found:
20314bacd2f9 ("staging: r8188eu: Fix PPPoE tag insertion on little endian systems")
dcf500065fab ("net: bnxt_ptp: fix compilation error")
5f7dc7d48c94 ("octeontx2-af: fix array bound error")
c7d58971dbea ("ALSA: mixart: Reduce size of mixart_timer_notify")
b3f1dd52c991 ("ARM: vexpress/spc: Avoid negative array index when !SMP")
a2151490cc6c ("drm/dp: Fix OOB read when handling Post Cursor2 register")
d4da1f27396f ("drm/dp: Fix off-by-one in register cache size")
47307c31d90a ("crypto: octeontx2 - Avoid stack variable overflow")
a6501e4b380f ("eeprom: at25: Restore missing allocation")
33ce7f2f95ca ("drm/imx: imx-ldb: fix out of bounds array access warning")
f051ae4f6c73 ("Input: cyapa_gen6 - fix out-of-bounds stack access")
f3217d6f2f7a ("firmware: xilinx: fix out-of-bounds access")
8a03447dd311 ("rtw88: fix subscript above array bounds compiler warning")
ad82a928eb58 ("s390/perf: fix gcc 8 array-bounds warning")
6038aa532a22 ("nvme: target: fix buffer overflow")
50a0d71a5d20 ("cros_ec: fix nul-termination for firmware build info")
43d15c201313 ("staging: rtl8822be: Keep array subscript no lower than zero")
The important part is that with this enabled now, we won't get _new_
problems introduced. Making the C code clear enough that the compiler
can understand the intent, though, can be a little annoying, but makes
things much easier to automatically check. Getting our code-base arranged
so the toolchain can actually do the analysis is well worth it.
--
Kees Cook
On Wed, Apr 20, 2022 at 11:45:05AM -0700, Kees Cook wrote:
> > -Wno-array-bounds
>
> Please no; we just spent two years fixing all the old non-flexible array
> definitions and so many other things fixed for this to be enable because
> it finds actual flaws (but we turned it off when it was introduced
> because of how much sloppy old code we had).
>
> > Is the obvious fix-all cure. The thing is, I want to hear if this new
> > warning has any actual use or is just crack induced madness like many of
> > the warnings we turn off.
>
> Yes, it finds real flaws. And also yes, it is rather opinionated about
> some "tricks" that have worked in C, but frankly, most of those tricks
> end up being weird/accidentally-correct and aren't great for long-term
> readability or robustness. Though I'm not speaking specifically to this
> proposed patch; I haven't looked closely at it yet.
So the whole access outside object is UB thing in C is complete rubbish
from an OS perspective. The memory is there and there are geniune uses
for it.
And so far, the patches I've seen for it make the code actively worse.
So we need a sane annotation to tell the compiler to shut up already
without making the code an unreadable mess.
> On 14 Apr 2022, at 22:30, Andrew Morton <[email protected]> wrote:
>
> On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <[email protected]> wrote:
>
>>> +/* The + 1 below places the pointers within the range of their array */
>>> #define for_class_range(class, _from, _to) \
>>> - for (class = (_from); class != (_to); class--)
>>> + for (class = (_from); class + 1 != (_to) + 1; class--)
>>
>> Urgh, so now we get less readable code, just because GCC is being
>> stupid?
>>
>> What's wrong with negative array indexes? memory is memory, stuff works.
>
> What's more, C is C. Glorified assembly language in which people do odd
> stuff.
Notably since the advent of clang, we moved a bit beyond glorified assembly language.
There is no 1 on 1 correspondence between what you write and the generated
assembly anymore, by a long shot. I’m sure you know that ;-), but that’s an
opportunity to plug Jason Turner’s conference on writing a C64 pong game using C++17
(https://www.youtube.com/watch?v=zBkNBP00wJE). That demonstrates,
in a funny way, just how far compilers go these days to massage your code.
>
> But this is presumably a released gcc version and we need to do
> something. And presumably, we need to do a backportable something, so
> people can compile older kernels with gcc-12.
Hmm, I must admit I had not considered the backporting implications.
>
> Is it possible to suppress just this warning with a gcc option? And if
> so, are we confident that this warning will never be useful in other
> places in the kernel?
I would advise against it, and not just because of warnings.
With GCC’s ability to track pointers to individual C objects, you can expect
that they will soon start optimising based on that collected knowledge.
An example of useful optimisation based on that knowledge is to
avoid memory reloads, The idea is that a write in array B[] does
not force you to reload all data you already fetched from array A[].
But that requires the compiler to prove that pointers to A[] stay in A[]
and that you don’t purposely build negative indexes from B[] or
anything weird like that.
> If no||no then we'll need to add workarounds such as these?
It is definitely possible to silence that warning. I would still recommend
adding this kind of changes, which I would personally describe more as
“accurate description intended of memory accesses” rather than “workarounds”.
To me, it’s on the same level as putting memory fences, for example.
>