.altinstructions section header details differ from .altinstructions

[joe-lawrence]

Yulia encountered problems building a modified module.patch [1] when running the integration tests for an early rhel-9.0 kernel. I added a bit of debugging to kpatch-build/create-diff-object.c, source and readelf:

ERROR: export.o: sec1->sh.sh_flags(0x2) vs sec2->sh.sh_flags(0x3)

    #define SHF_WRITE            (1 << 0)   /* Writable */
    #define SHF_ALLOC            (1 << 1)   /* Occupies memory during execution */

ERROR: export.o: sec1->sh.sh_entsize(0x0) vs sec2->sh.sh_entsize(0xc)
ERROR: export.o: .altinstructions section header details differ from .altinstructions


Section Headers:
[Nr]   Name              Type      Address           Off     Size    ES  Flg  Lk  Inf  Al

~/.kpatch/tmp/orig/fs/nfsd/export.o
[152]  .altinstructions  PROGBITS  0000000000000000  0bff01  000060  0c  WA   0   0    1

~/.kpatch/tmp/patched/fs/nfsd/export.o
[87]   .altinstructions  PROGBITS  0000000000000000  0040b8  000078  00  A    0   0    1

The only interesting change for module.patch is in joe-lawrence@1c85c2e where I substituted a different static key.

[1] https://github.com/joe-lawrence/kpatch/tree/integration-tests-kernel-5.13.0-0.rc4.33.el9

export-objs.tar.gz

[jpoimboe]

I think this should fix it?

