lkml.org 
[lkml]   [2022]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
SubjectRe: [PATCH v2 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle
From
On 3/10/2022 1:00 PM, Chang S. Bae wrote:
>
> BTW, now I'm suspicious of this JMP as patched at runtime with
> fpu_state_size_dynamic():

No, this jump was supposed to be replaced with NOP by objtool but it
didn't as fail to interpret TILERELEASE in this case.

>
>   22:   eb 01                    jmp    0x25
>   24:   c3                       retq
>   25:   b9 01 00 00 00           mov    $0x1,%ecx
>   2a:*  0f 01 d0                 xgetbv           <-- trapping instruction
>
> Still, the question is, if so, why it was patched on non-XFD systems.
> Let me analyze the case a bit further with 0day folks.
>

Looks like 0day picked an internal branch where the instruction's opcode
was intentionally removed.

In practice, upstream code should accompany by a complete opcode table.

If it ever happens, a warning follows like this on build:
arch/x86/kernel/fpu/core.o: warning: objtool: can't decode
instruction at .text:0x185e

But what actually happened is barely indicated by this message alone.
This decode failure ends up returning check() immediately [1] so the
file is entirely skipped from the tool's process.

I came to think of some improvements for this tool:

(1) Add more messages like [2]. This may help users understand what
happens in this build process.

(2) Move on next byte from the failed offset like [3]. Perhaps, this
continuation may alleviate the impact. It may misinterpret some bytes
but I think it will be re-aligned with padding bytes before the next
function (symbol).

Include Josh Poimboeuf. Appreciate any feedback.

Thanks,
Chang

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/objtool/check.c#n3515

[2]:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7c33ec67c4a9..34b60fa33fbe 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3507,27 +3507,27 @@ int check(struct objtool_file *file)
set_func_state(&func_cfi);

if (!cfi_hash_alloc(1UL << (file->elf->symbol_bits - 3)))
- goto out;
+ goto err;

cfi_hash_add(&init_cfi);
cfi_hash_add(&func_cfi);

ret = decode_sections(file);
if (ret < 0)
- goto out;
+ goto err;

warnings += ret;

if (list_empty(&file->insn_list))
- goto out;
+ return 0;

if (vmlinux && !validate_dup) {
ret = validate_vmlinux_functions(file);
if (ret < 0)
- goto out;
+ goto err;

warnings += ret;
- goto out;
+ return 0;
}

if (retpoline) {
@@ -3539,37 +3539,37 @@ int check(struct objtool_file *file)

ret = validate_functions(file);
if (ret < 0)
- goto out;
+ goto err;
warnings += ret;

ret = validate_unwind_hints(file, NULL);
if (ret < 0)
- goto out;
+ goto err;
warnings += ret;

if (!warnings) {
ret = validate_reachable_instructions(file);
if (ret < 0)
- goto out;
+ goto err;
warnings += ret;
}

ret = create_static_call_sections(file);
if (ret < 0)
- goto out;
+ goto err;
warnings += ret;

if (retpoline) {
ret = create_retpoline_sites_sections(file);
if (ret < 0)
- goto out;
+ goto err;
warnings += ret;
}

if (mcount) {
ret = create_mcount_loc_sections(file);
if (ret < 0)
- goto out;
+ goto err;
warnings += ret;
}

@@ -3580,11 +3580,14 @@ int check(struct objtool_file *file)
printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
}

-out:
+ return 0;
+
+err:
/*
* 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.
*/
+ WARN("check failed - no jump_label instructions were written.");
return 0;
}
[3]:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7c33ec67c4a9..1f1515373ca5 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -371,7 +371,7 @@ static int decode_instructions(struct objtool_file
*file)
!strcmp(sec->name, ".entry.text"))
sec->noinstr = true;

- for (offset = 0; offset < sec->sh.sh_size; offset +=
insn->len) {
+ for (offset = 0; offset < sec->sh.sh_size;) {
insn = malloc(sizeof(*insn));
if (!insn) {
WARN("malloc failed");
@@ -389,12 +389,15 @@ static int decode_instructions(struct objtool_file
*file)
&insn->len,
&insn->type,
&insn->immediate,
&insn->stack_ops);
- if (ret)
- goto err;
+ if (ret) {
+ offset++;
+ continue;
+ }

hash_add(file->insn_hash, &insn->hash,
sec_offset_hash(sec, insn->offset));
list_add_tail(&insn->list, &file->insn_list);
nr_insns++;
+ offset += insn->len;
}

list_for_each_entry(func, &sec->symbol_list, list) {
@@ -416,10 +419,6 @@ static int decode_instructions(struct objtool_file
*file)
printf("nr_insns: %lu\n", nr_insns);

return 0;
-
-err:
- free(insn);
- return ret;
}

\
 
 \ /
  Last update: 2022-03-22 08:08    [W:2.706 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site