Faulty logic in kpatch_replace_sections_syms() #1438

Activity

1. joe-lawrence commented on Jan 30, 2025

   

   joe-lawrence

   on Jan 30, 2025

   Contributor

   More actions

   @jpoimboe : what do you think about this? The code doesn't clear/reset the found flag, so this does look like bug.

   When clearing the flag and adding some more debugging, I found two cases that @j-piecuch reported above:

   The first, like:

   BUILD altinstr_aux                                                                                                                                 
   create-diff-object: ERROR: altinstr_aux.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x5c: can't find replacement symbol for .init.text+3172 reference
   

   where init.text is only 3172 bytes long, and so the rela addend is past the end of the init.text section.

   $ objdump -Dr -j .init.text test/unit/objs/x86_64/altinstr_aux.ORIG.o | tail -n3
    c5f:   41 5e                   pop    %r14
    c61:   41 5f                   pop    %r15
    c63:   c3                      ret
   

   and the second:

   BUILD static-call-pr-1315
   create-diff-object: ERROR: static-call-pr-1315.ORIG.o: kpatch_replace_sections_syms: 1693: .orc_unwind_ip+0x4: can't find replacement symbol for .static_call.text+5 reference
   

   which I'm not sure if we need to do this for the static call section anyway?
2. puranjaymohan commented on Jan 31, 2025

   

   puranjaymohan

   on Jan 31, 2025

   Contributor

   More actions

   I looked at this too with my limited linkers and loaders knowledge and I fail to understand it. For example:
   This error - .orc_unwind_ip+0x4: can't find replacement symbol for .static_call.text+0x5 reference

   referring to the following:

   Disassembly of section .static_call.text:
   
   0000000000000000 <__SCT__tp_func_kvm_mmu_pagetable_walk>:
      0:   e9 00 00 00 00          jmp    5 <__SCT__tp_func_kvm_mmu_pagetable_walk+0x5>
      5:   0f 1f 00                nopl   (%rax)
   

   Here .static_call.text+0x5 can be replaced with __SCT__tp_func_kvm_mmu_pagetable_walk+0x5 but when you look at the symbol table for __SCT__tp_func_kvm_mmu_pagetable_walk you see:

      868: 0000000000000000     5 FUNC    GLOBAL DEFAULT    2 __SCT__tp_func_kvm_mmu_pagetable_walk
   

   So, the size in the symbol table is 5 therefore kpatch_replace_sections_syms() skips it in the loop:

   } else if (target_off < start || target_off >= end)
   

   Here end is 5 (as verified from symbol table) and target_off is also 5 from reloc, so this symbol is skipped.

   Now I fail to understand why the symbol table says the size of this function is 5 when is it has 3 more bytes (0f 1f 00) making the size 8.
3. jpoimboe commented on Feb 5, 2025

   

   jpoimboe

   on Feb 5, 2025

   Member

   More actions

   Hm, ORC does create a "section terminator" which references the end of the section. I think those can just be skipped.
4. joe-lawrence commented on Feb 18, 2025

   

   joe-lawrence

   on Feb 18, 2025

   Contributor

   More actions

   FWIW, it seems that now an ud1 opcode is generated instead of nopl and the size of the function as reported in the symbol table is now the correct 8-bytes:

   $ git describe HEAD
   v6.14-rc3-12-g2408a807bfc3
   
   $ make defconfig && ./scripts/config --module CONFIG_KVM --module CONFIG_KVM_INTEL && make olddefconfig
   [ ... snip ... ]
   
   $ make -j$(nproc) arch/x86/kvm/mmu/mmu.o
   [ ... snip ... ]
   
   $ readelf --wide --symbols arch/x86/kvm/mmu/mmu.o | grep -w '__SCT__tp_func_kvm_mmu_pagetable_walk'
      469: 0000000000000000     8 FUNC    GLOBAL DEFAULT    8 __SCT__tp_func_kvm_mmu_pagetable_walk
   
   $ objdump -Dr -j.static_call.text arch/x86/kvm/mmu/mmu.o | head -n10
   
   arch/x86/kvm/mmu/mmu.o:     file format elf64-x86-64
   
   
   Disassembly of section .static_call.text:
   
   0000000000000000 <__SCT__tp_func_kvm_mmu_pagetable_walk>:
      0:   e9 00 00 00 00          jmp    5 <__SCT__tp_func_kvm_mmu_pagetable_walk+0x5>
                           1: R_X86_64_PC32        __traceiter_kvm_mmu_pagetable_walk-0x4
      5:   0f b9 cc                ud1    %esp,%ecx
   

   v5.19 commit torvalds/linux@ee88d36 ("x86,static_call: Use alternative RET encoding") explains the ud1, but even at ee88d363~1, I still see a symbol table size of 8 bytes.

   Going back to v5.16 commit torvalds/linux@2105a92 ("static_call,x86: Robustify trampoline patching"), introduced a "SCT" byte sequence and it seems like here is where the symbol table size gets represented correctly whenever there are 3 or more bytes added to __ARCH_DEFINE_STATIC_CALL_TRAMP. (If I substitute a smaller 1 or 2 byte sequence, the size seems to add up but the objdump includes further garbage? instructions. Things are consistent however, if I substitute in a longer 4, 7, etc. byte sequence.)

   So I don't really know what conclusions to reach from the code archeology to this point, other than that the unit test object files were probably generated on pre-v5.16 kernels where I can't explain how the symbol table calculates the function size, or why objdump spits out extra instructions.
5. github-actions commented on Mar 21, 2025

   

   github-actionsbot

   on Mar 21, 2025 – with GitHub Actions

   More actions

   This issue has been open for 30 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.
6. github-actions

   added

   stale

   on Mar 21, 2025
7. github-actions commented on Mar 28, 2025

   

   github-actionsbot

   on Mar 28, 2025 – with GitHub Actions

   More actions

   This issue was closed because it was inactive for 7 days after being marked stale.
8. github-actions

   closed this as completedon Mar 28, 2025
9. joe-lawrence

   self-assigned this

   on Mar 31, 2025
10. joe-lawrence

    removed

    stale

    on Mar 31, 2025
11. joe-lawrence

    reopened this on Mar 31, 2025