index 523aa4157f80..bc821056aba9 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -684,7 +684,7 @@ static int elf_add_alternative(struct elf *elf,
        sec = find_section_by_name(elf, ".altinstructions");
        if (!sec) {
                sec = elf_create_section(elf, ".altinstructions",
-                                        SHF_WRITE, size, 0);
+                                        SHF_ALLOC, size, 0);
 
                if (!sec) {
                        WARN_ELF("elf_create_section");

I'll try to submit a fix upstream tomorrow.

[joe-lawrence]

Sorry I ran out of time to fully test this today (and heading out the door for PTO). I got as far as rebuilding the distro kernel and now notice that its export.o :: .altinstructions section does not have the (W)rite flag set. I can try a full test in a week or so when I get back.

[jpoimboe]

No worries, it's clearly a bug. I'll go ahead and submit the fix when I get the chance.

[joe-lawrence]

Hi Josh,

Sorry for the late follow up on this, on 5.14.0-0.rc4.35.el9.x86_64 with the above mentioned objtool patch, we're closer, but still trip over the change in section entry size (0xc vs 0x0):

ERROR: export.o: .altinstructions section header details differ from .altinstructions

Section Headers:
[Nr]   Name              Type      Address           Off     Size    ES Flg Lk Inf Al

orig/fs/nfsd/export.o:
[152]  .altinstructions  PROGBITS  0000000000000000  0c1741  000060  0c A   0  0   1

patched/fs/nfsd/export.o:
[87]   .altinstructions  PROGBITS  0000000000000000  0040b8  000078  00 A   0  0   1

For this, I posted a possible objtool fix: https://lore.kernel.org/lkml/20210817014958.1108400-1-joe.lawrence@redhat.com/T/#u

However with that in place, we're left with a kpatch build failure:

...
  MODPOST modules-only.symvers
WARNING: modpost: "kpatch_string" [fs/nfsd/nfsd.ko] undefined!
WARNING: modpost: "__SCK__pv_sched_clock" [fs/nfsd/nfsd.ko] undefined!
WARNING: modpost: "__SCT__pv_sched_clock" [fs/nfsd/nfsd.ko] undefined!
  GEN     Module.symvers
  CC [M]  fs/nfsd/nfsd.mod.o
  LD [M]  fs/nfsd/nfsd.ko
af_netlink.o: new function: kpatch_string
export.o: WARNING: unable to correlate static local variable kpatch_key.4 used by __jump_table, assuming variable is new
export.o: changed function: e_show
make -C /root/.kpatch/src M=/root/.kpatch/tmp/patch CFLAGS_MODULE=''
make[1]: Entering directory '/root/.kpatch/src'
  LDS     /root/.kpatch/tmp/patch/kpatch.lds
  CC [M]  /root/.kpatch/tmp/patch/patch-hook.o
  LD [M]  /root/.kpatch/tmp/patch/livepatch-module.o
  MODPOST /root/.kpatch/tmp/patch/Module.symvers
ERROR: modpost: "__SCK__pv_sched_clock" [/root/.kpatch/tmp/patch/livepatch-module.ko] undefined!
ERROR: modpost: "__SCT__pv_sched_clock" [/root/.kpatch/tmp/patch/livepatch-module.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:150: /root/.kpatch/tmp/patch/Module.symvers] Error 1
make[2]: *** Deleting file '/root/.kpatch/tmp/patch/Module.symvers'
make[1]: *** [Makefile:1767: modules] Error 2
make[1]: Leaving directory '/root/.kpatch/src'
make: *** [Makefile:22: livepatch-module.ko] Error 2

I can see relocations for those symbols, but if they aren't exported by the kernel, shouldn't they have been converted into klp-relocations??

$ readelf --wide --relocs /root/.kpatch/tmp/patch/livepatch-module.o | awk '/__SCT__pv_sched_clock/ || /__SCK__pv_sched_clock/' RS="\n\n" ORS="\n\n"
Relocation section '.rela.text.e_show' at offset 0xf8a48 contains 27 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000001  0000006b00000004 R_X86_64_PLT32         0000000000000000 __fentry__ - 4
0000000000000011  000000280000000b R_X86_64_32S           0000000000000000 .rodata.svc_export_show.str1.1 + 0
000000000000002b  0000007b00000004 R_X86_64_PLT32         0000000000000000 strcmp - 4
0000000000000037  0000006100000004 R_X86_64_PLT32         0000000000000000 yield - 4
000000000000003c  0000007100000004 R_X86_64_PLT32         0000000000000000 __SCT__pv_sched_clock - 4
0000000000000043  0000007c00000002 R_X86_64_PC32          0000000000000000 jiffies - 4
000000000000004c  0000005b00000002 R_X86_64_PC32          0000000000000000 e_show.cold + d
0000000000000053  0000005b00000000 R_X86_64_NONE          0000000000000000 e_show.cold - 4
000000000000005f  0000005d0000000b R_X86_64_32S           0000000000000000 kpatch_key.4 + 0
0000000000000064  0000006e00000004 R_X86_64_PLT32         0000000000000000 static_key_disable - 4
000000000000006f  0000005d0000000b R_X86_64_32S           0000000000000000 kpatch_key.4 + 0
0000000000000074  0000008300000004 R_X86_64_PLT32         0000000000000000 static_key_enable - 4
00000000000000c5  0000002c0000000b R_X86_64_32S           0000000000000000 .rodata.e_show.str1.1 + 1b
00000000000000cc  0000005c0000000b R_X86_64_32S           0000000000000000 __UNIQUE_ID_ddebug955.7 + 0
00000000000000d1  0000007f00000004 R_X86_64_PLT32         0000000000000000 __dynamic_pr_debug - 4
0000000000000145  0000007200000004 R_X86_64_PLT32         0000000000000000 refcount_warn_saturate - 4
0000000000000151  0000002c0000000b R_X86_64_32S           0000000000000000 .rodata.e_show.str1.1 + 45
0000000000000159  0000008100000004 R_X86_64_PLT32         0000000000000000 seq_puts - 4
0000000000000164  0000002d0000000b R_X86_64_32S           0000000000000000 .rodata.e_show.str1.8 + 20
000000000000016c  0000008100000004 R_X86_64_PLT32         0000000000000000 seq_puts - 4
0000000000000171  0000008000000004 R_X86_64_PLT32         0000000000000000 kpatch_string - 4
000000000000017c  0000008100000004 R_X86_64_PLT32         0000000000000000 seq_puts - 4
000000000000018f  0000007300000004 R_X86_64_PLT32         0000000000000000 __x86_indirect_thunk_rdx - 4
000000000000019b  0000002c0000000b R_X86_64_32S           0000000000000000 .rodata.e_show.str1.1 + 54
00000000000001a3  0000008100000004 R_X86_64_PLT32         0000000000000000 seq_puts - 4
00000000000001b2  0000007200000004 R_X86_64_PLT32         0000000000000000 refcount_warn_saturate - 4
00000000000001c4  0000007200000004 R_X86_64_PLT32         0000000000000000 refcount_warn_saturate - 4

Relocation section '.rela.static_call_sites' at offset 0xf9840 contains 2 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000000  0000005a00000002 R_X86_64_PC32          0000000000000000 e_show + 3b
0000000000000004  0000007e00000002 R_X86_64_PC32          0000000000000000 __SCK__pv_sched_clock + 0

issue-1194.tar.gz

[jpoimboe]

Ha. I'm thinking we can probably call this working as designed. paravirt_sched_clock() was converted from a parainstruction to a non-exported static call, so it can no longer be called by a module. i.e. you'd get the same compile error trying to build the kernel normally with that patch. So I think the patch isn't realistic, and thus it's not a use case we'd need to worry about. While it's possible to use a .klp.rela here, I don't know if it would be straightforward with the current design.

To have the integration test still exercise paravirt, the call to paravirt_sched_clock() could be changed to a proper paravirt call, like __flush_tlb_local().

[joe-lawrence]

Kernel fixes have been merged upstream:

commit dc02368164bd0ec603e3f5b3dd8252744a667b8a
Author: Joe Lawrence joe.lawrence@redhat.com
Date: Sun Aug 22 18:50:36 2021 -0400

objtool: Make .altinstructions section entry size consistent

commit fe255fe6ad97685e5a4be0d871f43288dbc10ad6
Author: Joe Lawrence joe.lawrence@redhat.com
Date: Sun Aug 22 18:50:37 2021 -0400

objtool: Remove redundant 'len' field from struct section