dynup / kpatch

kpatch - live kernel patching
GNU General Public License v2.0
1.49k stars 304 forks source link

special-static.patch can fail to build on s390x / upstream v6.8 #1381

Closed joe-lawrence closed 5 months ago

joe-lawrence commented 7 months ago

For some reason, gcc on s390x decides to rearrange instructions due to this patch (my own notes correlating instructions added on the right hand side) and the build fails as those changes land in the .init.text section:

Disassembly of section .init.text:

 0000000000000000 <coredump_filter_setup>:
    0:  c0 04 00 00 00 00       jgnop   0 <coredump_filter_setup>
    6:  eb ef f0 88 00 24       stmg    %r14,%r15,136(%r15)
    c:  b9 04 00 ef             lgr     %r14,%r15
@@ -122,26 +122,26 @@
 00000000000001e0 <fork_idle>:
  1e0:  c0 04 00 00 00 00       jgnop   1e0 <fork_idle>
  1e6:  eb 9f f0 60 00 24       stmg    %r9,%r15,96(%r15)
  1ec:  b9 04 00 ef             lgr     %r14,%r15
  1f0:  e3 f0 ff 40 ff 71       lay     %r15,-192(%r15)
  1f6:  e3 e0 f0 98 00 24       stg     %r14,152(%r15)
  1fc:  c4 a8 00 00 00 00       lgrl    %r10,1fc <fork_idle+0x1c>
                        1fe: R_390_GOTENT       init_struct_pid+0x2
- 202:  d7 77 f0 a8 f0 a8       xc      168(120,%r15),168(%r15)            A
- 208:  c0 10 00 00 00 00       larl    %r1,208 <fork_idle+0x28>           B
-                       20a: R_390_PLT32DBL     idle_dummy+0x2
+ 202:  c0 10 00 00 00 00       larl    %r1,202 <fork_idle+0x22>           B
+                       204: R_390_PLT32DBL     idle_dummy+0x2
+ 208:  d7 77 f0 a8 f0 a8       xc      168(120,%r15),168(%r15)            A
  20e:  b9 04 00 92             lgr     %r9,%r2
- 212:  e3 10 f1 00 00 24       stg     %r1,256(%r15)                      C
- 218:  92 80 f0 cc             mvi     204(%r15),128                      D
- 21c:  e5 48 f0 a0 01 00       mvghi   160(%r15),256                      E
- 222:  e5 4c f0 fc 00 01       mvhi    252(%r15),1                        F
- 228:  b9 04 00 2a             lgr     %r2,%r10                           G
- 22c:  41 50 f0 a0             la      %r5,160(%r15)                      H
+ 212:  41 50 f0 a0             la      %r5,160(%r15)                      H
+ 216:  e3 10 f1 00 00 24       stg     %r1,256(%r15)                      C
+ 21c:  92 80 f0 cc             mvi     204(%r15),128                      D
+ 220:  e5 48 f0 a0 01 00       mvghi   160(%r15),256                      E
+ 226:  e5 4c f0 fc 00 01       mvhi    252(%r15),1                        F
+ 22c:  b9 04 00 2a             lgr     %r2,%r10                           G
  230:  a7 49 00 00             lghi    %r4,0
  234:  a7 39 00 00             lghi    %r3,0
  238:  c0 e5 00 00 00 00       brasl   %r14,238 <fork_idle+0x58>
                        23a: R_390_PLT32DBL     copy_process+0x2
  23e:  a7 19 f0 00             lghi    %r1,-4096
  242:  b9 04 00 b2             lgr     %r11,%r2
  246:  ec 21 00 2d 20 65       clgrjh  %r2,%r1,2a0 <fork_idle+0xc0>
  24c:  e3 a0 27 f8 00 24       stg     %r10,2040(%r2)

The fork_idle() function was annotated as __init way back in v5.14 torvalds/linux@f1a0a376ca0c ("sched/core: Initialize the idle task with preemption disabled"), so I'm surprised this is only failing now.

@jpoimboe : two questions about this test:

  1. special-static.patch was introduced to verify that kpatch-build could handle __warned and __key so-called special static local variables. Do these special symbols need to exist in kpatch-modified functions, or merely somewhere in kpatch-modified object files?
  2. Could this be turned into an unit test? The patch doesn't do much, and nothing I see specifically regarding the special symbols.
jpoimboe commented 7 months ago

Hm, that's odd. I haven't seen anything like that before. Maybe changes to non-bundled sections could be downgraded to warnings. Or if this seems like a fluke we could do KPATCH_IGNORE_SECTION() for that patch.

  • special-static.patch was introduced to verify that kpatch-build could handle __warned and __key so-called special static local variables. Do these special symbols need to exist in kpatch-modified functions, or merely somewhere in kpatch-modified object files?

I'm not 100% sure, but I think the current test triggers a change in a function which actually uses such variables, so let's stick with that :-)

  • Could this be turned into an unit test? The patch doesn't do much, and nothing I see specifically regarding the special symbols.

Yeah, I think so.

joe-lawrence commented 7 months ago

Here are the object files for special-static.patch as built on RHEL-9.3: special-static-objs.tar.gz

It looks like copy_signal() only references a few key variables, though the greater fork.o file a few others (like `already_done,_entry, andfunc`). If that's good enough, then I can push the stripped objects into the unit-test repo and deprecate the special-static.patch.

github-actions[bot] commented 6 months ago

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[bot] commented 6 months ago

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

github-actions[bot] commented 5 months ago

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[bot] commented 5 months ago

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