Faulty logic in `kpatch_replace_sections_syms()`

[j-piecuch]

The function uses a local bool found variable to keep track of whether a replacement symbol was found for a given rela. The variable is set to true when a replacement symbol is found for a particular rela, but when we move to the next rela, it should be reset to false, which never happens.

Simply adding the missing found = false; statement causes several unit tests to fail:

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                                                                                                                                  
BUILD ASSERT_RTNL-detect                                                                                                                           
create-diff-object: ERROR: ASSERT_RTNL-detect.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x308: can't find replacement symbol for .init.text+748 reference                                                                                                                            
BUILD clang-lstr
create-diff-object: ERROR: clang-lstr.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x14: can't find replacement symbol for .init.text+143 reference
BUILD clang-special-static
create-diff-object: ERROR: clang-special-static.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x19c: can't find replacement symbol for .init.text+302 reference
BUILD cmdline-ubsan
create-diff-object: ERROR: cmdline-ubsan.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x14: can't find replacement symbol for .init.text+94 reference
BUILD cold-noid-issue-1160
create-diff-object: ERROR: cold-noid-issue-1160.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x86c: can't find replacement symbol for .init.text+17 reference
BUILD dynamic-debug-special-static-issue-990
create-diff-object: ERROR: dynamic-debug-special-static-issue-990.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x84: can't find replacement symbol for .init.text+424 reference
BUILD mangled-symbol-modification-issue-1048
create-diff-object: ERROR: mangled-symbol-modification-issue-1048.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x14: can't find replacement symbol for .init.text+123 reference
BUILD mcount_loc-issue-1102
create-diff-object: ERROR: mcount_loc-issue-1102.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x1c: can't find replacement symbol for .init.text+121 reference
BUILD multifile-pr-1204
create-diff-object: ERROR: multifile-pr-1204.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x58: can't find replacement symbol for .init.text+78 reference
BUILD plt32-relocations-issue-975
create-diff-object: ERROR: plt32-relocations-issue-975.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x4: can't find replacement symbol for .init.text+34 reference
BUILD static-call-pr-1315
create-diff-object: ERROR: static-call-pr-1315.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0x4: can't find replacement symbol for .static_call.text+5 reference
BUILD static-local-moved-subfunction-issue-1054
create-diff-object: ERROR: static-local-moved-subfunction-issue-1054.ORIG.o: kpatch_replace_sections_syms: 1685: .orc_unwind_ip+0xc8: can't find replacement symbol for .init.text+711 reference

[joe-lawrence]

@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?

[puranjaymohan]

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.

[jpoimboe]

Hm, ORC does create a "section terminator" which references the end of the section. I think those can just be skipped.

[joe-lawrence]

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.

[github-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.

[github-actions]

This issue was closed because it was inactive for 7 days after being marked stale.