Message-ID: <20190401193529.GA26375@redhat.com>
Date:   Mon, 1 Apr 2019 15:35:29 -0400
From:   Joe Lawrence <joe.lawrence@...hat.com>
To:     Joao Moreira <jmoreira@...e.de>, Miroslav Benes <mbenes@...e.cz>
Cc:     live-patching@...r.kernel.org, pmladek@...e.cz, jikos@...e.cz,
        nstange@...e.de, jpoimboe@...hat.com, khlebnikov@...dex-team.ru,
        jeyu@...nel.org, matz@...e.de, linux-kernel@...r.kernel.org,
        yamada.masahiro@...ionext.com, linux-kbuild@...r.kernel.org,
        michal.lkml@...kovi.net
Subject: Re: [PATCH v2 2/8] kbuild: Support for Symbols.list creation

On Thu, Mar 28, 2019 at 04:17:03PM -0400, Joe Lawrence wrote:
> On Tue, Mar 26, 2019 at 04:53:12PM -0400, Joe Lawrence wrote:
> > On 3/26/19 2:13 PM, Joao Moreira wrote:
> > >
> > >
> > > On 3/26/19 1:15 PM, Joe Lawrence wrote:
> > >>
> > > > Hi Joao,
> > > >
> > > > This change seems to work okay for (again) single object modules, but
> > > > I'm having issues with multi-object modules.
> > > >
> > >
> > > Hi Joe, thanks for the sources, this made everything much easier in my
> > > side :)
> > >
> > > In the patch below I change a little bit the interface used to inform
> > > kbuild that a module is a livepatch. Instead of defining the flag
> > > LIVEPATCH_ per .o file, we define it per module (what actually makes
> > > much more sense). We later use $(basetarget) in the Makefile for
> > > checking the flags. By doing so, and invoking cmd_livepatch both from
> > > the $(single-used-m) and $(multi-used-m) we ensure that the .livepatch
> > > file is created for each module, what later in the pipeline flags the
> > > invocation of klp-convert.
> > >
> > > I tested the following patch with the sources you provided (with little
> > > modifications, removing the .o from the LIVEPATCH_ definitions and using
> > > the module name instead of the object names), achieving successful
> > > compilation and conversion. I also tested against the sample
> > > livepatches, thus I think it might be ok now.
> >
> > Cool thanks for taking a look -- I can confirm that the toy code I sent over
> > builds with those modifications and so does the sample and selftest I was
> > working on.  I'll set about refactoring that klp-convert selftest to combine
> > .o files into a more compact module.
>
> Hmm, maybe I spoke too soon.
>
> I am having issues if I have a two-object livepatch module in which each
> object file needs to specify its own KLP_MODULE_RELOC for the same
> symbol name.
>
> For example: I have test_klp_convert.ko which is comprised of
> test_klp_convert_a.o. which needs to refer to state_show,1 and
> test_klp_convert_b.o. which needs to refer to state_show,2.
>
>   % make
>   ...
>     KLP     lib/livepatch/test_klp_convert.ko
>   klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,0 vs. vmlinux.state_show,1.
>   klp-convert: Unable to load user-provided sympos
>   make[2]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert.ko] Error 255
>   make[1]: *** [/home/cloud-user/disk/linux/Makefile:1282: modules] Error 2
>   make: *** [Makefile:170: sub-make] Error 2
>
> I take a closer look next week, but in the meantime, see the source
> files below.

Okay, with a fresh set of eyes today, I think the issue can be
summarized as:

  * "Special" livepatch symbols, as processed by klp-convert, require
    external linkage, otherwise a new local storage instance will be
    created and miss klp-convert altogether

  * When linking together two object files, their combined symbol table
    will include a de-duped list of uniquly named global symbols

So if we are to run klp-convert on the combined module object file (as
per v2 plus suggested changes in this thread), we are going to run into
problems if ...


Example
=======

(quoted in previous reply), test_klp_convert_a and test_klp_convert_b
have their own "state_show" variable in which each wishes to resolve to
unique symbol positions (2 and 3 accordingly).


test_klp_convert_a
------------------

We get one symbol table entry and one relocation as expected.

  % eu-readelf --symbols lib/livepatch/test_klp_convert_a.o

  Symbol table [27] '.symtab' contains 38 entries:
   29 local symbols  String table: [28] '.strtab'
    Num:            Value   Size Type    Bind   Vis          Ndx Name
    ...
     30: 0000000000000000      0 NOTYPE  GLOBAL DEFAULT    UNDEF state_show
    ...

  % eu-readelf --relocs lib/livepatch/test_klp_convert_a.o

  Relocation section [12] '.rela.klp.module_relocs.vmlinux' for section [11] '.klp.module_relocs.vmlinux' at offset 0x4b8 contains 1 entry:
    Offset              Type            Value               Addend Name
    000000000000000000  X86_64_64       000000000000000000      +0 state_show


test_klp_convert_b
------------------

