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 <[email protected]>
---
tools/objtool/check.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e94756e09c..9146177fc9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4669,166 +4669,168 @@ static void free_insns(struct objtool_file *file)
int check(struct objtool_file *file)
{
int ret, warnings = 0;
arch_initial_func_cfi_state(&initial_func_cfi);
init_cfi_state(&init_cfi);
init_cfi_state(&func_cfi);
set_func_state(&func_cfi);
init_cfi_state(&force_undefined_cfi);
force_undefined_cfi.force_undefined = true;
- if (!cfi_hash_alloc(1UL << (file->elf->symbol_bits - 3)))
+ if (!cfi_hash_alloc(1UL << (file->elf->symbol_bits - 3))) {
+ ret = -1;
goto out;
+ }
cfi_hash_add(&init_cfi);
cfi_hash_add(&func_cfi);
ret = decode_sections(file);
if (ret < 0)
goto out;
warnings += ret;
if (!nr_insns)
goto out;
if (opts.retpoline) {
ret = validate_retpoline(file);
if (ret < 0)
- return ret;
+ goto out;
warnings += ret;
}
if (opts.stackval || opts.orc || opts.uaccess) {
ret = validate_functions(file);
if (ret < 0)
goto out;
warnings += ret;
ret = validate_unwind_hints(file, NULL);
if (ret < 0)
goto out;
warnings += ret;
if (!warnings) {
ret = validate_reachable_instructions(file);
if (ret < 0)
goto out;
warnings += ret;
}
} else if (opts.noinstr) {
ret = validate_noinstr_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.unret) {
/*
* Must be after validate_branch() and friends, it plays
* further games with insn->visited.
*/
ret = validate_unrets(file);
if (ret < 0)
- return ret;
+ goto out;
warnings += ret;
}
if (opts.ibt) {
ret = validate_ibt(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.sls) {
ret = validate_sls(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.static_call) {
ret = create_static_call_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.retpoline) {
ret = create_retpoline_sites_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.cfi) {
ret = create_cfi_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.rethunk) {
ret = create_return_sites_sections(file);
if (ret < 0)
goto out;
warnings += ret;
if (opts.hack_skylake) {
ret = create_direct_call_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
}
if (opts.mcount) {
ret = create_mcount_loc_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.prefix) {
ret = add_prefix_symbols(file);
if (ret < 0)
- return ret;
+ goto out;
warnings += ret;
}
if (opts.ibt) {
ret = create_ibt_endbr_seal_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
if (opts.orc && nr_insns) {
ret = orc_create(file);
if (ret < 0)
goto out;
warnings += ret;
}
free_insns(file);
if (opts.verbose)
disas_warned_funcs(file);
if (opts.stats) {
printf("nr_insns_visited: %ld\n", nr_insns_visited);
printf("nr_cfi: %ld\n", nr_cfi);
printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
}
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;
+ return ret;
}
--
2.34.1
On Tue, Dec 12, 2023 at 06:53:38PM +0000, Dimitri John Ledkov 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 <[email protected]>
We had problems trying this before, but we can try again. Maybe we'll
have better luck now :-)
> if (opts.orc && nr_insns) {
> ret = orc_create(file);
> if (ret < 0)
> goto out;
> warnings += ret;
> }
>
> free_insns(file);
>
> if (opts.verbose)
> disas_warned_funcs(file);
>
> if (opts.stats) {
> printf("nr_insns_visited: %ld\n", nr_insns_visited);
> printf("nr_cfi: %ld\n", nr_cfi);
> printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
> printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
> }
>
> 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;
> + return ret;
Here it should check for ret < 0, since orc_create() or some other
function might have returned non-fatal warnings (ret > 0).
Also the comment is no longer relevant.
--
Josh
On Tue, 12 Dec 2023 at 20:30, Josh Poimboeuf <[email protected]> wrote:
>
> On Tue, Dec 12, 2023 at 06:53:38PM +0000, Dimitri John Ledkov 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 <[email protected]>
>
> We had problems trying this before, but we can try again. Maybe we'll
> have better luck now :-)
Well there are two classes of things - positive warnings count &
negative fatal errors
And yes I do want to add a toggle for making warnings errors.
>
> > if (opts.orc && nr_insns) {
> > ret = orc_create(file);
> > if (ret < 0)
> > goto out;
> > warnings += ret;
> > }
> >
> > free_insns(file);
> >
> > if (opts.verbose)
> > disas_warned_funcs(file);
> >
> > if (opts.stats) {
> > printf("nr_insns_visited: %ld\n", nr_insns_visited);
> > printf("nr_cfi: %ld\n", nr_cfi);
> > printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
> > printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
> > }
> >
> > 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;
> > + return ret;
>
> Here it should check for ret < 0, since orc_create() or some other
> function might have returned non-fatal warnings (ret > 0).
Yes that's true... I had it in, and then removed, but I didn't double
check if each of the possible last ret assignments are guaranteed to
not be above 0.
>
> Also the comment is no longer relevant.
>
Well, I'm hoping to add --error option next, and yeah, make warnings
fatal. Or for example at least make the retpoline, sls, rethumb ones
to be fatal - because i found myself in a very surprising situation
where retpoline enabled build of kernel, had non-retpoline paths
remaining and not otherwise silenced as safe.
Maybe I should finish that second patch and make it available as a
config option.
At least right now, with Ubuntu Noble (development series) and
v6.7-rc4 everything is squeaky clean to enable CONFIG_OBJTOOL_WERROR=y
and make all warnings, fatal.
Also i think having it as a config option will result in various
automation tools able to flip it on, and submit bug reports when
something somewhere regresses.
--
Dimitri
Sent from Ubuntu Pro
https://ubuntu.com/pro
On Tue, Dec 12, 2023 at 08:39:30PM +0000, Dimitri John Ledkov wrote:
> Well, I'm hoping to add --error option next, and yeah, make warnings
> fatal. Or for example at least make the retpoline, sls, rethumb ones
> to be fatal - because i found myself in a very surprising situation
> where retpoline enabled build of kernel, had non-retpoline paths
> remaining and not otherwise silenced as safe.
>
> Maybe I should finish that second patch and make it available as a
> config option.
>
> At least right now, with Ubuntu Noble (development series) and
> v6.7-rc4 everything is squeaky clean to enable CONFIG_OBJTOOL_WERROR=y
> and make all warnings, fatal.
>
> Also i think having it as a config option will result in various
> automation tools able to flip it on, and submit bug reports when
> something somewhere regresses.
Right, the warnings should never be ignored.
I agree we need CONFIG_OBJTOOL_WERROR. Problem is, upstream supports a
lot more than just Ubuntu configs, and there are several outstanding
warnings, reported by both bots and humans.
Fixing them is on my TODO list, it's just that other things are taking
priority.
If we introduce CONFIG_OBJTOOL_WERROR now, build bots will start
reporting build failures and people will start complaining more loudly.
Without more help I'm lacking the bandwidth to stay on top of that.
--
Josh