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

["Chang S. Bae" <chang.seok.bae@intel.com>]

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