Patches 1-2 are in preparation for patch 3.
Patch 3 adds sanity checking to prevent unsupported relocations in
alternatives.
Josh Poimboeuf (3):
objtool: Fail the kernel build on fatal errors
objtool: Add is_static_jump() helper
objtool: Add relocation check for alternative sections
tools/objtool/check.c | 48 +++++++++++++++++++++++++++++++++++--------
1 file changed, 39 insertions(+), 9 deletions(-)
Josh Poimboeuf (3):
objtool: Fail the kernel build on fatal errors
objtool: Add is_static_jump() helper
objtool: Add relocation check for alternative sections
tools/objtool/check.c | 48 +++++++++++++++++++++++++++++++++++--------
1 file changed, 39 insertions(+), 9 deletions(-)
--
2.21.1
When objtool encounters a fatal error, it usually means the binary is
corrupt or otherwise broken in some way. Up until now, such errors were
just treated as warnings which didn't fail the kernel build.
However, objtool is now stable enough that if a fatal error is
discovered, it most likely means something is seriously wrong and it
should fail the kernel build.
Note that this doesn't apply to "normal" objtool warnings; only fatal
ones.
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b6da413bcbd6..61d2d1877fd2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2509,8 +2509,14 @@ int check(const char *_objname, bool orc)
out:
cleanup(&file);
- /* ignore warnings for now until we get all the code cleaned up */
- if (ret || warnings)
- return 0;
+ if (ret < 0) {
+ /*
+ * Fatal error. The binary is corrupt or otherwise broken in
+ * some way, or objtool itself is broken. Fail the kernel
+ * build.
+ */
+ return ret;
+ }
+
return 0;
}
--
2.21.1
Relocations in alternative code can be dangerous, because the code is
copy/pasted to the text section after relocations have been resolved,
which can corrupt PC-relative addresses.
However, relocations might be acceptable in some cases, depending on the
architecture. For example, the x86 alternatives code manually fixes up
the target addresses for PC-relative jumps and calls.
So disallow relocations in alternative code, except where the x86 arch
code allows it.
This code may need to be tweaked for other arches when objtool gets
support for them.
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5ea2ce7ed8a3..2d52a40e6cb9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -786,6 +786,27 @@ static int handle_group_alt(struct objtool_file *file,
insn->ignore = orig_insn->ignore_alts;
insn->func = orig_insn->func;
+ /*
+ * Since alternative replacement code is copy/pasted by the
+ * kernel after applying relocations, generally such code can't
+ * have relative-address relocation references to outside the
+ * .altinstr_replacement section, unless the arch's
+ * alternatives code can adjust the relative offsets
+ * accordingly.
+ *
+ * The x86 alternatives code adjusts the offsets only when it
+ * encounters a branch instruction at the very beginning of the
+ * replacement group.
+ */
+ if ((insn->offset != special_alt->new_off ||
+ (insn->type != INSN_CALL && !is_static_jump(insn))) &&
+ find_rela_by_dest_range(insn->sec, insn->offset, insn->len)) {
+
+ WARN_FUNC("unsupported relocation in alternatives section",
+ insn->sec, insn->offset);
+ return -1;
+ }
+
if (!is_static_jump(insn))
continue;
--
2.21.1
There are several places where objtool tests for a non-dynamic (aka
direct) jump. Move the check to a helper function.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 61d2d1877fd2..5ea2ce7ed8a3 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -97,14 +97,19 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
for (insn = next_insn_same_sec(file, insn); insn; \
insn = next_insn_same_sec(file, insn))
+static bool is_static_jump(struct instruction *insn)
+{
+ return insn->type == INSN_JUMP_CONDITIONAL ||
+ insn->type == INSN_JUMP_UNCONDITIONAL;
+}
+
static bool is_sibling_call(struct instruction *insn)
{
/* An indirect jump is either a sibling call or a jump to a table. */
if (insn->type == INSN_JUMP_DYNAMIC)
return list_empty(&insn->alts);
- if (insn->type != INSN_JUMP_CONDITIONAL &&
- insn->type != INSN_JUMP_UNCONDITIONAL)
+ if (!is_static_jump(insn))
return false;
/* add_jump_destinations() sets insn->call_dest for sibling calls. */
@@ -571,8 +576,7 @@ static int add_jump_destinations(struct objtool_file *file)
unsigned long dest_off;
for_each_insn(file, insn) {
- if (insn->type != INSN_JUMP_CONDITIONAL &&
- insn->type != INSN_JUMP_UNCONDITIONAL)
+ if (!is_static_jump(insn))
continue;
if (insn->ignore || insn->offset == FAKE_JUMP_OFFSET)
@@ -782,8 +786,7 @@ static int handle_group_alt(struct objtool_file *file,
insn->ignore = orig_insn->ignore_alts;
insn->func = orig_insn->func;
- if (insn->type != INSN_JUMP_CONDITIONAL &&
- insn->type != INSN_JUMP_UNCONDITIONAL)
+ if (!is_static_jump(insn))
continue;
if (!insn->immediate)
--
2.21.1
On Mon, Feb 10, 2020 at 10:33 AM Josh Poimboeuf <[email protected]> wrote:
>
> So disallow relocations in alternative code, except where the x86 arch
> code allows it.
LGTM. Did this actually find anything? And if not, did you verify that
it would by intentionally creating a bad alternative (perhaps the
broken one from my patch?)
Linus
On 2/10/20 6:32 PM, Josh Poimboeuf wrote:
> When objtool encounters a fatal error, it usually means the binary is
> corrupt or otherwise broken in some way. Up until now, such errors were
> just treated as warnings which didn't fail the kernel build.
>
> However, objtool is now stable enough that if a fatal error is
> discovered, it most likely means something is seriously wrong and it
> should fail the kernel build.
>
> Note that this doesn't apply to "normal" objtool warnings; only fatal
> ones.
>
> Suggested-by: Borislav Petkov <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Reviewed-by: Julien Thierry <[email protected]>
> ---
> tools/objtool/check.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index b6da413bcbd6..61d2d1877fd2 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2509,8 +2509,14 @@ int check(const char *_objname, bool orc)
> out:
> cleanup(&file);
>
> - /* ignore warnings for now until we get all the code cleaned up */
> - if (ret || warnings)
> - return 0;
> + if (ret < 0) {
> + /*
> + * Fatal error. The binary is corrupt or otherwise broken in
> + * some way, or objtool itself is broken. Fail the kernel
> + * build.
> + */
> + return ret;
> + }
> +
> return 0;
> }
>
--
Julien Thierry
On 2/10/20 6:32 PM, Josh Poimboeuf wrote:
> There are several places where objtool tests for a non-dynamic (aka
> direct) jump. Move the check to a helper function.
>
Makes sense.
> Signed-off-by: Josh Poimboeuf <[email protected]>
Reviewed-by: Julien Thierry <[email protected]>
> ---
> tools/objtool/check.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 61d2d1877fd2..5ea2ce7ed8a3 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -97,14 +97,19 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
> for (insn = next_insn_same_sec(file, insn); insn; \
> insn = next_insn_same_sec(file, insn))
>
> +static bool is_static_jump(struct instruction *insn)
> +{
> + return insn->type == INSN_JUMP_CONDITIONAL ||
> + insn->type == INSN_JUMP_UNCONDITIONAL;
> +}
> +
> static bool is_sibling_call(struct instruction *insn)
> {
> /* An indirect jump is either a sibling call or a jump to a table. */
> if (insn->type == INSN_JUMP_DYNAMIC)
> return list_empty(&insn->alts);
>
> - if (insn->type != INSN_JUMP_CONDITIONAL &&
> - insn->type != INSN_JUMP_UNCONDITIONAL)
> + if (!is_static_jump(insn))
> return false;
>
> /* add_jump_destinations() sets insn->call_dest for sibling calls. */
> @@ -571,8 +576,7 @@ static int add_jump_destinations(struct objtool_file *file)
> unsigned long dest_off;
>
> for_each_insn(file, insn) {
> - if (insn->type != INSN_JUMP_CONDITIONAL &&
> - insn->type != INSN_JUMP_UNCONDITIONAL)
> + if (!is_static_jump(insn))
> continue;
>
> if (insn->ignore || insn->offset == FAKE_JUMP_OFFSET)
> @@ -782,8 +786,7 @@ static int handle_group_alt(struct objtool_file *file,
> insn->ignore = orig_insn->ignore_alts;
> insn->func = orig_insn->func;
>
> - if (insn->type != INSN_JUMP_CONDITIONAL &&
> - insn->type != INSN_JUMP_UNCONDITIONAL)
> + if (!is_static_jump(insn))
> continue;
>
> if (!insn->immediate)
>
--
Julien Thierry
On 2/10/20 6:32 PM, Josh Poimboeuf wrote:
> Relocations in alternative code can be dangerous, because the code is
> copy/pasted to the text section after relocations have been resolved,
> which can corrupt PC-relative addresses.
>
> However, relocations might be acceptable in some cases, depending on the
> architecture. For example, the x86 alternatives code manually fixes up
> the target addresses for PC-relative jumps and calls.
>
> So disallow relocations in alternative code, except where the x86 arch
> code allows it.
>
> This code may need to be tweaked for other arches when objtool gets
> support for them.
>
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> tools/objtool/check.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 5ea2ce7ed8a3..2d52a40e6cb9 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -786,6 +786,27 @@ static int handle_group_alt(struct objtool_file *file,
> insn->ignore = orig_insn->ignore_alts;
> insn->func = orig_insn->func;
>
> + /*
> + * Since alternative replacement code is copy/pasted by the
> + * kernel after applying relocations, generally such code can't
> + * have relative-address relocation references to outside the
> + * .altinstr_replacement section, unless the arch's
> + * alternatives code can adjust the relative offsets
> + * accordingly.
> + *
> + * The x86 alternatives code adjusts the offsets only when it
> + * encounters a branch instruction at the very beginning of the
> + * replacement group.
Yes, arm64 is a bit more permissive regarding this although it does
disallow some patterns.
I guess once we introduce other archs there should be some function:
bool arch_validate_alt_insn(struct instruction *new, struct instruction
*new, struct special_alt *alt);
> + */
> + if ((insn->offset != special_alt->new_off ||
> + (insn->type != INSN_CALL && !is_static_jump(insn))) &&
> + find_rela_by_dest_range(insn->sec, insn->offset, insn->len)) {
> +
> + WARN_FUNC("unsupported relocation in alternatives section",
> + insn->sec, insn->offset);
> + return -1;
> + }
> +
> if (!is_static_jump(insn))
> continue;
>
>
Reviewed-by: Julien Thierry <[email protected]>
Cheers,
--
Julien Thierry
On Mon, Feb 10, 2020 at 05:51:44PM -0800, Linus Torvalds wrote:
> On Mon, Feb 10, 2020 at 10:33 AM Josh Poimboeuf <[email protected]> wrote:
> LGTM. Did this actually find anything? And if not, did you verify that
> it would by intentionally creating a bad alternative (perhaps the
> broken one from my patch?)
Yeah, we used your patch while playing with this. Lemme do some builds
with this and see what fires. Nothing should, I would strongly assume.
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
The following commit has been merged into the core/objtool branch of tip:
Commit-ID: a22961409c02b93ffa7ed78f67fb57a1ba6c787d
Gitweb: https://git.kernel.org/tip/a22961409c02b93ffa7ed78f67fb57a1ba6c787d
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Mon, 10 Feb 2020 12:32:39 -06:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 11 Feb 2020 13:36:01 +01:00
objtool: Add is_static_jump() helper
There are several places where objtool tests for a non-dynamic (aka
direct) jump. Move the check to a helper function.
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Julien Thierry <[email protected]>
Link: https://lkml.kernel.org/r/9b8b438df918276315e4765c60d2587f3c7ad698.1581359535.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 796f6a1..9016ae1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -97,14 +97,19 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
for (insn = next_insn_same_sec(file, insn); insn; \
insn = next_insn_same_sec(file, insn))
+static bool is_static_jump(struct instruction *insn)
+{
+ return insn->type == INSN_JUMP_CONDITIONAL ||
+ insn->type == INSN_JUMP_UNCONDITIONAL;
+}
+
static bool is_sibling_call(struct instruction *insn)
{
/* An indirect jump is either a sibling call or a jump to a table. */
if (insn->type == INSN_JUMP_DYNAMIC)
return list_empty(&insn->alts);
- if (insn->type != INSN_JUMP_CONDITIONAL &&
- insn->type != INSN_JUMP_UNCONDITIONAL)
+ if (!is_static_jump(insn))
return false;
/* add_jump_destinations() sets insn->call_dest for sibling calls. */
@@ -553,8 +558,7 @@ static int add_jump_destinations(struct objtool_file *file)
unsigned long dest_off;
for_each_insn(file, insn) {
- if (insn->type != INSN_JUMP_CONDITIONAL &&
- insn->type != INSN_JUMP_UNCONDITIONAL)
+ if (!is_static_jump(insn))
continue;
if (insn->ignore || insn->offset == FAKE_JUMP_OFFSET)
@@ -764,8 +768,7 @@ static int handle_group_alt(struct objtool_file *file,
insn->ignore = orig_insn->ignore_alts;
insn->func = orig_insn->func;
- if (insn->type != INSN_JUMP_CONDITIONAL &&
- insn->type != INSN_JUMP_UNCONDITIONAL)
+ if (!is_static_jump(insn))
continue;
if (!insn->immediate)
The following commit has been merged into the core/objtool branch of tip:
Commit-ID: dc4197236c20e761f2007c641afd193f81a00a74
Gitweb: https://git.kernel.org/tip/dc4197236c20e761f2007c641afd193f81a00a74
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Mon, 10 Feb 2020 12:32:40 -06:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 11 Feb 2020 13:39:52 +01:00
objtool: Add relocation check for alternative sections
Relocations in alternative code can be dangerous, because the code is
copy/pasted to the text section after relocations have been resolved,
which can corrupt PC-relative addresses.
However, relocations might be acceptable in some cases, depending on the
architecture. For example, the x86 alternatives code manually fixes up
the target addresses for PC-relative jumps and calls.
So disallow relocations in alternative code, except where the x86 arch
code allows it.
This code may need to be tweaked for other arches when objtool gets
support for them.
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Julien Thierry <[email protected]>
Link: https://lkml.kernel.org/r/7b90b68d093311e4e8f6b504a9e1c758fd7e0002.1581359535.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9016ae1..b038de2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -768,6 +768,27 @@ static int handle_group_alt(struct objtool_file *file,
insn->ignore = orig_insn->ignore_alts;
insn->func = orig_insn->func;
+ /*
+ * Since alternative replacement code is copy/pasted by the
+ * kernel after applying relocations, generally such code can't
+ * have relative-address relocation references to outside the
+ * .altinstr_replacement section, unless the arch's
+ * alternatives code can adjust the relative offsets
+ * accordingly.
+ *
+ * The x86 alternatives code adjusts the offsets only when it
+ * encounters a branch instruction at the very beginning of the
+ * replacement group.
+ */
+ if ((insn->offset != special_alt->new_off ||
+ (insn->type != INSN_CALL && !is_static_jump(insn))) &&
+ find_rela_by_dest_range(insn->sec, insn->offset, insn->len)) {
+
+ WARN_FUNC("unsupported relocation in alternatives section",
+ insn->sec, insn->offset);
+ return -1;
+ }
+
if (!is_static_jump(insn))
continue;
The following commit has been merged into the core/objtool branch of tip:
Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
objtool: Fail the kernel build on fatal errors
When objtool encounters a fatal error, it usually means the binary is
corrupt or otherwise broken in some way. Up until now, such errors were
just treated as warnings which didn't fail the kernel build.
However, objtool is now stable enough that if a fatal error is
discovered, it most likely means something is seriously wrong and it
should fail the kernel build.
Note that this doesn't apply to "normal" objtool warnings; only fatal
ones.
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Julien Thierry <[email protected]>
Link: https://lkml.kernel.org/r/f18c3743de0fef673d49dd35760f26bdef7f6fc3.1581359535.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4768d91..796f6a1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2491,8 +2491,14 @@ int check(const char *_objname, bool orc)
out:
cleanup(&file);
- /* ignore warnings for now until we get all the code cleaned up */
- if (ret || warnings)
- return 0;
+ if (ret < 0) {
+ /*
+ * Fatal error. The binary is corrupt or otherwise broken in
+ * some way, or objtool itself is broken. Fail the kernel
+ * build.
+ */
+ return ret;
+ }
+
return 0;
}
On Tue, Feb 11, 2020 at 12:47:38PM -0000, tip-bot2 for Josh Poimboeuf wrote:
> The following commit has been merged into the core/objtool branch of tip:
>
> Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
> Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
> Author: Josh Poimboeuf <[email protected]>
> AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
> Committer: Borislav Petkov <[email protected]>
> CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
>
> objtool: Fail the kernel build on fatal errors
>
> When objtool encounters a fatal error, it usually means the binary is
> corrupt or otherwise broken in some way. Up until now, such errors were
> just treated as warnings which didn't fail the kernel build.
>
> However, objtool is now stable enough that if a fatal error is
> discovered, it most likely means something is seriously wrong and it
> should fail the kernel build.
>
> Note that this doesn't apply to "normal" objtool warnings; only fatal
> ones.
Clang still has some toolchain issues which need to be sorted out, so
upgrading the fatal errors is causing their CI to fail.
So I think we need to drop this one for now.
Boris, are you able to just drop it or should I send a revert?
--
Josh
Josh Poimboeuf <[email protected]> writes:
> On Tue, Feb 11, 2020 at 12:47:38PM -0000, tip-bot2 for Josh Poimboeuf wrote:
>> The following commit has been merged into the core/objtool branch of tip:
>>
>> Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
>> Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
>> Author: Josh Poimboeuf <[email protected]>
>> AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
>> Committer: Borislav Petkov <[email protected]>
>> CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
>>
>> objtool: Fail the kernel build on fatal errors
>>
>> When objtool encounters a fatal error, it usually means the binary is
>> corrupt or otherwise broken in some way. Up until now, such errors were
>> just treated as warnings which didn't fail the kernel build.
>>
>> However, objtool is now stable enough that if a fatal error is
>> discovered, it most likely means something is seriously wrong and it
>> should fail the kernel build.
>>
>> Note that this doesn't apply to "normal" objtool warnings; only fatal
>> ones.
>
> Clang still has some toolchain issues which need to be sorted out, so
> upgrading the fatal errors is causing their CI to fail.
Good. Last time we made it fail they just fixed their stuff.
> So I think we need to drop this one for now.
Why? It's our decision to define which level of toolchain brokeness is
tolerable.
> Boris, are you able to just drop it or should I send a revert?
I really want to see a revert which has a proper justification why the
issues of clang are tolerable along with a clear statement when this
fatal error will come back. And 'when' means a date, not 'when clang is
fixed'.
Thanks,
tglx
On Fri, Feb 14, 2020 at 01:10:26AM +0100, Thomas Gleixner wrote:
> Josh Poimboeuf <[email protected]> writes:
> > On Tue, Feb 11, 2020 at 12:47:38PM -0000, tip-bot2 for Josh Poimboeuf wrote:
> >> The following commit has been merged into the core/objtool branch of tip:
> >>
> >> Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
> >> Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
> >> Author: Josh Poimboeuf <[email protected]>
> >> AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
> >> Committer: Borislav Petkov <[email protected]>
> >> CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
> >>
> >> objtool: Fail the kernel build on fatal errors
> >>
> >> When objtool encounters a fatal error, it usually means the binary is
> >> corrupt or otherwise broken in some way. Up until now, such errors were
> >> just treated as warnings which didn't fail the kernel build.
> >>
> >> However, objtool is now stable enough that if a fatal error is
> >> discovered, it most likely means something is seriously wrong and it
> >> should fail the kernel build.
> >>
> >> Note that this doesn't apply to "normal" objtool warnings; only fatal
> >> ones.
> >
> > Clang still has some toolchain issues which need to be sorted out, so
> > upgrading the fatal errors is causing their CI to fail.
>
> Good. Last time we made it fail they just fixed their stuff.
>
> > So I think we need to drop this one for now.
>
> Why? It's our decision to define which level of toolchain brokeness is
> tolerable.
>
> > Boris, are you able to just drop it or should I send a revert?
>
> I really want to see a revert which has a proper justification why the
> issues of clang are tolerable along with a clear statement when this
> fatal error will come back. And 'when' means a date, not 'when clang is
> fixed'.
Fair enough. The root cause was actually a bug in binutils which gets
triggered by a new clang feature. So instead of reverting the above
patch, I think I've figured out a way to work around the binutils bug,
while also improving objtool at the same time (win-win).
The binutils bug will be fixed in binutils 2.35.
BTW, to be fair, this was less "Clang has issues" and more "Josh is
lazy". I didn't test the patch with Clang -- I tend to rely on 0-day
bot reports because I don't have the bandwidth to test the
kernel/config/toolchain combinations. Nick tells me Clang will soon be
integrated with the 0-day bot, which should help prevent this type of
thing in the future.
--
Josh
On Fri, Feb 14, 2020 at 9:58 AM Josh Poimboeuf <[email protected]> wrote:
>
> On Fri, Feb 14, 2020 at 01:10:26AM +0100, Thomas Gleixner wrote:
> > Josh Poimboeuf <[email protected]> writes:
> > > On Tue, Feb 11, 2020 at 12:47:38PM -0000, tip-bot2 for Josh Poimboeuf wrote:
> > >> The following commit has been merged into the core/objtool branch of tip:
> > >>
> > >> Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
> > >> Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
> > >> Author: Josh Poimboeuf <[email protected]>
> > >> AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
> > >> Committer: Borislav Petkov <[email protected]>
> > >> CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
> > >>
> > >> objtool: Fail the kernel build on fatal errors
> > >>
> > >> When objtool encounters a fatal error, it usually means the binary is
> > >> corrupt or otherwise broken in some way. Up until now, such errors were
> > >> just treated as warnings which didn't fail the kernel build.
> > >>
> > >> However, objtool is now stable enough that if a fatal error is
> > >> discovered, it most likely means something is seriously wrong and it
> > >> should fail the kernel build.
> > >>
> > >> Note that this doesn't apply to "normal" objtool warnings; only fatal
> > >> ones.
> > >
> > > Clang still has some toolchain issues which need to be sorted out, so
> > > upgrading the fatal errors is causing their CI to fail.
> >
> > Good. Last time we made it fail they just fixed their stuff.
> >
> > > So I think we need to drop this one for now.
> >
> > Why? It's our decision to define which level of toolchain brokeness is
> > tolerable.
> >
> > > Boris, are you able to just drop it or should I send a revert?
> >
> > I really want to see a revert which has a proper justification why the
> > issues of clang are tolerable along with a clear statement when this
> > fatal error will come back. And 'when' means a date, not 'when clang is
> > fixed'.
>
> Fair enough. The root cause was actually a bug in binutils which gets
> triggered by a new clang feature. So instead of reverting the above
> patch, I think I've figured out a way to work around the binutils bug,
> while also improving objtool at the same time (win-win).
>
> The binutils bug will be fixed in binutils 2.35.
>
> BTW, to be fair, this was less "Clang has issues" and more "Josh is
> lazy". I didn't test the patch with Clang -- I tend to rely on 0-day
> bot reports because I don't have the bandwidth to test the
> kernel/config/toolchain combinations. Nick tells me Clang will soon be
> integrated with the 0-day bot, which should help prevent this type of
> thing in the future.
Hi Rong, Philip,
Do you have any status updates on turning on the 0day bot emails to
the patch authors in production? It's been quite handy in helping us
find issues, for the private mails we've been triaging daily.
--
Thanks,
~Nick Desaulniers
On Wed, Feb 19, 2020 at 02:43:39PM -0800, Nick Desaulniers wrote:
> On Fri, Feb 14, 2020 at 9:58 AM Josh Poimboeuf <[email protected]> wrote:
> >
> > On Fri, Feb 14, 2020 at 01:10:26AM +0100, Thomas Gleixner wrote:
> > > Josh Poimboeuf <[email protected]> writes:
> > > > On Tue, Feb 11, 2020 at 12:47:38PM -0000, tip-bot2 for Josh Poimboeuf wrote:
> > > >> The following commit has been merged into the core/objtool branch of tip:
> > > >>
> > > >> Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
> > > >> Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
> > > >> Author: Josh Poimboeuf <[email protected]>
> > > >> AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
> > > >> Committer: Borislav Petkov <[email protected]>
> > > >> CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
> > > >>
> > > >> objtool: Fail the kernel build on fatal errors
> > > >>
> > > >> When objtool encounters a fatal error, it usually means the binary is
> > > >> corrupt or otherwise broken in some way. Up until now, such errors were
> > > >> just treated as warnings which didn't fail the kernel build.
> > > >>
> > > >> However, objtool is now stable enough that if a fatal error is
> > > >> discovered, it most likely means something is seriously wrong and it
> > > >> should fail the kernel build.
> > > >>
> > > >> Note that this doesn't apply to "normal" objtool warnings; only fatal
> > > >> ones.
> > > >
> > > > Clang still has some toolchain issues which need to be sorted out, so
> > > > upgrading the fatal errors is causing their CI to fail.
> > >
> > > Good. Last time we made it fail they just fixed their stuff.
> > >
> > > > So I think we need to drop this one for now.
> > >
> > > Why? It's our decision to define which level of toolchain brokeness is
> > > tolerable.
> > >
> > > > Boris, are you able to just drop it or should I send a revert?
> > >
> > > I really want to see a revert which has a proper justification why the
> > > issues of clang are tolerable along with a clear statement when this
> > > fatal error will come back. And 'when' means a date, not 'when clang is
> > > fixed'.
> >
> > Fair enough. The root cause was actually a bug in binutils which gets
> > triggered by a new clang feature. So instead of reverting the above
> > patch, I think I've figured out a way to work around the binutils bug,
> > while also improving objtool at the same time (win-win).
> >
> > The binutils bug will be fixed in binutils 2.35.
> >
> > BTW, to be fair, this was less "Clang has issues" and more "Josh is
> > lazy". I didn't test the patch with Clang -- I tend to rely on 0-day
> > bot reports because I don't have the bandwidth to test the
> > kernel/config/toolchain combinations. Nick tells me Clang will soon be
> > integrated with the 0-day bot, which should help prevent this type of
> > thing in the future.
>
> Hi Rong, Philip,
> Do you have any status updates on turning on the 0day bot emails to
> the patch authors in production? It's been quite handy in helping us
> find issues, for the private mails we've been triaging daily.
Hi Nick, this is on our schedule in a new 2-3 weeks, sorry not to update
your in another mail loop earlier.
What I plan to do is to cc you for the clang reports when 0-day ci sends
to kernel patch author. If you notice something may be related to clang (since
we always integrate newer clang version), you can help filter it out. How
do you think?
> --
> Thanks,
> ~Nick Desaulniers
(everyone else to bcc)
On Wed, Feb 19, 2020 at 4:44 PM Philip Li <[email protected]> wrote:
>
> On Wed, Feb 19, 2020 at 02:43:39PM -0800, Nick Desaulniers wrote:
> > On Fri, Feb 14, 2020 at 9:58 AM Josh Poimboeuf <[email protected]> wrote:
> > >
> > > On Fri, Feb 14, 2020 at 01:10:26AM +0100, Thomas Gleixner wrote:
> > > > Josh Poimboeuf <[email protected]> writes:
> > > > > On Tue, Feb 11, 2020 at 12:47:38PM -0000, tip-bot2 for Josh Poimboeuf wrote:
> > > > >> The following commit has been merged into the core/objtool branch of tip:
> > > > >>
> > > > >> Commit-ID: 644592d328370af4b3e027b7b1ae9f81613782d8
> > > > >> Gitweb: https://git.kernel.org/tip/644592d328370af4b3e027b7b1ae9f81613782d8
> > > > >> Author: Josh Poimboeuf <[email protected]>
> > > > >> AuthorDate: Mon, 10 Feb 2020 12:32:38 -06:00
> > > > >> Committer: Borislav Petkov <[email protected]>
> > > > >> CommitterDate: Tue, 11 Feb 2020 13:27:03 +01:00
> > > > >>
> > > > >> objtool: Fail the kernel build on fatal errors
> > > > >>
> > > > >> When objtool encounters a fatal error, it usually means the binary is
> > > > >> corrupt or otherwise broken in some way. Up until now, such errors were
> > > > >> just treated as warnings which didn't fail the kernel build.
> > > > >>
> > > > >> However, objtool is now stable enough that if a fatal error is
> > > > >> discovered, it most likely means something is seriously wrong and it
> > > > >> should fail the kernel build.
> > > > >>
> > > > >> Note that this doesn't apply to "normal" objtool warnings; only fatal
> > > > >> ones.
> > > > >
> > > > > Clang still has some toolchain issues which need to be sorted out, so
> > > > > upgrading the fatal errors is causing their CI to fail.
> > > >
> > > > Good. Last time we made it fail they just fixed their stuff.
> > > >
> > > > > So I think we need to drop this one for now.
> > > >
> > > > Why? It's our decision to define which level of toolchain brokeness is
> > > > tolerable.
> > > >
> > > > > Boris, are you able to just drop it or should I send a revert?
> > > >
> > > > I really want to see a revert which has a proper justification why the
> > > > issues of clang are tolerable along with a clear statement when this
> > > > fatal error will come back. And 'when' means a date, not 'when clang is
> > > > fixed'.
> > >
> > > Fair enough. The root cause was actually a bug in binutils which gets
> > > triggered by a new clang feature. So instead of reverting the above
> > > patch, I think I've figured out a way to work around the binutils bug,
> > > while also improving objtool at the same time (win-win).
> > >
> > > The binutils bug will be fixed in binutils 2.35.
> > >
> > > BTW, to be fair, this was less "Clang has issues" and more "Josh is
> > > lazy". I didn't test the patch with Clang -- I tend to rely on 0-day
> > > bot reports because I don't have the bandwidth to test the
> > > kernel/config/toolchain combinations. Nick tells me Clang will soon be
> > > integrated with the 0-day bot, which should help prevent this type of
> > > thing in the future.
> >
> > Hi Rong, Philip,
> > Do you have any status updates on turning on the 0day bot emails to
> > the patch authors in production? It's been quite handy in helping us
> > find issues, for the private mails we've been triaging daily.
> Hi Nick, this is on our schedule in a new 2-3 weeks, sorry not to update
> your in another mail loop earlier.
No worries.
>
> What I plan to do is to cc you for the clang reports when 0-day ci sends
> to kernel patch author. If you notice something may be related to clang (since
> we always integrate newer clang version), you can help filter it out. How
> do you think?
If you would kindly cc our mailing list "clang-built-linux
<[email protected]>" we'd be happy to continue to
triage and provide suggestions. That level of indirection better
allows us to deal with subscriptions and change of email addresses
without having to disturb you.
--
Thanks,
~Nick Desaulniers