Just like the other object file, one symbol table entry and one
relocation:

  % eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
  Symbol table [24] '.symtab' contains 23 entries:
   19 local symbols  String table: [25] '.strtab'
    Num:            Value   Size Type    Bind   Vis          Ndx Name
    ...
     20: 0000000000000000      0 NOTYPE  GLOBAL DEFAULT    UNDEF state_show
    ...

  % eu-readelf --relocs lib/livepatch/test_klp_convert_b.o

  Relocation section [ 9] '.rela.klp.module_relocs.vmlinux' for section [ 8] '.klp.module_relocs.vmlinux' at offset 0x118 contains 1 entry:
    Offset              Type            Value               Addend Name
    000000000000000000  X86_64_64       000000000000000000      +0 state_show


test_klp_convert
----------------

But the combined test_klp_convert.klp.o file has only a single
unresolved "state_show" symbol in its symbol table:

  % eu-readelf --symbols lib/livepatch/test_klp_convert.klp.o

  Symbol table [35] '.symtab' contains 57 entries:
   47 local symbols  String table: [36] '.strtab'
    Num:            Value   Size Type    Bind   Vis          Ndx Name
    ...
     52: 0000000000000000      0 NOTYPE  GLOBAL DEFAULT    UNDEF state_show
    ...

though the .rela.klp.module_relocs.vmlinux relocation section has two
entries:

  % eu-readelf --relocs lib/livepatch/test_klp_convert.klp.o

  Relocation section [17] '.rela.klp.module_relocs.vmlinux' for section [16] '.klp.module_relocs.vmlinux' at offset 0x446c8 contains 2 entries:
    Offset              Type            Value               Addend Name
    000000000000000000  X86_64_64       000000000000000000      +0 state_show
    0x0000000000000010  X86_64_64       000000000000000000      +0 state_show

and it looks like the combined KLP_MODULE_RELOC still contains the two
unique symbol position values (2 and 3):

  % objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
  00000000  00 00 00 00 00 00 00 00  02 00 00 00 00 00 00 00  |................|
  00000010  00 00 00 00 00 00 00 00  03 00 00 00              |............|
  0000001c


Maybe we can work around this by modifying the annotation macros and/or
klp-convert, or live with this for now.

Note: I'm inclined to defer work on resolving this limitation to v4.  I
would like v3 to focus on collecting and squashing the feedback up to
now on v2.  There are a few other outstanding issues that I have run
across while testing this patchset, so I feel that it would be clearer
for folks to base comments on those off a clean v3 slate.

-- Joe

> ==> lib/livepatch/test_klp_convert_a.c <==
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/livepatch.h>
>
> /* klp-convert symbols - vmlinux */
> extern void *state_show;
> __used void print_state_show(void)
> {
> 	pr_info("%s: state_show: %p\n", __func__, state_show);
> }
>
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_a[] = {
> 	KLP_SYMPOS(state_show, 1)
> };
>
> static struct klp_func funcs[] = {
> 	{
> 	}, { }
> };
>
> static struct klp_object objs[] = {
> 	{
> 		/* name being NULL means vmlinux */
> 		.funcs = funcs,
> 	}, { }
> };
>
> static struct klp_patch patch = {
> 	.mod = THIS_MODULE,
> 	.objs = objs,
> };
>
> static int test_klp_convert_init(void)
> {
> 	int ret;
>
> 	ret = klp_enable_patch(&patch);
> 	if (ret)
> 		return ret;
>
> 	return 0;
> }
>
> static void test_klp_convert_exit(void)
> {
> }
>
> module_init(test_klp_convert_init);
> module_exit(test_klp_convert_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Joe Lawrence <joe.lawrence@...hat.com>");
> MODULE_DESCRIPTION("Livepatch test: klp-convert");
>
> ==> lib/livepatch/test_klp_convert_b.c <==
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/livepatch.h>
>
> /* klp-convert symbols - vmlinux */
> extern void *state_show;
>
> __used void print_state_show_b(void)
> {
> 	pr_info("%s: state_show: %p\n", __func__, state_show);
> }
>
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> 	KLP_SYMPOS(state_show, 2)
> };
>
>
> ==> lib/livepatch/Makefile <==
> # SPDX-License-Identifier: GPL-2.0
> #
> # Makefile for livepatch test code.
>
> LIVEPATCH_test_klp_atomic_replace := y
> LIVEPATCH_test_klp_callbacks_demo := y
> LIVEPATCH_test_klp_callbacks_demo2 := y
> LIVEPATCH_test_klp_convert := y
> LIVEPATCH_test_klp_livepatch := y
>
> obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
> 				test_klp_callbacks_demo.o \
> 				test_klp_callbacks_demo2.o \
> 				test_klp_callbacks_busy.o \
> 				test_klp_callbacks_mod.o \
> 				test_klp_convert.o \
> 				test_klp_livepatch.o \
> 				test_klp_shadow_vars.o
>
> test_klp_convert-y := \
> 	test_klp_convert_a.o \
> 	test_klp_convert_b.o
>
> # Target modules to be livepatched require CC_FLAGS_FTRACE
> CFLAGS_test_klp_callbacks_busy.o	+= $(CC_FLAGS_FTRACE)
> CFLAGS_test_klp_callbacks_mod.o		+= $(CC_FLAGS_FTRACE)
> CFLAGS_test_klp_convert_mod.o		+= $(CC_FLAGS_FTRACE)
-- Joe

Powered by blists - more mailing lists