Some ppc64le cleanups and fixes by jpoimboe · Pull Request #757 · dynup/kpatch

jpoimboe · 2017-11-08T20:31:30Z

This is a followup to #754. This gets rid of the ppc64le-specific changes in create-klp-module, and fixes a regression which was introduced in #737.

This still needs a lot of testing on both x86 and ppc64le. I recommend meminfo-string.patch, as well as a trivial patch to load_elf_binary() in fs/binfmt_elf.c, so that #700 gets tested properly. It also needs integration testing on x86.

kamalesh-babulal · 2017-11-09T14:45:44Z

@jpoimboe Thanks for working on it. I gave it a spin on my vm with testcases/test/integration/fedora-25/{gcc-static-local-var-2.patch,data-read-mostly.patch,meminfo-string.patch} livepatch modules stacked and the previously seen stalls are not seen, few kernbench iterations were run to stress the vm. I did not get a chance to review the patchset yet.

jpoimboe · 2017-11-10T01:15:19Z

@kamalesh-babulal Thanks for testing!

I just coded up a GCC plugin which converts local calls to global (basically inserting nops after the local calls), based on our upstream mailing list discussions.

I did some basic testing with it, but I haven't had a chance to test it with kpatch yet because I don't have access to any POWER HW at the moment. If you want to try it out, I've added it as a commit to this pull request.

Otherwise I should hopefully be able to get ahold of some HW soon.

is_localentry_sym() isn't quite the right name, because it also checks for the 8-byte gap introduced by GCC 6, and also checks that the function is otherwise at the beginning of the section. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

The STT_FUNC and SHN_UNDEF checks aren't needed because they're already implied by the localentry check. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

On ppc64le, adding a printk to total_mapping_size() caused it to change from non-localentry to localentry, presumably because it was no longer a leaf function. With GCC 6, a localentry function is offset by 8 in the section, so different st_values are ok. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

…ng assumption kpatch_replace_sections_syms() assumes that all bundled symbols start at section offset zero. With ppc64le and GCC 6+, that assumption is no longer accurate. When replacing a rela symbol section with its corresponding symbol, adjust the addend as necessary. Also, with this fix in place, the workaround in create_klp_relasecs_and_syms() can be removed. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

kpatch_relocation's 'dest' addend and 'offset' fields are redundant. In fact, the 'offset' field isn't always accurate because it doesn't have a relocation, so its value doesn't adjust when multiple .o files are combined. Just use the 'dest' addend instead. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

When creating .kpatch.relocations, there's no reason to convert the relocation destinations to symbols. In fact, it's actively harmful because it makes it harder for create-klp-module to deal with the GCC 6+ 8-byte localentry gap. This also fixes a regression which was introduced in 5888f31, which broke ppc64le relocations. Fixes dynup#754. Fixes: 5888f31 ("create-klp-module: support unbundled symbols") Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

kamalesh-babulal · 2017-11-10T09:28:33Z

