Function pointer translation to klp-relocations broken?

[joe-lawrence]

For a simple patch like:

diff -Nupr src.orig/fs/proc/cmdline.c src/fs/proc/cmdline.c
--- src.orig/fs/proc/cmdline.c	2022-10-24 15:41:08.858760066 -0400
+++ src/fs/proc/cmdline.c	2022-10-24 15:41:11.698715352 -0400
@@ -6,8 +6,7 @@
 
 static int cmdline_proc_show(struct seq_file *m, void *v)
 {
-	seq_puts(m, saved_command_line);
-	seq_putc(m, '\n');
+	seq_printf(m, "%s cmdline_proc_show=%p\n", saved_command_line, cmdline_proc_show);
 	return 0;
 }

kpatch-build used to create a dynrela / klp-relocation for the cmdline_proc_show function pointer so that its value it resolved at runtime to the original function and not the kpatch instance. (See #755 and #789.)

This behavior is no longer true for both RHEL-9.6 x86 and ppc64le arches. For example, from the generated livepatch .ko on power:

  Disassembly of section .text.cmdline_proc_show:
  
  0000000000000000 <cmdline_proc_show-0x8>:
          ...
                          0: R_PPC64_REL64        .TOC.-0x8
  
  0000000000000008 <cmdline_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 <cmdline_proc_show+0xc>
                          14: R_PPC64_REL24       _mcount
    18:   a6 02 08 7c     mflr    r0
    1c:   00 00 22 3d     addis   r9,r2,0
                          1c: R_PPC64_TOC16_HA    .toc+0xa0
    20:   00 00 42 3d     addis   r10,r2,0
>                         20: R_PPC64_TOC16_HA    .toc+0x90
    24:   00 00 29 e9     ld      r9,0(r9)
                          24: R_PPC64_TOC16_LO_DS .toc+0xa0
    28:   00 00 ca e8     ld      r6,0(r10)
>                         28: R_PPC64_TOC16_LO_DS .toc+0x90
    2c:   00 00 42 3d     addis   r10,r2,0
                          2c: R_PPC64_TOC16_HA    .toc+0xa8
    30:   00 00 8a e8     ld      r4,0(r10)
                          30: R_PPC64_TOC16_LO_DS .toc+0xa8
    34:   10 00 01 f8     std     r0,16(r1)
    38:   a1 ff 21 f8     stdu    r1,-96(r1)
    3c:   00 00 a9 e8     ld      r5,0(r9)
    40:   01 00 00 48     bl      40 <cmdline_proc_show+0x38>
                          40: R_PPC64_REL24       seq_printf
    44:   00 00 00 60     nop
    48:   00 00 60 38     li      r3,0
    4c:   60 00 21 38     addi    r1,r1,96
    50:   10 00 01 e8     ld      r0,16(r1)
    54:   a6 03 08 7c     mtlr    r0
    58:   20 00 80 4e     blr

  Relocation section '.rela.text.cmdline_proc_show' at offset 0x1420 contains 10 entries:
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  0000000000000000  0000005a0000002c R_PPC64_REL64          0000000000000000 .TOC. - 8
  0000000000000008  0000000000000076 R_PPC64_ENTRY                             0
  0000000000000014  000000640000000a R_PPC64_REL24          0000000000000000 _mcount + 0
  000000000000001c  0000001100000032 R_PPC64_TOC16_HA       0000000000000000 .toc + a0
> 0000000000000020  0000001100000032 R_PPC64_TOC16_HA       0000000000000000 .toc + 90
  0000000000000024  0000001100000040 R_PPC64_TOC16_LO_DS    0000000000000000 .toc + a0
> 0000000000000028  0000001100000040 R_PPC64_TOC16_LO_DS    0000000000000000 .toc + 90
  000000000000002c  0000001100000032 R_PPC64_TOC16_HA       0000000000000000 .toc + a8
  0000000000000030  0000001100000040 R_PPC64_TOC16_LO_DS    0000000000000000 .toc + a8
  0000000000000040  000000630000000a R_PPC64_REL24          0000000000000000 seq_printf + 0

  Relocation section '.klp.rela.vmlinux..toc' at offset 0x6afe0 contains 1 entry:
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  00000000000000a0  0000006600000026 R_PPC64_ADDR64         0000000000000000 .klp.sym.vmlinux.saved_command_line,0 + 0

The current function pointer detection code looks like:

static int function_ptr_rela(const struct rela *rela)
{
	const struct rela *rela_toc = toc_rela(rela);

	return (rela_toc && rela_toc->sym->type == STT_FUNC &&
		!rela_toc->sym->parent &&
		rela_toc->addend == (int)rela_toc->sym->sym.st_value &&
		(rela->type == R_X86_64_32S ||
		rela->type == R_PPC64_TOC16_HA ||
		rela->type == R_PPC64_TOC16_LO_DS));
}

For x86_64, if I de-tangle the rela_toc->* checks from the R_X86_64_32S check (effectively reducing the condition to only verify the relocation type on x86), then this architecture passes.

For ppc64le, I believe the expectation that rela_toc->addend == (int)rela_toc->sym->sym.st_value is failing. If I hack in an 8-byte offset to the addend, then the klp-relocation is created once again.

This issue is a Friday brain dump. I don't have PowerPC arch paged into my brain, so I'm wondering if the fact that on RHEL-9, the function symbol is set 8-bytes into section (rather than at the beginning as on RHEL-7) is messing with this addend check.

[puranjaymohan]

the function symbol is set 8-bytes into section (rather than at the beginning as on RHEL-7) is messing with this addend check.

Could this be related to the two nops added by -fpatchable-function-entry=2 ? because on arm64 this is what happens.

EDIT - st_value should match the offset added by nops!

[wardenjohn]

I have a basic question in my mind. .rela.text.cmdline_proc_show is the relocation to the text section which contains the patched code, isn't it?

[joe-lawrence]

@puranjaymohan says:

Could this be related to the two nops added by -fpatchable-function-entry=2 ? because on arm64 this is what happens.

EDIT - st_value should match the offset added by nops!

I thought about that at first too, but RHEL doesn't use -fpatchable-function-entry on ppc64le until rhel-10. In that case, we would have seen something like:

Disassembly of section .text.cmdline_proc_show:

0000000000000000 <cmdline_proc_show-0x8>:
        ...
                        0: R_PPC64_REL64        .TOC.-0x8

0000000000000008 <cmdline_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:   00 00 00 60     nop
> 14:   00 00 00 60     nop

where the nops basically replace the dummy mflr / bl mcount instructions.

@wardenjohn says:

I have a basic question in my mind. .rela.text.cmdline_proc_show is the relocation to the text section which contains the patched code, isn't it?

Yes that is correct, the patch modifies cmdline_proc_show() which is assigned the .text.cmdline_proc_show section due to the -ffunction-sections compiler flag. So then .rela.text.cmdline_proc_show are the relocations to that text section.

[joe-lawrence]

Another Friday info dump (not much new info). RHEL-7 the (kpatch) kernel builds fs/proc/cmdline.o with prologue like:

 0000000000000000 <cmdline_proc_show>:
     0:   00 00 4c 3c     addis   r2,r12,0
                          0: R_PPC64_REL16_HA     .TOC.
     4:   00 00 42 38     addi    r2,r2,0
                          4: R_PPC64_REL16_LO     .TOC.+0x4

despite kpatch-build adding -mcmodel=large to the gcc build line.

On RHEL-8 and onward, we get the expected large-code model prologue:

  0000000000000000 <cmdline_proc_show-0x8>:
          ...
                          0: R_PPC64_REL64        .TOC.-0x8
  
  0000000000000008 <cmdline_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

I haven't figured out why RHEL-7 builds with what looks like the medium/small code model rather than the large. Maybe it's the older gcc version (4.8.5) or mcpu target (power8)?

[github-actions]

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.