On Thu, Aug 27, 2020 at 10:10 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > On Thu, 27 Aug 2020 at 10:06, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Thu, Aug 27, 2020 at 11:52:50AM +0800, kernel test robot wrote: > > > > > > First bad commit (maybe != root cause): > > > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > head: 15bc20c6af4ceee97a1f90b43c0e386643c071b4 > > > commit: 5fb8ef25803ef33e2eb60b626435828b937bed75 crypto: chacha - move existing library code into lib/crypto > > > date: 9 months ago > > > config: i386-randconfig-r015-20200827 (attached as .config) > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > > > reproduce (this is a W=1 build): > > > git checkout 5fb8ef25803ef33e2eb60b626435828b937bed75 > > > # save the attached .config to linux build tree > > > make W=1 ARCH=i386 > > > > > > If you fix the issue, kindly add following tag as appropriate > > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > > > > > All warnings (new ones prefixed by >>): > > > > > > lib/crypto/chacha.c: In function 'chacha_permute': > > > >> lib/crypto/chacha.c:65:1: warning: the frame size of 1604 bytes is larger than 1024 bytes [-Wframe-larger-than=] > > > 65 | } > > > | ^ > > > > This doesn't happen with a normal configuration. To recreate > > this warning, you need to enable both GCOV_KERNEL and UBSAN. > > > > This is the minimal gcc command-line to recreate it: > > > > gcc -Wframe-larger-than=1024 -fprofile-arcs -fsanitize=object-size -c -O2 chacha.c > > > > If you take away either profile-arcs or sanitize=object-size then > > the problem goes away. > > > > Any suggestions? > > > > Is it really worth it to obsess about this? Special compiler > instrumentation simply leads to a larger stack footprint in many > cases, which is why we use a larger stack to begin with (at least we > do so for Kasan, so if we don't for Ubsan, we should consider it) > > Past experience also shows that this is highly dependent on the exact > compiler version, so issues like these are often moving targets. Yes, I do think it is important to address these in some form, for multiple reasons: * With the limited amount of stack space in normal uninstrumented kernels, I do think it is vital to have a fairly low default warning limit in order to catch all cases that do something dangerously stupid, either because of code bugs or compiler bugs. * I also think we do want the warning enabled in other configurations, in particular because the compiler tends to make increasingly stupid decisions when combining less common instrumentations, which again can lead to instant exploitable bugs, e.g. when a function's stack frame grows beyond the total stack size. In many cases the gcc and clang developers are both able to address these quickly when we send a good bug report (which unfortunately can be a lot of work). * The crypto cipher code unfortunately is particularly prone to running into these issues because each new compiler version tries to find more clever tricks to optimize code that in turn implements an algorithm that intentionally tries to not have any clever shortcuts. In many cases the stack size warning that we see in some configurations is an indicator for the compiler running into a false optimization even on the non-instrumented code that leads to lower performance from unnecessary register spilling that should be avoided. Arnd
