Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Apr 2022 11:45:05 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Christophe de Dinechin <dinechin@redhat.com>,
        trivial@kernel.org, Ben Segall <bsegall@google.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>, Mel Gorman <mgorman@suse.de>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org,
        Zhen Lei <thunder.leizhen@huawei.com>,
        Juri Lelli <juri.lelli@redhat.com>
Subject: Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
Message-ID: <202204201117.F44DCF9@keescook>
References: <20220414150855.2407137-1-dinechin@redhat.com>
 <20220414150855.2407137-2-dinechin@redhat.com>
 <Ylg73c83AJGwz9UN@hirez.programming.kicks-ass.net>
 <20220414133050.b820fa45d42de4cfc24db82b@linux-foundation.org>
 <20220417155205.GI2731@worktop.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220417155205.GI2731@worktop.programming.kicks-ass.net>
Precedence: bulk

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 <peterz@infradead.org> 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