Re: [RFC 00/31] objtool, livepatch: Livepatch module generation

[Joe Lawrence <joe.lawrence@redhat.com>]

On Thu, Sep 12, 2024 at 09:44:04AM -0400, Joe Lawrence wrote:
> On Wed, Sep 11, 2024 at 12:39:42AM -0700, Josh Poimboeuf wrote:
> > On Mon, Sep 02, 2024 at 08:59:43PM -0700, Josh Poimboeuf wrote:
> > > Hi,
> > > 
> > > Here's a new way to build livepatch modules called klp-build.
> > > 
> > > I started working on it when I realized that objtool already does 99% of
> > > the work needed for detecting function changes.
> > > 
> > > This is similar in concept to kpatch-build, but the implementation is
> > > much cleaner.
> > > 
> > > Personally I still have reservations about the "source-based" approach
> > > (klp-convert and friends), including the fragility and performance
> > > concerns of -flive-patching.  I would submit that klp-build might be
> > > considered the "official" way to make livepatch modules.
> > > 
> > > Please try it out and let me know what you think.  Based on v6.10.
> > > 
> > > Also avaiable at:
> > > 
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git klp-build-rfc
> > 
> > Here's an updated branch with a bunch of fixes.  It's still incompatible
> > with BTF at the moment, otherwise it should (hopefully) fix the rest of
> > the issues reported so far.
> > 
> > While the known bugs are fixed, I haven't finished processing all the
> > review comments yet.  Once that happens I'll post a proper v2.
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git klp-build-v1.5
> 
> Hi Josh,
> 
> I've had much better results with v1.5, thanks for collecting up those
> fixes in a branch.
>

Today's experiment used the centos-stream-10's kernel config with
CONFIG_MODULE_ALLOW_BTF_MISMATCH=y and cs-10's gcc (GCC) 14.2.1 20240801
(Red Hat 14.2.1-1).

First, more gcc nits (running top-level `make`):

  check.c: In function ‘decode_instructions’:
  check.c:410:54: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
    410 |                                 insns = calloc(sizeof(*insn), INSN_CHUNK_SIZE);
        |                                                      ^
  check.c:410:54: note: earlier argument should specify number of elements, later size of each element
  check.c: In function ‘init_pv_ops’:
  check.c:551:38: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
    551 |         file->pv_ops = calloc(sizeof(struct pv_state), nr);
        |                                      ^~~~~~
  check.c:551:38: note: earlier argument should specify number of elements, later size of each element

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 63c2d6c06..c6f192859 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -407,7 +407,7 @@ static void decode_instructions(struct objtool_file *file)
 
 		for (offset = 0; offset < sec_size(sec); offset += insn->len) {
 			if (!insns || idx == INSN_CHUNK_MAX) {
-				insns = calloc(sizeof(*insn), INSN_CHUNK_SIZE);
+				insns = calloc(INSN_CHUNK_SIZE, sizeof(*insn));
 				ERROR_ON(!insns, "calloc");
 
 				idx = 0;
@@ -548,7 +548,7 @@ static void init_pv_ops(struct objtool_file *file)
 		return;
 
 	nr = sym->len / sizeof(unsigned long);
-	file->pv_ops = calloc(sizeof(struct pv_state), nr);
+	file->pv_ops = calloc(nr, sizeof(struct pv_state));
 	ERROR_ON(!file->pv_ops, "calloc");
 
 	for (idx = 0; idx < nr; idx++)

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

and now a happy build of objtool.


The top-level `make` moves onto building all the kernel objects, but
then objtool vmlinux.o crashes:

  $ gdb --args ./tools/objtool/objtool --sym-checksum --hacks=jump_label --hacks=noinstr --hacks=skylake --ibt --orc --retpoline --rethunk --static-call --uaccess --prefix=16 --link vmlinux.o
  
  Program received signal SIGSEGV, Segmentation fault.
  ignore_unreachable_insn (file=0x435ea0 <file>, insn=0x1cd928c0) at check.c:3980
  3980            if (prev_insn->dead_end &&
  
  (gdb) bt
  #0  ignore_unreachable_insn (file=0x435ea0 <file>, insn=0x1cd928c0) at check.c:3980
  #1  validate_reachable_instructions (file=0x435ea0 <file>) at check.c:4452
  #2  check (file=file@entry=0x435ea0 <file>) at check.c:4610
  #3  0x0000000000412d4f in objtool_run (argc=<optimized out>, argc@entry=14, argv=argv@entry=0x7fffffffdd78) at builtin-check.c:206
  #4  0x0000000000417f9b in main (argc=14, argv=0x7fffffffdd78) at objtool.c:131
  
  (gdb) p prev_insn
  $1 = (struct instruction *) 0x0

which I worked around by copying a similar conditional check on
prev_insn after calling prev_insn_same_sec():

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 63c2d6c06..c6f192859 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3977,7 +3977,7 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
 	 * It may also insert a UD2 after calling a __noreturn function.
 	 */
 	prev_insn = prev_insn_same_sec(file, insn);
-	if (prev_insn->dead_end &&
+	if (prev_insn && prev_insn->dead_end &&
 	    (insn->type == INSN_BUG ||
 	     (insn->type == INSN_JUMP_UNCONDITIONAL &&
 	      insn->jump_dest && insn->jump_dest->type == INSN_BUG)))

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

and now a happy kernel build and boot.


A klp-build of the usual cmdline.patch succeeds, however it generates
some strange relocations:

  Relocation section '.rela.text' at offset 0x238 contains 6 entries:
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  0000000000000016  0000004600000004 R_X86_64_PLT32         0000000000000000 __kmalloc_noprof - 4
  0000000000000035  0000004e00000004 R_X86_64_PLT32         0000000000000000 __fentry__ - 4
  000000000000003c  0000000000000000 R_X86_64_NONE                             -4
  
  Relocation section '.rela.klp.relocs' at offset 0x1168 contains 2 entries:
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  0000000000000000  0000000700000001 R_X86_64_64            0000000000000000 .text + 3c
  0000000000000008  0000000000000001 R_X86_64_64                               -4
  
  Relocation section '.klp.rela.h..text' at offset 0x53f18 contains 1 entry: 
      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
  000000000000003c  0000000000000002 R_X86_64_PC32                             -4

No bueno.  FWIW, Song's 0001-test-klp.patch does seem to build w/o odd
relocations and it loads fine.

--
Joe