Received-SPF: pass (google.com: domain of linux-kernel+bounces-21304-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 9 Jan 2024 11:24:47 -0800
From: Josh Poimboeuf <jpoimboe@kernel.org>
To: Ingo Molnar <mingo@kernel.org>
Cc: Dimitri John Ledkov <dimitri.ledkov@canonical.com>,
	peterz@infradead.org, x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] objtool: Make objtool check actually fatal upon
 fatal errors
Message-ID: <20240109192447.yhl37mwaw5jdkxjs@treble>
References: <20231213134303.2302285-1-dimitri.ledkov@canonical.com>
 <20231213134303.2302285-2-dimitri.ledkov@canonical.com>
 <ZZu9Nvkp3PdSeLHQ@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZZu9Nvkp3PdSeLHQ@gmail.com>

On Mon, Jan 08, 2024 at 10:15:34AM +0100, Ingo Molnar wrote:
> 
> * Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
> 
> > Currently function calls within check() are sensitive to fatal errors
> > (negative return codes) and abort execution prematurely. However, in
> > all such cases the check() function still returns 0, and thus
> > resulting in a successful kernel build.
> > 
> > The only correct code paths were the ones that escpae the control flow
> > with `return ret`.
> > 
> > Make the check() function return `ret` status code, and make all
> > negative return codes goto that instruction. This makes fatal errors
> > (not warnings) from various function calls actually fail the
> > build. E.g. if create_retpoline_sites_sections() fails to create elf
> > section pair retpoline_sites the tool now exits with an error code.
> > 
> > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
> 
> So, is this not expected to be the case anymore:
> 
> >  out:
> > -	/*
> > -	 *  For now, don't fail the kernel build on fatal warnings.  These
> > -	 *  errors are still fairly common due to the growing matrix of
> > -	 *  supported toolchains and their recent pace of change.
> > -	 */
> > -	return 0;
> 
> ?
> 
> How about making it only fatal if CONFIG_WERROR=y, ie. an analogue to our 
> treatment of compiler warnings?

Objtool has two classes of warnings:

1) "fatal"

   - allocation failures
   - CFG recreation failures
   - annotation parsing errors
   - other objtool bugs

2) non-"fatal":

  - missing security features (retpolines, IBT, SLS INT3)
  - unreachable instructions (note this warning may indicate more
    serious issues like an incomplete or buggy objtool CFG)

The first class of "warning" is actually an error.  It means objtool
couldn't reasonably continue, so it exited early.  I'm thinking this
should always fail the build so it can be reported and fixed ASAP.

We tried doing that before, but ending up reverting it (for raisins).
We should try again (as per the above patch).

The second class of warning, though it doesn't abort objtool, can still
be quite serious.  Ignoring it can fail the boot, or can expose the user
to certain attacks.

My proposal would be to always fail for #1, and to make #2 dependent on
CONFIG_WERROR.

Note the latter may be problematic at the moment due to some outstanding
warnings reported by Arnd and randconfig build bots.  I try to fix those
when I can, but any help would be appreciated.

-- 
Josh