Skip to content

Conversation

@sumanthkorikkar
Copy link
Contributor

@sumanthkorikkar sumanthkorikkar commented Mar 7, 2025

Typically, symbols within the same object file can be referenced directly for better performance. However, when the patched code stores a function pointer (R_390_GOTENT and rela symbol type is STT_FUNC), using a dynrela is a safer approach. This ensures that if the function is used as a asynchronous callback, the kernel does not attempt to execute an invalid pointer after the module is unloaded. Instead, with a dynrela, the kernel will invoke the original function, preventing potential crashes.

Test program: Test if the original function ptr address is printed during patch load / unload.
iii-i/linux@0f400db

Patched code (function ptr rela):
void *patchme(void) { printk(KERN_NOTICE "patched\n"); return patchme; }
0: c0 04 00 00 00 00 jgnop 0 <patchme>
6: eb ef f0 88 00 24 stmg %r14,%r15,136(%r15)
c: c4 28 00 00 00 00 lgrl %r2,c <patchme+0xc>
e: R_390_GOTENT .LC0+0x2
12: b9 04 00 ef lgr %r14,%r15
16: e3 f0 ff e8 ff 71 lay %r15,-24(%r15)
1c: e3 e0 f0 98 00 24 stg %r14,152(%r15)
22: c0 e5 00 00 00 00 brasl %r14,22 <patchme+0x22>
24: R_390_PLT32DBL _printk+0x2
28: c4 28 00 00 00 00 lgrl %r2,28 <patchme+0x28>
2a: R_390_GOTENT patchme+0x2 <- func ptr
2e: eb ef f0 a0 00 04 lmg %r14,%r15,160(%r15)
34: c0 f4 00 00 00 00 jg 34 <patchme+0x34>
36: R_390_PC32DBL __s390_indirect_jump_r14+0x2
3a: 07 07 nopr %r7
3c: 07 07 nopr %r7
3e: 07 07 nopr %r7

Reference:

  1. Question about pointer equality #1437
  2. kpatch-build, x86: do not use the patched functions as callbacks directly #755

Typically, symbols within the same object file can be referenced
directly for better performance. However, when the patched code stores a
function pointer (R_390_GOTENT and rela symbol type is STT_FUNC), using
a dynrela is a safer approach. This ensures that if the function is used
as a asynchronous callback, the kernel does not attempt to execute an
invalid pointer after the module is unloaded. Instead, with a dynrela,
the kernel will invoke the original function, preventing potential
crashes.

Test program: Test if the original function ptr address is printed
during patch load / unload.
iii-i/linux@0f400db

Patched code (function ptr rela):
void *patchme(void) { printk(KERN_NOTICE "patched\n"); return patchme; }
   0:   c0 04 00 00 00 00       jgnop   0 <patchme>
   6:   eb ef f0 88 00 24       stmg    %r14,%r15,136(%r15)
   c:   c4 28 00 00 00 00       lgrl    %r2,c <patchme+0xc>
                        e: R_390_GOTENT .LC0+0x2
  12:   b9 04 00 ef             lgr     %r14,%r15
  16:   e3 f0 ff e8 ff 71       lay     %r15,-24(%r15)
  1c:   e3 e0 f0 98 00 24       stg     %r14,152(%r15)
  22:   c0 e5 00 00 00 00       brasl   %r14,22 <patchme+0x22>
                        24: R_390_PLT32DBL      _printk+0x2
  28:   c4 28 00 00 00 00       lgrl    %r2,28 <patchme+0x28>
                        2a: R_390_GOTENT        patchme+0x2  <- func ptr
  2e:   eb ef f0 a0 00 04       lmg     %r14,%r15,160(%r15)
  34:   c0 f4 00 00 00 00       jg      34 <patchme+0x34>
                        36: R_390_PC32DBL       __s390_indirect_jump_r14+0x2
  3a:   07 07                   nopr    %r7
  3c:   07 07                   nopr    %r7
  3e:   07 07                   nopr    %r7

Reference:
1. dynup#1437
2. dynup#755

Signed-off-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
@sumanthkorikkar sumanthkorikkar force-pushed the fix-function-ptr-rela-s390 branch from 84ef443 to 9d1ddec Compare March 7, 2025 09:46
Copy link
Contributor

@joe-lawrence joe-lawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, and thanks for untangling that massive return line. Arch-specific switch may be longer to read, but IMHO way more readable for non-PowerPC cases.

Before merging, I'm trying a few more tests patching an OOT kernel module as trying to find perfect kernel code to test combinations (function pointers in .text, .data, etc.) for an integration test is difficult (as you found noticed). The best compromise we may currently have is to use an OOT module to generate a unit test (cached .o files), we'll see.

@joe-lawrence joe-lawrence merged commit 11c3c75 into dynup:master Mar 14, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants