2020-09-17 08:51:23

by Ilie Halip

[permalink] [raw]
Subject: [PATCH] objtool: ignore unreachable trap after call to noreturn functions

With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap instruction
after a call to a noreturn function. In this case, objtool warns that the
ud2 instruction is unreachable.

objtool silences similar warnings (trap after dead end instructions), so
expand that check to include dead end functions.

Cc: Nick Desaulniers <[email protected]>
Cc: Rong Chen <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Philip Li <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Ilie Halip <[email protected]>
---
tools/objtool/check.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e034a8f24f46..eddf8bf16b05 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2612,9 +2612,10 @@ static bool is_ubsan_insn(struct instruction *insn)
"__ubsan_handle_builtin_unreachable"));
}

-static bool ignore_unreachable_insn(struct instruction *insn)
+static bool ignore_unreachable_insn(struct objtool_file *file, struct instruction *insn)
{
int i;
+ struct instruction *prev_insn;

if (insn->ignore || insn->type == INSN_NOP)
return true;
@@ -2639,8 +2640,11 @@ static bool ignore_unreachable_insn(struct instruction *insn)
* __builtin_unreachable(). The BUG() macro has an unreachable() after
* the UD2, which causes GCC's undefined trap logic to emit another UD2
* (or occasionally a JMP to UD2).
+ * CONFIG_UBSAN_TRAP may also insert a UD2 after calling a __noreturn
+ * function.
*/
- if (list_prev_entry(insn, list)->dead_end &&
+ prev_insn = list_prev_entry(insn, list);
+ if ((prev_insn->dead_end || dead_end_function(file, prev_insn->call_dest)) &&
(insn->type == INSN_BUG ||
(insn->type == INSN_JUMP_UNCONDITIONAL &&
insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
@@ -2767,7 +2771,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
return 0;

for_each_insn(file, insn) {
- if (insn->visited || ignore_unreachable_insn(insn))
+ if (insn->visited || ignore_unreachable_insn(file, insn))
continue;

WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
--
2.25.1


2020-09-17 18:20:17

by Nick Desaulniers

[permalink] [raw]
Subject: Re: [PATCH] objtool: ignore unreachable trap after call to noreturn functions

On Thu, Sep 17, 2020 at 1:49 AM Ilie Halip <[email protected]> wrote:
>
> With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap instruction
> after a call to a noreturn function. In this case, objtool warns that the
> ud2 instruction is unreachable.
>
> objtool silences similar warnings (trap after dead end instructions), so
> expand that check to include dead end functions.

Reviewed-by: Nick Desaulniers <[email protected]>
Tested-by: Nick Desaulniers <[email protected]>
Reported-by: kbuild test robot <[email protected]>

Thanks for the patch, Ilie! With this, a
defconfig+CONFIG_UBSAN+CONFIG_UBSAN_TRAP goes from 82 unreachable
instruction warnings from objtool to 0.

$ make LLVM=1 -j71 defconfig
$ make LLVM=1 -j71 menuconfig
<enable UBSAN and UBSAN_TRAP>
$ make LLVM=1 -j71 2> log.txt
$ grep warning: log.txt | cut -d ' ' -f 2- | sort | uniq -c | wc -l
82
$ b4 am https://lore.kernel.org/lkml/[email protected]/
-o - | git am
$ make LLVM=1 -j71 clean
$ make LLVM=1 -j71 2> log.txt
$ grep warning: log.txt | cut -d ' ' -f 2- | sort | uniq -c | wc -l
0

>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Rong Chen <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Philip Li <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
> Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Ilie Halip <[email protected]>
> ---
> tools/objtool/check.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e034a8f24f46..eddf8bf16b05 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2612,9 +2612,10 @@ static bool is_ubsan_insn(struct instruction *insn)
> "__ubsan_handle_builtin_unreachable"));
> }
>
> -static bool ignore_unreachable_insn(struct instruction *insn)
> +static bool ignore_unreachable_insn(struct objtool_file *file, struct instruction *insn)
> {
> int i;
> + struct instruction *prev_insn;
>
> if (insn->ignore || insn->type == INSN_NOP)
> return true;
> @@ -2639,8 +2640,11 @@ static bool ignore_unreachable_insn(struct instruction *insn)
> * __builtin_unreachable(). The BUG() macro has an unreachable() after
> * the UD2, which causes GCC's undefined trap logic to emit another UD2
> * (or occasionally a JMP to UD2).
> + * CONFIG_UBSAN_TRAP may also insert a UD2 after calling a __noreturn
> + * function.
> */
> - if (list_prev_entry(insn, list)->dead_end &&
> + prev_insn = list_prev_entry(insn, list);
> + if ((prev_insn->dead_end || dead_end_function(file, prev_insn->call_dest)) &&
> (insn->type == INSN_BUG ||
> (insn->type == INSN_JUMP_UNCONDITIONAL &&
> insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
> @@ -2767,7 +2771,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
> return 0;
>
> for_each_insn(file, insn) {
> - if (insn->visited || ignore_unreachable_insn(insn))
> + if (insn->visited || ignore_unreachable_insn(file, insn))
> continue;
>
> WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
> --
> 2.25.1
>


--
Thanks,
~Nick Desaulniers

2020-09-17 22:19:34

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH] objtool: ignore unreachable trap after call to noreturn functions

On Thu, Sep 17, 2020 at 11:49:04AM +0300, Ilie Halip wrote:
> With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap instruction
> after a call to a noreturn function. In this case, objtool warns that the
> ud2 instruction is unreachable.
>
> objtool silences similar warnings (trap after dead end instructions), so
> expand that check to include dead end functions.
>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Rong Chen <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Philip Li <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
> Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Ilie Halip <[email protected]>

The patch looks good to me. Which versions of Clang do the trap after
noreturn call? It would be good to have that in the commit message.

--
Josh

2020-09-18 05:39:30

by Ilie Halip

[permalink] [raw]
Subject: Re: [PATCH] objtool: ignore unreachable trap after call to noreturn functions

> The patch looks good to me. Which versions of Clang do the trap after
> noreturn call? It would be good to have that in the commit message.

I omitted this because it happens with all versions of clang that are
supported for building the kernel. clang-9 is the oldest version that
could build the mainline x86_64 kernel right now, and it has the same
behavior.

Should I send a v2 with this info?

I.H.

2020-09-18 15:53:39

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH] objtool: ignore unreachable trap after call to noreturn functions

On Fri, Sep 18, 2020 at 08:35:40AM +0300, Ilie Halip wrote:
> > The patch looks good to me. Which versions of Clang do the trap after
> > noreturn call? It would be good to have that in the commit message.
>
> I omitted this because it happens with all versions of clang that are
> supported for building the kernel. clang-9 is the oldest version that
> could build the mainline x86_64 kernel right now, and it has the same
> behavior.

Ok. It should at least mention that this is a Clang-specific thing,
since GCC's version of UBSAN_TRAP doesn't do it.

> Should I send a v2 with this info?

Yes, please.

--
Josh

2020-09-19 06:45:59

by Ilie Halip

[permalink] [raw]
Subject: [PATCH v2] objtool: ignore unreachable trap after call to noreturn functions

With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap instruction
after a call to a noreturn function. In this case, objtool warns that the
ud2 instruction is unreachable.

This is a behavior seen with clang, from the oldest version capable of
building the mainline x64_64 kernel (9.0), to the latest experimental
version (12.0).

objtool silences similar warnings (trap after dead end instructions), so
so expand that check to include dead end functions.

Cc: Nick Desaulniers <[email protected]>
Cc: Rong Chen <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Philip Li <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
Suggested-by: Nick Desaulniers <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
Tested-by: Nick Desaulniers <[email protected]>
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Ilie Halip <[email protected]>
---

Changed in v2:
- added a mention that this is a clang issue across all versions
- added Nick's Reviewed-by, Tested-by
- added Reported-by

tools/objtool/check.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e034a8f24f46..eddf8bf16b05 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2612,9 +2612,10 @@ static bool is_ubsan_insn(struct instruction *insn)
"__ubsan_handle_builtin_unreachable"));
}

-static bool ignore_unreachable_insn(struct instruction *insn)
+static bool ignore_unreachable_insn(struct objtool_file *file, struct instruction *insn)
{
int i;
+ struct instruction *prev_insn;

if (insn->ignore || insn->type == INSN_NOP)
return true;
@@ -2639,8 +2640,11 @@ static bool ignore_unreachable_insn(struct instruction *insn)
* __builtin_unreachable(). The BUG() macro has an unreachable() after
* the UD2, which causes GCC's undefined trap logic to emit another UD2
* (or occasionally a JMP to UD2).
+ * CONFIG_UBSAN_TRAP may also insert a UD2 after calling a __noreturn
+ * function.
*/
- if (list_prev_entry(insn, list)->dead_end &&
+ prev_insn = list_prev_entry(insn, list);
+ if ((prev_insn->dead_end || dead_end_function(file, prev_insn->call_dest)) &&
(insn->type == INSN_BUG ||
(insn->type == INSN_JUMP_UNCONDITIONAL &&
insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
@@ -2767,7 +2771,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
return 0;

for_each_insn(file, insn) {
- if (insn->visited || ignore_unreachable_insn(insn))
+ if (insn->visited || ignore_unreachable_insn(file, insn))
continue;

WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
--
2.25.1

2020-09-19 11:04:29

by Sedat Dilek

[permalink] [raw]
Subject: Re: [PATCH v2] objtool: ignore unreachable trap after call to noreturn functions

On Sat, Sep 19, 2020 at 8:41 AM Ilie Halip <[email protected]> wrote:
>
> With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap instruction
> after a call to a noreturn function. In this case, objtool warns that the
> ud2 instruction is unreachable.
>
> This is a behavior seen with clang, from the oldest version capable of
> building the mainline x64_64 kernel (9.0), to the latest experimental
> version (12.0).
>
> objtool silences similar warnings (trap after dead end instructions), so
> so expand that check to include dead end functions.
>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Rong Chen <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Philip Li <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
> Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
> Suggested-by: Nick Desaulniers <[email protected]>
> Reviewed-by: Nick Desaulniers <[email protected]>
> Tested-by: Nick Desaulniers <[email protected]>
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Ilie Halip <[email protected]>
> ---

Tested-by: Sedat Dilek <[email protected]>

- Sedat -

>
> Changed in v2:
> - added a mention that this is a clang issue across all versions
> - added Nick's Reviewed-by, Tested-by
> - added Reported-by
>
> tools/objtool/check.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e034a8f24f46..eddf8bf16b05 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2612,9 +2612,10 @@ static bool is_ubsan_insn(struct instruction *insn)
> "__ubsan_handle_builtin_unreachable"));
> }
>
> -static bool ignore_unreachable_insn(struct instruction *insn)
> +static bool ignore_unreachable_insn(struct objtool_file *file, struct instruction *insn)
> {
> int i;
> + struct instruction *prev_insn;
>
> if (insn->ignore || insn->type == INSN_NOP)
> return true;
> @@ -2639,8 +2640,11 @@ static bool ignore_unreachable_insn(struct instruction *insn)
> * __builtin_unreachable(). The BUG() macro has an unreachable() after
> * the UD2, which causes GCC's undefined trap logic to emit another UD2
> * (or occasionally a JMP to UD2).
> + * CONFIG_UBSAN_TRAP may also insert a UD2 after calling a __noreturn
> + * function.
> */
> - if (list_prev_entry(insn, list)->dead_end &&
> + prev_insn = list_prev_entry(insn, list);
> + if ((prev_insn->dead_end || dead_end_function(file, prev_insn->call_dest)) &&
> (insn->type == INSN_BUG ||
> (insn->type == INSN_JUMP_UNCONDITIONAL &&
> insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
> @@ -2767,7 +2771,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
> return 0;
>
> for_each_insn(file, insn) {
> - if (insn->visited || ignore_unreachable_insn(insn))
> + if (insn->visited || ignore_unreachable_insn(file, insn))
> continue;
>
> WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200919064118.1899325-1-ilie.halip%40gmail.com.

2020-09-21 15:21:32

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH v2] objtool: ignore unreachable trap after call to noreturn functions

On Sat, Sep 19, 2020 at 09:41:18AM +0300, Ilie Halip wrote:
> With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap instruction
> after a call to a noreturn function. In this case, objtool warns that the
> ud2 instruction is unreachable.
>
> This is a behavior seen with clang, from the oldest version capable of
> building the mainline x64_64 kernel (9.0), to the latest experimental
> version (12.0).
>
> objtool silences similar warnings (trap after dead end instructions), so
> so expand that check to include dead end functions.
>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Rong Chen <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Philip Li <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
> Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
> Suggested-by: Nick Desaulniers <[email protected]>
> Reviewed-by: Nick Desaulniers <[email protected]>
> Tested-by: Nick Desaulniers <[email protected]>
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Ilie Halip <[email protected]>
> ---
>
> Changed in v2:
> - added a mention that this is a clang issue across all versions
> - added Nick's Reviewed-by, Tested-by
> - added Reported-by

Thanks. Queued.

--
Josh