@jpoimboe Thank you for the GCC plugin. I tried running on the latest kernel and the build fails
with exactly same error for different .o files:

  gcc -m64 -Wp,-MD,arch/powerpc/net/.bpf_jit_comp64.o.d  -nostdinc -isystem /usr/lib/gcc/powerpc64le-linux-gnu/6/include -I./arch/powerpc/include -I./arch/powerpc/include/generated  -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/kconfig.h -D__KERNEL__ -DCC_USING_MPROFILE_KERNEL -Iarch/powerpc -DHAVE_AS_ATHIGH=1 -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -Werror-implicit-function-declaration -Wno-format-security -std=gnu89 -fno-PIE -msoft-float -pipe -Iarch/powerpc -mtraceback=no -mabi=elfv2 -mcmodel=medium -mno-pointers-to-nested-functions -mcpu=power7 -mno-altivec -mno-vsx -mno-spe -mspe=no -funit-at-a-time -fno-dwarf2-cfi-asm -mno-string -mno-sched-epilog -Wa,-maltivec -mlittle-endian -mno-strict-align -fno-delete-null-pointer-checks -Wno-frame-address -O2 --param=allow-store-data-races=0 -DCC_HAVE_ASM_GOTO -Wframe-larger-than=2048 -fno-stack-protector -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-var-tracking-assignments -g -gdwarf-4 -pg -mprofile-kernel -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -Werror=implicit-int -Werror=strict-prototypes -Werror=date-time -Werror=incompatible-pointer-types -Werror=designated-init -I/root/josh/kpatch/kmod/patch -ffunction-sections -fdata-sections -mcmodel=large -fplugin=/root/josh/kpatch/kpatch-build/gcc-plugins/ppc64le-plugin.so    -DKBUILD_BASENAME='"bpf_jit_comp64"'  -DKBUILD_MODNAME='"bpf_jit_comp64"' -c -o arch/powerpc/net/bpf_jit_comp64.o arch/powerpc/net/bpf_jit_comp64.c
{standard input}: Assembler messages:
{standard input}:4849: Error: symbol `.L717' is already defined
scripts/Makefile.build:314: recipe for target 'arch/powerpc/net/bpf_jit_comp64.o' failed
make[1]: *** [arch/powerpc/net/bpf_jit_comp64.o] Error 1
Makefile:1024: recipe for target 'arch/powerpc/net' failed

Looks like the label is generated twice .L717

        blt 7,.L717      #
.L242:
        .loc 1 483 0
        andi. 7,3,0xffff         #, tmp2907, imm
.LVL141:
        bne 0,.L249      #
--
        bgt 7,.L717      #
        rlwinm 9,9,0,16,29       # tmp4716, tmp4359,,
        sldi 8,8,2       #, tmp4714, _2361
        oris 9,9,0x4181  #,, tmp4717, tmp4716
        stwx 9,14,8      # *_2762, tmp4717
        lwz 9,4(30)      # ctx_254(D)->idx,
--
.L717:
.LBB413:
.LBB363:
        .loc 1 253 0
        lis 9,0x4081     # tmp4367,
        sldi 8,8,2       #, tmp4365, _2361
--
.L717:
        .loc 1 478 0 discriminator 1
        cmplwi 7,9,7     #, tmp2866, prephitmp_3436
        bne 7,.L242      #
        .loc 1 480 0
        rldicl 9,3,0,32  # imm.77_1033, imm

no-op are inserted after the branch (local functions)

0000000000000008 <meminfo_proc_show>:
   8:   f8 ff 4c e8     ld      r2,-8(r12)
                        8: R_PPC64_ENTRY        *ABS*
   c:   14 62 42 7c     add     r2,r2,r12
  10:   a6 02 08 7c     mflr    r0
  14:   01 00 00 48     bl      14 <meminfo_proc_show+0xc>
                        14: R_PPC64_REL24       _mcount
  18:   a6 02 08 7c     mflr    r0
...
  e8:   01 00 00 48     bl      e8 <meminfo_proc_show+0xe0>
                        e8: R_PPC64_REL24       show_val_kb
  ec:   00 00 00 60     nop
  f0:   00 00 22 3d     addis   r9,r2,0
                        f0: R_PPC64_TOC16_HA    .toc+0x40
  f4:   88 00 a1 e8     ld      r5,136(r1)
  f8:   78 fb e3 7f     mr      r3,r31
  fc:   00 00 89 e8     ld      r4,0(r9)
                        fc: R_PPC64_TOC16_LO_DS .toc+0x40
 100:   01 00 00 48     bl      100 <meminfo_proc_show+0xf8>
                        100: R_PPC64_REL24      show_val_kb
 104:   00 00 00 60     nop
 108:   00 00 42 3d     addis   r10,r2,0
                        108: R_PPC64_TOC16_HA   .toc+0x48
 10c:   78 db 65 7f     mr      r5,r27
 110:   00 00 8a e8     ld      r4,0(r10)
                        110: R_PPC64_TOC16_LO_DS        .toc+0x48
 114:   78 fb e3 7f     mr      r3,r31
 118:   01 00 00 48     bl      118 <meminfo_proc_show+0x110>
                        118: R_PPC64_REL24      show_val_kb
 11c:   00 00 00 60     nop

jpoimboe · 2017-11-10T14:02:14Z

Oops, I found where it was also changing the label. It should be fixed now. I've got a POWER8 box today, should be able to give it some more testing.

kamalesh-babulal · 2017-11-10T14:52:54Z

The livepatch module for meminfo-string.patch gets created and livepatched successfully with the kernel patch:

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f8968..39b01fdb 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -613,7 +613,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
                case R_PPC_REL24:
                        /* FIXME: Handle weak symbols here --RR */
-                       if (sym->st_shndx == SHN_UNDEF) {
+                       if (sym->st_shndx == SHN_UNDEF ||
+                           sym->st_shndx == SHN_LIVEPATCH) {
                                /* External: go via stub */
                                value = stub_for_addr(sechdrs, value, me);
                                if (!value)

jpoimboe · 2017-11-10T14:56:38Z

Awesome!

kamalesh-babulal · 2017-11-10T15:00:06Z

It fails for data-readonly.patch case:

# dmesg |tail -n1
[  958.074036] module_64: livepatch_data_read_mostly: Expect noop after relocate, got 7c630034

# readelf -s -W ./dev.o
...
  1189: 0000000000000008   888 FUNC    GLOBAL DEFAULT [<localentry>: 8]   168 dev_queue_xmit_nit
  1190: 0000000000000008   200 FUNC    GLOBAL DEFAULT [<localentry>: 8]    26 netdev_walk_all_upper_dev_rcu
  1191: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND dev_mc_init
  1192: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND nf_hooks_needed
  1193: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND sprintf
...
...

# objdump -dr ./livepatch_data_read_mostly.o
...

0000000000000008 <netdev_has_any_upper_dev>:
   8:   f8 ff 4c e8     ld      r2,-8(r12)
                        8: R_PPC64_ENTRY        *ABS*
   c:   14 62 42 7c     add     r2,r2,r12
  10:   a6 02 08 7c     mflr    r0
  14:   01 00 00 48     bl      14 <netdev_has_any_upper_dev+0xc>
...
...
  4c:   00 00 89 e8     ld      r4,0(r9)
                        4c: R_PPC64_TOC16_LO_DS .toc+0x60
  50:   78 fb e3 7f     mr      r3,r31
  54:   01 00 00 48     bl      54 <netdev_has_upper_dev+0x4c>
  58:   34 00 63 7c     cntlzw  r3,r3
  5c:   7e d9 63 54     rlwinm  r3,r3,27,5,31
...
...
                        ac: R_PPC64_TOC16_LO_DS .toc+0x60
  b0:   78 fb e3 7f     mr      r3,r31
  b4:   01 00 00 48     bl      b4 <netdev_has_upper_dev+0xac>
  b8:   34 00 63 7c     cntlzw  r3,r3
  bc:   7e d9 63 54     rlwinm  r3,r3,27,5,31
  c0:   01 00 63 68     xori    r3,r3,1

It one of the special case, where netdev_walk_all_upper_dev_rcu is an EXPORT_SYMBOL but it can be accessed via localentry for this unit.

jpoimboe · 2017-11-10T15:04:18Z

Ok, I'll take a look at it. I may need to tweak the plugin, it might not be finding all the branches yet.

…l calls This is in response to an upstream discussion for the following patch: https://lkml.kernel.org/r/1508217523-18885-1-git-send-email-kamalesh@linux.vnet.ibm.com This should hopefully make it a lot easier for the ppc64le kernel module code to support klp relocations. The gcc-common.h and gcc-generate-rtl-pass.h header files are copied from the upstream Linux source tree. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

jpoimboe · 2017-11-10T15:17:39Z

Found the bug already :-) (Untested) fix pushed.

kamalesh-babulal · 2017-11-10T15:41:02Z

data-readonly.patch module generation issue is also fixed :). I will test more over the weekend. Thanks again.

jpoimboe · 2017-11-10T15:52:45Z

Weird, I'm still getting "module_64: livepatch_data_read_mostly: Expect noop after relocate, got 3c820000" with data-readonly.patch. Will keep looking...

jpoimboe · 2017-11-10T17:10:35Z

My latest error is due to the following code:

 14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                        14c: R_PPC64_REL24      net_set_todo
 150:   00 00 82 3c     addis   r4,r2,0

It's a sibling call to net_set_todo(). Notice it's a b instruction instead of a bl instruction. So the "call" never returns and it doesn't need r2 to be restored afterwards.

Attempting to fix it by changing the restore_r2() check to ignore this situation.

jpoimboe · 2017-11-10T18:10:16Z

Got it to work with the following kernel patch:

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b99f9f..6015e2f0da9c 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -490,9 +490,13 @@ static int restore_r2(u32 *instruction, struct module *me)
        if (is_early_mcount_callsite(instruction - 1))
                return 1;
 
+       /* Sibling calls don't return, so they don't need to restore r2 */
+       if (instruction[-1] == PPC_INST_BRANCH)
+               return 1;
+
        if (*instruction != PPC_INST_NOP) {
-               pr_err("%s: Expect noop after relocate, got %08x\n",
-                      me->name, *instruction);
+               pr_err("%s: Expect noop after relocate, got %08x at %pS\n",
+                      me->name, *instruction, instruction);
                return 0;
        }
        /* ld r2,R2_STACK_OFFSET(r1) */
@@ -614,7 +618,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
                case R_PPC_REL24:
                        /* FIXME: Handle weak symbols here --RR */
-                       if (sym->st_shndx == SHN_UNDEF) {
+                       if (sym->st_shndx == SHN_UNDEF ||
+                           sym->st_shndx == SHN_LIVEPATCH) {
                                /* External: go via stub */
                                value = stub_for_addr(sechdrs, value, me);
                                if (!value)

jpoimboe · 2017-11-10T18:38:10Z

Also successfully patched load_elf_binary(), so #700 seems to be fixed as well.

I'll send you my two kernel patches in case you want to send them together with the SHN_LIVEPATCH change after you do some more testing.

kamalesh-babulal · 2017-11-11T06:54:07Z

Weird, I'm still getting "module_64: livepatch_data_read_mostly: Expect noop after relocate, got 3c820000" with data-readonly.patch. Will keep looking...
My latest error is due to the following code:

 14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                        14c: R_PPC64_REL24      net_set_todo
 150:   00 00 82 3c     addis   r4,r2,0

I am able to reproduce the issue with gcc-5 not with gcc-6. Looking at the objdump on code generated with gcc-6, the above branch is missing. After adding the check for sibling call to the kernel. The issue is resolved for gcc-5 compiled module. I run more tests from kpatch/test/integration/fedora-25/ bucket for both gcc-5, gcc-6.

kamalesh-babulal · 2017-11-11T06:55:51Z

Also successfully patched load_elf_binary(), so #700 seems to be fixed as well.

Yes, this fixes #700 for ppc64le too.

I'll send you my two kernel patches in case you want to send them together with the SHN_LIVEPATCH change after you do some more testing.

Thanks, will send it after the testing to LKML.

kamalesh-babulal · 2017-11-13T16:26:48Z

I was able to build and livepatch around 19 modules based on testcases from kpatch/test/integration/fedora-25/:

livepatch_bug_table_section     8680  1
livepatch_cmdline_string     6118  1
livepatch_fixup_section     7490  1
livepatch_tracepoints_section    10114  1
livepatch_module_call_external     9470  1
livepatch_macro_hooks    12598  1
livepatch_gcc_static_local_var_5    19807  1
livepatch_gcc_static_local_var_4    13146  1
livepatch_gcc_static_local_var_3     9048  1
livepatch_special_static    29494  1
livepatch_new_globals    12594  1
livepatch_new_function     9541  1
livepatch_gcc_static_local_var_2    13434  1
livepatch_gcc_mangled_3    15473  1
livepatch_gcc_isra      8535  1
livepatch_gcc_constprop     8462  1
livepatch_data_read_mostly    48315  1
livepatch_data_new     11806  1
livepatch_convert_global_local    16972  1

I have two VM's with these modules loaded one with gcc-5 and another with gcc-6. Planning to leave them idle over night and run some workload after a long idle period to be sure. I will push the patches to LKML by morning, after running some smoke test type workload.

jpoimboe mentioned this pull request Nov 8, 2017

ppc64le: Fix wrong offset for destination symbols in create_klp_relasecs_and_syms() #754

Closed

jpoimboe added 6 commits November 9, 2017 19:16

create-diff-object: simplify is_gcc6_localentry_bundled_sym()

e598d96

The STT_FUNC and SHN_UNDEF checks aren't needed because they're already implied by the localentry check. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

jpoimboe force-pushed the TODO-ppc-fix branch 2 times, most recently from fd3c00f to f1df17b Compare November 10, 2017 03:25

jpoimboe force-pushed the TODO-ppc-fix branch from f1df17b to 04207bd Compare November 10, 2017 13:59

jpoimboe force-pushed the TODO-ppc-fix branch from 04207bd to 63a94b8 Compare November 10, 2017 15:17

kamalesh-babulal mentioned this pull request Dec 4, 2017

More ppc64le fixes #773

Merged

jpoimboe merged commit 258ac3f into dynup:master Dec 21, 2017

Conversation

jpoimboe commented Nov 8, 2017

Uh oh!

kamalesh-babulal commented Nov 9, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

kamalesh-babulal commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

kamalesh-babulal commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

kamalesh-babulal commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

kamalesh-babulal commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

jpoimboe commented Nov 10, 2017

Uh oh!

kamalesh-babulal commented Nov 11, 2017

Uh oh!

kamalesh-babulal commented Nov 11, 2017

Uh oh!

kamalesh-babulal commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kamalesh-babulal commented Nov 13, 2017 •

edited

Loading