[RFC] PC64le - do not use the patched functions as callbacks directly #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Found in the scope of dynup#755 but not related to the main problem discussed there. kpatch_create_patches_sections() and kpatch_create_intermediate_sections() used 'hint' in error messages. However, the string 'hint' refers to is owned by 'kelf_base' and is freed before kpatch_create_*_sections() are called. As a result, if these functions try to output errors and print 'hint', create-diff-object will crash. As suggested in the mentioned PR, 'hint' is actually no longer needed at that stage, so I have removed it from kpatch_create_*_sections().
kpatch-build: 'hint' is not needed in kpatch_create_*_sections()
It was observed by Evgenii Shatokhin in PR#755, that when the RCU callback was called on the patched function, from unloaded livepatch module triggered a kernel crash. This patch implements the approach on Powerpc outlined in PR#755. With -mcmodel=large, like any other data, function pointers are also loaded relative to the current TOC base and are populated as relocation entries in .toc section. Every function call/passing such function as pointers needs to access them through .toc section + offset. Simplest approach would be to convert the .toc + offset relocation into a dynamic rela referring to the original function address but the same function is also a patched function. Thus, on Powerpc we need to append a new relocation entry into .rela.toc, duplicating the entry used as a function pointer and converting the newly added rela as dynamic rela. Also, modify the function to load the new entry by modifying the addend value to newly created dynamic rela. Cc: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Evgenii Shatokhin <eshatokhin@virtuozzo.com> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Author
|
Sorry, for creating the pull request against the wrong branch. The PowerPC the pull request has been created at PR#789. |
Owner
|
That's fine. |
euspectre
pushed a commit
that referenced
this pull request
Sep 11, 2018
When a patch is composed of multiple .o files which have .parainstructions sections, loading the patch causes a panic: general protection fault: 0000 [#1] SMP Modules linked in: livepatch_4_9_88_1_20180518_1(OK+) livepatch_4_9_88_1_20180510_1(OK) ... CPU: 1 PID: 17257 Comm: insmod Tainted: G O K 4.9.0-6-amd64 #1 Debian 4.9.88-1 Hardware name: HP ProLiant MicroServer Gen8, BIOS J06 11/02/2015 task: ffff9ff3411a4480 task.stack: ffffac8f8271c000 RIP: 0010:[<ffffffff8ae2e1d0>] [<ffffffff8ae2e1d0>] apply_paravirt+0xc0/0x140 RSP: 0018:ffffac8f8271f9a0 EFLAGS: 00010216 RAX: 00010749ffffffff RBX: ffffffffc0940658 RCX: 0000000000000085 RDX: 00000000bfebfbff RSI: ffffac8f8271f9a2 RDI: 0000000000000246 RBP: ffffac8f8271f9a2 R08: 0000000000000085 R09: ffffffff8ae5acb0 R10: 0000000000000001 R11: ffff9ff3544b4400 R12: ffffffffc0940660 R13: 0000000000000000 R14: ffff9ff3544b49c0 R15: ffff9ff3b43f0800 FS: 00007f04c1cea700(0000) GS:ffff9ff3ca640000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000560cfd63e460 CR3: 00000001455c6000 CR4: 0000000000160670 Stack: 401f0ff889486973 6172007172006b00 746c00650031312e 007265746e655f69 74006e6f69007870 ffffac8f006e6f69 00ffac8f8271fa28 ffffffff8b13ae86 ffffac8f8271fa68 ffffffffc09471ec ffffffff8b7da9eb 0000000affffffff Call Trace: [<ffffffff8b13ae86>] ? vsscanf+0x4c6/0x800 [<ffffffff8b13b20e>] ? sscanf+0x4e/0x70 [<ffffffff8ae52be5>] ? arch_klp_init_object_loaded+0x105/0x130 [<ffffffff8b13b0be>] ? vsscanf+0x6fe/0x800 [<ffffffff8b13b20e>] ? sscanf+0x4e/0x70 [<ffffffff8aee29e8>] ? klp_init_object_loaded+0xf8/0x210 [<ffffffff8aee2d85>] ? klp_register_patch+0x285/0x390 [<ffffffffc09491fa>] ? patch_init+0x1fa/0x1000 [livepatch_4_9_88_1_20180518_1] [<ffffffffc0949000>] ? 0xffffffffc0949000 [<ffffffff8ae0218e>] ? do_one_initcall+0x4e/0x180 [<ffffffff8afc87dd>] ? __vunmap+0x6d/0xc0 [<ffffffff8afc87dd>] ? __vunmap+0x6d/0xc0 [<ffffffff8af7eaa1>] ? do_init_module+0x5b/0x1ed [<ffffffff8af025a6>] ? load_module+0x2596/0x2ab0 [<ffffffff8aefed50>] ? __symbol_put+0x60/0x60 [<ffffffff8af02d06>] ? SYSC_finit_module+0xc6/0xf0 [<ffffffff8ae03b7d>] ? do_syscall_64+0x8d/0xf0 [<ffffffff8b41244e>] ? entry_SYSCALL_64_after_swapgs+0x58/0xc6 Code: 8d 7c 05 00 e8 62 f7 ff ff 0f b6 53 f9 48 8b 7b f0 48 89 ee e8 f2 f8 ff ff 49 39 dc 76 57 44 0f b6 43 09 41 80 f8 ff 75 84 0f 0b <48> 8b 10 48 8d 7d 08 48 83 e7 f8 48 89 55 00 89 ca 48 8b 74 10 RIP [<ffffffff8ae2e1d0>] apply_paravirt+0xc0/0x140 RSP <ffffac8f8271f9a0> ---[ end trace 128c0fa6efe85d9e ]--- The panic is caused by a corrupt .klp.arch.vmlinux..parainstructions section: Relocation section [208] '.rela.klp.arch.vmlinux..parainstructions' for section [207] '.klp.arch.vmlinux..parainstructions' at offset 0x29dc78 contains 10 entries: Offset Type Value Addend Name 000000000000000000 X86_64_64 000000000000000000 +750 __get_user_pages 0x0000000000000010 X86_64_64 000000000000000000 +823 __get_user_pages 0x0000000000000020 X86_64_64 000000000000000000 +890 __get_user_pages 0x0000000000000030 X86_64_64 000000000000000000 +941 __get_user_pages 0x0000000000000040 X86_64_64 000000000000000000 +1631 __get_user_pages 0x0000000000000050 X86_64_64 000000000000000000 +1671 __get_user_pages 0x000000000000005c X86_64_64 000000000000000000 +1245 handle_userfault 0x000000000000006c X86_64_64 000000000000000000 +1340 handle_userfault 0x000000000000007c X86_64_64 000000000000000000 +1417 handle_userfault 0x000000000000008c X86_64_64 000000000000000000 +1717 handle_userfault The entries are supposed to be 16 bytes each, but notice they become misaligned starting with the 'handle_userfault' entry. This happens because the kernel linking process lies about the .parainstructions section size, underreporting it by four bytes. So when two .parainstructions sections are merged together, it results in a corrupted .klp.arch.vmlinux..parainstructions section. Fix it by properly aligning the section before merging it with another one. Fixes dynup#852. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
euspectre
pushed a commit
that referenced
this pull request
Jan 17, 2020
With the following patch:
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index e008aefc3a9d..7c70e369390d 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2228,6 +2228,8 @@ static void xs_tcp_shutdown(struct rpc_xprt *xprt)
struct socket *sock = transport->sock;
int skst = transport->inet ? transport->inet->sk_state : TCP_CLOSE;
+ asm("nop");
+
if (sock == NULL)
return;
switch (skst) {
We saw the following panic on a RHEL7.6 kernel:
Unable to handle kernel paging request for data at address 0xd00000000577f390
Faulting instruction address: 0xd000000002e918f4
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: kpatch_3_10_0_957_1_3_1_1(OEK) nfsd nfs_acl rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache sunrpc virtio_balloon ip_tables xfs libcrc32c virtio_net virtio_console virtio_blk virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
CPU: 9 PID: 5961 Comm: kworker/9:1H Kdump: loaded Tainted: G OE K------------ 3.10.0-957.1.3.el7.ppc64le #1
Workqueue: xprtiod xprt_autoclose [sunrpc]
task: c00000000300c3c0 ti: c0000003f1814000 task.ti: c0000003f1814000
NIP: d000000002e918f4 LR: d000000002e57394 CTR: c00000000089d100
REGS: c0000003f1817980 TRAP: 0300 Tainted: G OE K------------ (3.10.0-957.1.3.el7.ppc64le)
MSR: 8000000100009033 <SF,EE,ME,IR,DR,RI,LE> CR: 240f2084 XER: 20000000
CFAR: 000000010bb5270c DAR: d00000000577f390 DSISR: 40000000 SOFTE: 1
GPR00: c00000000000b054 c0000003f1817c00 d00000000579add8 c000000214f0f4d0
GPR04: c0000003fd618200 c0000003fd618200 0000000000000001 0000000000000dc2
GPR08: 0000000000000dc3 0000000000000000 0000000000000000 d00000000577f370
GPR12: c0000003f1814000 c000000007b85100 c00000000012fd88 c0000003f711bb40
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000001 c0000000013510b0 0000000000000001 fffffffffffffef7
GPR24: 0000000000000000 0000000000000000 0000000000000000 c000000001b60600
GPR28: c000000214f0f000 c000000214f0f4d0 c000000214f0f408 c000000214f0f448
NIP [d000000002e918f4] __rpc_create_common.part.6+0x640/0x533c [sunrpc]
LR [d000000002e57394] xprt_autoclose+0x74/0xe0 [sunrpc]
Call Trace:
[c0000003f1817c00] [c00000000000b054] livepatch_handler+0x30/0x80 (unreliable)
[c0000003f1817c40] [c00000000012333c] process_one_work+0x1dc/0x680
[c0000003f1817ce0] [c000000000123980] worker_thread+0x1a0/0x520
[c0000003f1817d80] [c00000000012fe74] kthread+0xf4/0x100
[c0000003f1817e30] [c00000000000a628] ret_from_kernel_thread+0x5c/0xb4
Instruction dump:
396b4570 f8410018 e98b0020 7d8903a6 4e800420 00000000 73747562 000f49c0
c0000000 3d62fffe 396b4598 f8410018 <e98b0020> 7d8903a6 4e800420 00000000
---[ end trace 98e026b8fa880db7 ]---
The original version of xs_tcp_shutdown() has the following sequence:
0xd000000003cfda44 <xs_tcp_shutdown+148>: addi r1,r1,64
0xd000000003cfda48 <xs_tcp_shutdown+152>: ld r0,16(r1)
0xd000000003cfda4c <xs_tcp_shutdown+156>: ld r29,-24(r1)
0xd000000003cfda50 <xs_tcp_shutdown+160>: ld r30,-16(r1)
0xd000000003cfda54 <xs_tcp_shutdown+164>: ld r31,-8(r1)
0xd000000003cfda58 <xs_tcp_shutdown+168>: mtlr r0
0xd000000003cfda5c <xs_tcp_shutdown+172>: b 0xd000000003cfd768
That is, it restores the stack to the caller's stack frame and then does
a sibling call to the localentry point of xs_reset_transport()). So
when xs_reset_transport() returns, it will return straight to
xs_tcp_shutdown()'s caller (xprt_autoclose).
The patched version of the function has this instead (dumped from a
vmcore):
0xd000000003df0834 <xs_tcp_shutdown+148>: addi r1,r1,64
0xd000000003df0838 <xs_tcp_shutdown+152>: ld r0,16(r1)
0xd000000003df083c <xs_tcp_shutdown+156>: ld r29,-24(r1)
0xd000000003df0840 <xs_tcp_shutdown+160>: ld r30,-16(r1)
0xd000000003df0844 <xs_tcp_shutdown+164>: ld r31,-8(r1)
0xd000000003df0848 <xs_tcp_shutdown+168>: mtlr r0
0xd000000003df084c <xs_tcp_shutdown+172>: b 0xd000000003df0ad0
After restoring the stack, instead of branching directly to
xs_reset_transport(), it (rightfully) branches to a toc stub. A stub is
needed because the function it's branching to is in another module
(branching from the patch module to the sunrpc module).
The stub is:
0xd000000003df0ad0 <xs_tcp_shutdown+816>: addis r11,r2,-1
0xd000000003df0ad4 <xs_tcp_shutdown+820>: addi r11,r11,26328
0xd000000003df0ad8 <xs_tcp_shutdown+824>: std r2,24(r1)
0xd000000003df0adc <xs_tcp_shutdown+828>: ld r12,32(r11)
0xd000000003df0ae0 <xs_tcp_shutdown+832>: mtctr r12
0xd000000003df0ae4 <xs_tcp_shutdown+836>: bctr
And the "std r2,24(r1)" corrupts the caller's stack.
This stub makes sense for a normal call, because the stack would be
owned by the caller of the stub, so it's ok to write r2 to it. But
because this is a sibling call, the stack has been restored and r2 gets
incorrectly saved to the original caller's stack (i.e., xprt_autoclose's
stack).
So xprt_autoclose() -- which is in the sunrpc module -- gets the
livepatch module's toc pointer written to its stack. It panics on when
it tries to use that vlue on its very next call.
Fix it by disallowing sibling calls from patched functions on ppc64le.
In theory we could instead a) generate a custom stub, or b) modify the
kernel livepatch_handler code to save/restore the stack r2 value, but
this is easier for now.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
euspectre
pushed a commit
that referenced
this pull request
Jan 17, 2020
…ymbols
With the following patch:
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index b60c9c7498dd..39a39ca89230 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1594,6 +1594,8 @@ static void xs_tcp_state_change(struct sock *sk)
struct rpc_xprt *xprt;
struct sock_xprt *transport;
+ asm("nop");
+
read_lock_bh(&sk->sk_callback_lock);
if (!(xprt = xprt_from_sock(sk)))
goto out;
I saw the following panic on a RHEL8 kernel:
Unable to handle kernel paging request for data at address 0xcc0080040
Faulting instruction address: 0xc000000000b1515c
Oops: Kernel access of bad area, sig: 7 [#1]
LE SMP NR_CPUS=2048 NUMA PowerNV
Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache nfsd auth_rpcgss nfs_acl lockd grace kpatch_4_18_0_118_1_1(OEK) i2c_dev sunrpc ofpart powernv_flash at24 sg xts ipmi_powernv ipmi_devintf ipmi_msghandler uio_pdrv_genirq uio mtd vmx_crypto ibmpowernv opal_prd xfs libcrc32c sd_mod ast i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm ahci libahci libata tg3 drm_panel_orientation_quirks dm_mirror dm_region_hash dm_log dm_mod
CPU: 15 PID: 7814 Comm: kworker/u260:0 Kdump: loaded Tainted: G OE K --------- - - 4.18.0-118.el8.ppc64le #1
Workqueue: xprtiod xs_tcp_setup_socket [sunrpc]
NIP: c000000000b1515c LR: c000000000ad9968 CTR: c000000000b15140
REGS: c000001fab6ff6b0 TRAP: 0300 Tainted: G OE K --------- - - (4.18.0-118.el8.ppc64le)
MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 44002222 XER: 20040000
CFAR: c000000000078c7c DAR: 0000000cc0080040 DSISR: 00080000 IRQMASK: 0
GPR00: c000000000ad9968 c000001fab6ff930 c000000001662800 0000000cc0080000
GPR04: c00800000f5cfaa4 c000001f998fd0a8 c000001ff67e8080 c0000000016f46f0
GPR08: c000001fb4918f80 0000000000000000 0000000cc0080040 c0000000011b8980
GPR12: 0000000000002000 c000001ffffee200 c00000000017c458 c000001fe8a23a40
GPR16: c00000000150e480 c000001fd6e90090 0000000000000000 0000000000000000
GPR20: c00000000150e498 fffffffffffffef7 0000000000000402 0000000000000000
GPR24: c000001fd6e90380 0000000000000000 c00800000f5cfaa4 0000000000000000
GPR28: 00000000000004c4 c000001f998fd0a8 c00800000f5cfaa4 0000000cc0080000
NIP [c000000000b1515c] dst_release+0x2c/0x110
LR [c000000000ad9968] skb_release_head_state+0x178/0x190
Call Trace:
[c000001fab6ff930] [c000000000b15140] dst_release+0x10/0x110 (unreliable)
[c000001fab6ff9a0] [c000000000ad9968] skb_release_head_state+0x178/0x190
[c000001fab6ff9d0] [c000000000adb058] __kfree_skb+0x28/0x120
[c000001fab6ffa00] [c000000000be8d64] tcp_rcv_state_process+0xc24/0x1180
[c000001fab6ffa90] [c000000000cd5478] tcp_v6_do_rcv+0x1a8/0x5e0
[c000001fab6ffae0] [c000000000ad1724] __release_sock+0xc4/0x1a0
[c000001fab6ffb40] [c000000000ad1850] release_sock+0x50/0xe0
[c000001fab6ffb70] [c000000000c20018] inet_stream_connect+0x68/0x90
[c000001fab6ffbb0] [c000000000ac0f50] kernel_connect+0x30/0x50
[c000001fab6ffbd0] [c00800000f55dc34] xs_tcp_setup_socket+0xbc/0x650 [sunrpc]
[c000001fab6ffc70] [c000000000172014] process_one_work+0x2f4/0x5c0
[c000001fab6ffd10] [c000000000172adc] worker_thread+0xcc/0x760
[c000001fab6ffdc0] [c00000000017c5fc] kthread+0x1ac/0x1c0
[c000001fab6ffe30] [c00000000000b75c] ret_from_kernel_thread+0x5c/0x80
Instruction dump:
60000000 3c4c00b5 3842d6d0 7c0802a6 4b563b61 fbe1fff8 f821ff91 7c7f1b79
4182003c fbc10060 7c0004ac 395f0040 <7d205028> 3129ffff 7d20512d 40c2fff4
The problem is that the function has a GCC switch jump table, and the
.toc had the wrong offset for the jump table.
This is the switch jump table code from xs_tcp_state_changed():
70: 12 00 3d 89 lbz r9,18(r29)
74: 0b 00 89 2b cmplwi cr7,r9,11
78: f8 02 9d 41 bgt cr7,370 <xs_tcp_state_change+0x368>
7c: 00 00 42 3d addis r10,r2,0
7c: R_PPC64_TOC16_HA .toc+0x188
80: 00 00 4a e9 ld r10,0(r10)
80: R_PPC64_TOC16_LO_DS .toc+0x188
84: 64 17 29 79 rldicr r9,r9,2,61
88: aa 4a 2a 7d lwax r9,r10,r9
8c: 14 52 29 7d add r9,r9,r10
90: a6 03 29 7d mtctr r9
94: 20 04 80 4e bctr
98: d8 02 00 00 .long 0x2d8
9c: 38 00 00 00 .long 0x38
a0: d8 02 00 00 .long 0x2d8
a4: d8 02 00 00 .long 0x2d8
a8: 68 02 00 00 .long 0x268
ac: d8 02 00 00 .long 0x2d8
b0: d8 02 00 00 .long 0x2d8
b4: c8 01 00 00 .long 0x1c8
b8: 38 01 00 00 .long 0x138
bc: 88 01 00 00 .long 0x188
c0: d8 02 00 00 .long 0x2d8
c4: 68 01 00 00 .long 0x168
The switch jump table address is at offset 0x98. The code reads this
offset from .toc+0x188:
Relocation section '.rela.toc' at offset 0x75320 contains 134 entries:
Offset Info Type Symbol's Value Symbol's Name + Addend
0000000000000188 0000003f00000026 R_PPC64_ADDR64 0000000000000000 .text.xs_tcp_state_change + 98
After create-diff-object runs, the .toc entry now looks like this:
0000000000000188 0000000200000026 R_PPC64_ADDR64 0000000000000008 xs_tcp_state_change + 98
Notice the offset is the same, but it's now referring to the function
symbol instead of the text symbol. That's done by
kpatch_replace_sections_syms().
On x86, that's not a problem, because the function symbol is at offset 0
in the .text.function section. So the section symbol and the function
symbol are at the same location.
But on ppc64le, with -ffunction-sections, GCC 6+ somehow thinks it's a
good idea to associate the function symbol with the localentry point,
which is at an 8-byte offset from its corresponding section:
Num: Value Size Type Bind Vis Ndx Name
2: 0000000000000008 1228 FUNC LOCAL DEFAULT 3 xs_tcp_state_change [<localentry>: 8]
Notice the "Value" is 8 instead of 0.
That causes the .toc entry's jump table address to be wrongly offset by
8 bytes.
The fix is to adjust the rela addend accordingly in
kpatch_replace_sections_syms().
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
euspectre
pushed a commit
that referenced
this pull request
Jan 17, 2020
We saw the following panic on ppc64le when loading the macro-callbacks integration test: livepatch: enabling patch 'kpatch_macro_callbacks' Oops: Exception in kernel mode, sig: 4 [#1] LE SMP NR_CPUS=2048 NUMA pSeries Modules linked in: kpatch_macro_callbacks(OEK+) rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache sunrpc sg pseries_rng xts vmx_crypto xfs libcrc32c sd_mod ibmvscsi scsi_transport_srp ibmveth dm_mirror dm_region_hash dm_log dm_mod [last unloaded: kpatch_gcc_static_local_var_6] CPU: 2 PID: 17445 Comm: insmod Kdump: loaded Tainted: G OE K --------- - - 4.18.0-128.el8.ppc64le #1 NIP: d00000000bb708e0 LR: c0000000001fd610 CTR: d00000000bb708e0 REGS: c00000040e98f640 TRAP: 0700 Tainted: G OE K --------- - - (4.18.0-128.el8.ppc64le) MSR: 800000000288b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28008228 XER: 20040003 CFAR: c0000000001fd60c IRQMASK: 0 GPR00: c0000000001fd5c0 c00000040e98f8c0 c000000001662a00 c000000733525400 GPR04: 0000000000000800 0000000000000800 c0000000015e2c00 c0000007335254a8 GPR08: 0000000000000001 d00000000bb708e0 c0000007eeb68400 0000000000000000 GPR12: d00000000bb708e0 c000000007fad600 0000000000000001 aaaaaaaaaaaaaaab GPR16: 000000000000ff20 000000000000fff1 000000000000fff2 d00000000bb90000 GPR20: 00000000000000a9 c00000040e98fc00 c000000000d8a728 c00000040e98fc00 GPR24: d00000000bb73f88 00000000006080c0 d00000000bb73a38 c000000733525400 GPR28: 0000000000000001 c000000733525400 ffffffffffffffed c0000007eeb60900 NIP [d00000000bb708e0] callback_info.isra.0+0x7c/0x66c [kpatch_macro_callbacks] LR [c0000000001fd610] __klp_enable_patch+0x130/0x230 Call Trace: [c00000040e98f8c0] [c0000000001fd5c0] __klp_enable_patch+0xe0/0x230 (unreliable) [c00000040e98f940] [c0000000001fd7d8] klp_enable_patch+0xc8/0x100 [c00000040e98f980] [d00000000bb7079c] patch_init+0x460/0x4cc [kpatch_macro_callbacks] [c00000040e98fa20] [c000000000010108] do_one_initcall+0x58/0x248 [c00000040e98fae0] [c00000000023b860] do_init_module+0x80/0x330 [c00000040e98fb70] [c0000000002416a4] load_module+0x3994/0x3d00 [c00000040e98fd30] [c000000000241cf4] sys_finit_module+0xc4/0x130 [c00000040e98fe30] [c00000000000b388] system_call+0x5c/0x70 Instruction dump: 7cea482a 48000235 e8410018 48000014 3c620000 e8638160 48000221 e8410018 38210060 e8010010 7c0803a6 4e800020 <0000ae18> 00000000 3c4c0001 3842ae18 The problem was introduced by a recent fix: e8f7f2d ("create-diff-object/ppc64le: Fix replace_sections_syms() for bundled symbols") We didn't notice the fact that there's a hack in kpatch_include_callback_elements() which reverts the work of kpatch_replace_sections_syms() for callback function symbols. The problem is that that revert is only partial, causing the callback pointers to point to the .TOC data which is located 8 bytes before the start of the function code. This happens because kpatch_include_callback_elements() makes the same assumption that kpatch_replace_sections_syms() had previously made: that bundled symbols are always located at the start of their corresponding sections. kpatch_include_callback_elements() mysteriously strips references to the callback function symbols, replacing them with section symbols. In this case it replaced a 'pre_patch_callback' function reference with a '.text.unlikely.pre_patch_callback' section reference. But it didn't adjust the rela->addend accordingly. Joe discovered the reasoning for why kpatch_include_callback_elements() removes function symbol references in the commit log for 7dfad2f ("fix dynrela corruption in load/unload hooks"): In the case of the hook functions, we strip the FUNC symbol to prevent it from being added to the kpatch.funcs section as a patched function. But that justification doesn't really make sense, at least not with the current code. Callbacks aren't added to .kpatch.funcs anyway. They're classifed as NEW. Only CHANGED functions are added to .kpatch.funcs. So remove that hack, fixing this bug in the process. This does have a side effect of showing the callback functions as new functions, because their symbols are now included. Before: aio.o: found callback: post_unpatch_callback aio.o: found callback: pre_patch_callback aio.o: found callback: pre_unpatch_callback aio.o: new function: callback_info.isra.0 After: aio.o: found callback: post_unpatch_callback aio.o: found callback: pre_patch_callback aio.o: found callback: pre_unpatch_callback aio.o: new function: callback_info.isra.0 aio.o: new function: pre_patch_callback aio.o: new function: post_patch_callback aio.o: new function: pre_unpatch_callback aio.o: new function: post_unpatch_callback But anyway they _are_ new functions, so the new output seems more correct to me. Fixes: e8f7f2d ("create-diff-object/ppc64le: Fix replace_sections_syms() for bundled symbols") Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was observed by Evgenii Shatokhin in PR#755, that when the RCU callback was called on the patched function, from unloaded livepatch module triggered a kernel crash.
This patch implements the approach on Powerpc outlined in PR#755. With -mcmodel=large, like any other data, function pointers are also loaded relative to the current TOC base and are populated as relocation entries in .toc section. Every function call/passing such function as pointers needs to access them through .toc section + offset.
The simplest approach would be to convert the .toc + offset relocation into a dynamic rela referring to the original function address but the same function is also a patched function. Thus, on PowerPC we need to append a new relocation entry into .rela.toc, duplicating the entry used as a function pointer and converting the newly added rela as dynamic rela.
Also, modify the function to load the new entry by modifying the addend value to newly created dynamic rela.