Re: [PATCH v2 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle

[Chang S. Bae]

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;
 }