Re: [PATCH v4 00/10] klp-convert livepatch build tooling

Re: [PATCH v4 00/10] klp-convert livepatch build tooling

[Miroslav Benes]

On Fri, 14 Jun 2019, Petr Mladek wrote:

> On Thu 2019-06-13 16:48:02, Joe Lawrence wrote:
> > On 6/13/19 9:15 AM, Joe Lawrence wrote:
> > > On 6/13/19 9:00 AM, Miroslav Benes wrote:
> > >> Hi Joe,
> > >>
> > >> first, I'm sorry for the lack of response so far.
> > >>
> > >> Maybe you've already noticed but the selftests fail. Well, at least in
> > >> my VM. When test_klp_convert1.ko is loaded, the process is killed with
> > >>
> > >> [  518.041826] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > >> [  518.042816] #PF: supervisor read access in kernel mode
> > >> [  518.043393] #PF: error_code(0x0000) - not-present page
> > >> [  518.043981] PGD 0 P4D 0
> > >> [  518.044185] Oops: 0000 [#1] SMP PTI
> > >> [  518.044518] CPU: 2 PID: 2255 Comm: insmod Tainted: G           O  K   5.1.0-klp_convert_v4-193435-g67748576637e #2
> > >> [  518.045784] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
> > >> [  518.046940] RIP: 0010:test_klp_convert_init+0x1c/0x40 [test_klp_convert1]
> > >> [  518.047611] Code: 1b a0 48 89 c6 e9 a8 c0 f4 e0 0f 1f 40 00 0f 1f 44 00 00 53 48 c7 c7 00 30 1b a0 e8 5e 33 f6 e0 85 c0 89 c3 74 04 89 d8 5b c3 <48> 8b 35 5d ef e4 5f 48 c7 c7 28 20 1b a0 e8 75 c0 f4 e0 e8 6c ff
> > >> [  518.049779] RSP: 0018:ffffc90000f37cc8 EFLAGS: 00010246
> > >> [  518.050243] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000027de0
> > >> [  518.050922] RDX: 0000000000000000 RSI: 0000000000000006 RDI: ffff88807ab54f40
> > >> [  518.051619] RBP: ffffffffa01b1080 R08: 0000000096efde7a R09: 0000000000000001
> > >> [  518.052332] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000ffffffff
> > >> [  518.053012] R13: 0000000000000000 R14: ffff888078b55000 R15: ffffc90000f37ea0
> > >> [  518.053714] FS:  00007febece1fb80(0000) GS:ffff88807d400000(0000) knlGS:0000000000000000
> > >> [  518.054514] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [  518.055078] CR2: 0000000000000000 CR3: 000000007a56a000 CR4: 00000000000006e0
> > >> [  518.055818] Call Trace:
> > >> [  518.056007]  do_one_initcall+0x6a/0x2da
> > >> [  518.056340]  ? do_init_module+0x22/0x230
> > >> [  518.056702]  ? rcu_read_lock_sched_held+0x96/0xa0
> > >> [  518.057125]  ? kmem_cache_alloc_trace+0x284/0x2e0
> > >> [  518.057493]  do_init_module+0x5a/0x230
> > >> [  518.057900]  load_module+0x17bc/0x1f50
> > >> [  518.058214]  ? __symbol_put+0x40/0x40
> > >> [  518.058499]  ? vfs_read+0x12d/0x160
> > >> [  518.058766]  __do_sys_finit_module+0x83/0xc0
> > >> [  518.059122]  do_syscall_64+0x57/0x190
> > >> [  518.059407]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > >> ...
> > >>
> > >> It crashes right in test_klp_convert_init() when print_*() using
> > >> supposed-to-be-converted symbols are called. I'll debug it next week. Can
> > >> you reproduce it too?
> > > 
> > > Hey, thanks for the report..
> > > 
> > > I don't recall the tests crashing, but I had put this patchset on the
> > > side for a few weeks now.  I'll try to fire up a VM and see what happens
> > > today.
> > > 
> > 
> > Hmm, I haven't been able to reproduce using my original base (Linux 5.1-rc6)
> > or when rebased ontop of livepatching.git/master + 997a55f3fb6d("stacktrace: Unbreak stack_trace_save_tsk_reliable()")
> 
> I stared into the code a bit but I did not find any bug. Let's hope
> that it was just some pre-vacation last minute mistake (system
> inconsistency or so ;-)

It was not and I do not understand it much, so there is a brain dump here.

I'll take test_klp_convert1.c as an example. When you compile it, 
test_klp_convert1.klp.o (not-yet-converted kernel module) has these 
relevant relocations.

Relocation section '.rela.text' at offset 0x1328 contains 27 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000008  0000002800000002 R_X86_64_PC32          0000000000000000 saved_command_line - 4
000000000000009f  0000002800000002 R_X86_64_PC32          0000000000000000 saved_command_line - 4

When converted, test_klp_convert1.ko has

Relocation section '.rela.text' at offset 0x138 contains 22 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
000000000000009f  0000002600000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4

and

Relocation section '.klp.rela.vmlinux..text' at offset 0x1968 contains 1 
entry:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000008  0000002600000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4

See? One of the relocations was not moved to the correct .klp.rela 
section. The final module should have

Relocation section '.klp.rela.vmlinux..text' at offset 0x1928 contains 2 
entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
000000000000009f  0000002600000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4
0000000000000008  0000002600000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4

and no saved_command_line relocation in .rela.text.

It thus makes sense that there is a NULL pointer dereference. The code 
accesses non-relocated symbol and boom.

So I made a couple of experiments and found that GCC is somehow involved. 
If klp-convert (from scripts/livepatch/) is compiled with our GCC 4.8.5 
from SLE12, the output is incorrect. If I compile it with GCC 7.4.0 from 
openSUSE Leap 15.1, the output is correct.

If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make 
convert_rela() list-safe") (from Joe's expanded github tree), the problem 
disappears.

I haven't spotted any problem in the code and I cannot explain a 
dependency on GCC version. Any ideas?

Miroslav

Re: [PATCH v4 00/10] klp-convert livepatch build tooling

[Joe Lawrence]

On 6/25/19 7:36 AM, Miroslav Benes wrote:
> 
 > [ ... snip ... ]
 >
> So I made a couple of experiments and found that GCC is somehow involved.
> If klp-convert (from scripts/livepatch/) is compiled with our GCC 4.8.5
> from SLE12, the output is incorrect. If I compile it with GCC 7.4.0 from
> openSUSE Leap 15.1, the output is correct.
> 
> If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make
> convert_rela() list-safe") (from Joe's expanded github tree), the problem
> disappears.
> 
> I haven't spotted any problem in the code and I cannot explain a
> dependency on GCC version. Any ideas?
> 

Thanks for revisiting and debugging this.  Narrowing it down to my "fix" 
to convert_rela() should be helpful.

In my case, I was probably testing with RHEL-8, which has gcc 8.2 vs 
RHEL-7 which has gcc 4.8.  I'll have to make sure to try with a few 
different versions for the next round.

-- Joe

Re: [PATCH v4 00/10] klp-convert livepatch build tooling

[Joe Lawrence]

On Tue, Jun 25, 2019 at 01:36:37PM +0200, Miroslav Benes wrote:
>
> [ ... snip ... ]
>
> If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make
> convert_rela() list-safe") (from Joe's expanded github tree), the problem
> disappears.
>
> I haven't spotted any problem in the code and I cannot explain a
> dependency on GCC version. Any ideas?
>

I can confirm that test_klp_convert1.ko crashes with RHEL-7 and its
older gcc.  I added some debugging printf's to klp-convert and see:

  % ./scripts/livepatch/klp-convert \
          ./Symbols.list \
          lib/livepatch/test_klp_convert1.klp.o \
          lib/livepatch/test_klp_convert1.ko | \
          grep saved_command_line

  convert_rela: oldsec: .rela.text rela @ 0x1279670 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
  convert_rela: oldsec: .rela.text rela @ 0x1279cd0 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x9a
  move_rela: rela @ 0x1279670 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
  main: skipping rela @ 0x1279cd0 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) (!must_convert)

I think the problem is:

- Relas at different offsets, but for the same symbol may share symbol
  storage.  Note the same rela->sym value above.

- Before d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
  list-safe"), convert_rela() iterated through the entire section's
  relas, moving any of the same name.  This was determined not to be
  list safe when moving consecutive relas in the linked list.

- After d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
  list-safe"), convert_rela() still iterates through the section relas,
  but only updates r1->sym->klp_rela_sec instead of moving them.
  move_rela() was added to be called by the for-each-rela loop in
  main().

  - Bug 1: klp_rela_sec probably belongs in struct rela and not struct
    symbol

  - Bug 2: the main loop skips over second, third, etc. matching relas
    anyway as the shared symbol name will have already been converted

The following fix might not be elegant, but I can't think of a clever
way to handle the original issue d59cadc0a8f8 ("[squash] klp-convert:
make convert_rela() list-safe") as well as these resulting regressions.
So I broke out the moving of relas to a seperate loop.  That is probably
worth a comment and at the same time we might be able to drop some of
these other "safe" loop traversals for ordinary list_for_each_entry.

-- Joe

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

diff --git a/scripts/livepatch/elf.h b/scripts/livepatch/elf.h
index 7551409..57a8242 100644
--- a/scripts/livepatch/elf.h
+++ b/scripts/livepatch/elf.h
@@ -33,7 +33,6 @@ struct symbol {
 	struct list_head list;
 	GElf_Sym sym;
 	struct section *sec;
-	struct section *klp_rela_sec;
 	char *name;
 	unsigned int idx;
 	unsigned char bind, type;
@@ -45,6 +44,7 @@ struct rela {
 	struct list_head list;
 	GElf_Rela rela;
 	struct symbol *sym;
+	struct section *klp_rela_sec;
 	unsigned int type;
 	unsigned long offset;
 	int addend;
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index b5873ab..50d6471 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -525,7 +525,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r,
 
 	list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
 		if (r1->sym->name == r->sym->name) {
-			r1->sym->klp_rela_sec = sec;
+			r1->klp_rela_sec = sec;
 		}
 	}
 	return true;
@@ -535,7 +535,7 @@ static void move_rela(struct rela *r)
 {
 	/* Move the converted rela to klp rela section */
 	list_del(&r->list);
-	list_add(&r->list, &r->sym->klp_rela_sec->relas);
+	list_add(&r->list, &r->klp_rela_sec->relas);
 }
 
 /* Checks if given symbol name matches a symbol in exp_symbols */
@@ -687,8 +687,11 @@ int main(int argc, const char **argv)
 					return -1;
 				}
 			}
+		}
 
-			move_rela(rela);
+		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+			if (is_converted(rela->sym->name))
+				move_rela(rela);
 		}
 	}
 

Re: [PATCH v4 00/10] klp-convert livepatch build tooling

[Miroslav Benes]

On Tue, 25 Jun 2019, Joe Lawrence wrote:

> On Tue, Jun 25, 2019 at 01:36:37PM +0200, Miroslav Benes wrote:
> >
> > [ ... snip ... ]
> >
> > If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make
> > convert_rela() list-safe") (from Joe's expanded github tree), the problem
> > disappears.
> >
> > I haven't spotted any problem in the code and I cannot explain a
> > dependency on GCC version. Any ideas?
> >
> 
> I can confirm that test_klp_convert1.ko crashes with RHEL-7 and its
> older gcc.  I added some debugging printf's to klp-convert and see:
> 
>   % ./scripts/livepatch/klp-convert \
>           ./Symbols.list \
>           lib/livepatch/test_klp_convert1.klp.o \
>           lib/livepatch/test_klp_convert1.ko | \
>           grep saved_command_line
> 
>   convert_rela: oldsec: .rela.text rela @ 0x1279670 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
>   convert_rela: oldsec: .rela.text rela @ 0x1279cd0 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x9a
>   move_rela: rela @ 0x1279670 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
>   main: skipping rela @ 0x1279cd0 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) (!must_convert)
> 
> I think the problem is:
> 
> - Relas at different offsets, but for the same symbol may share symbol
>   storage.  Note the same rela->sym value above.
> 
> - Before d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
>   list-safe"), convert_rela() iterated through the entire section's
>   relas, moving any of the same name.  This was determined not to be
>   list safe when moving consecutive relas in the linked list.
> 
> - After d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
>   list-safe"), convert_rela() still iterates through the section relas,
>   but only updates r1->sym->klp_rela_sec instead of moving them.
>   move_rela() was added to be called by the for-each-rela loop in
>   main().
> 
>   - Bug 1: klp_rela_sec probably belongs in struct rela and not struct
>     symbol
> 
>   - Bug 2: the main loop skips over second, third, etc. matching relas
>     anyway as the shared symbol name will have already been converted

Yes, it explains the issue.
 
> The following fix might not be elegant, but I can't think of a clever
> way to handle the original issue d59cadc0a8f8 ("[squash] klp-convert:
> make convert_rela() list-safe") as well as these resulting regressions.
> So I broke out the moving of relas to a seperate loop.

It works. Thanks Joe.

> That is probably
> worth a comment and at the same time we might be able to drop some of
> these other "safe" loop traversals for ordinary list_for_each_entry.

I think _safe from list_for_each_entry_safe(rela, tmprela, &sec->relas, 
list) in the main loop could be dropped, because convert_rela() only marks 
relas and does not move them anywhere. 
Similarly, list_for_each_entry_safe(r1, r2, &oldsec->relas, list) in 
convert_rela() itself.

Miroslav

Re: [PATCH v4 03/10] livepatch: Add klp-convert tool

[Masahiro Yamada]

On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
>
> Livepatches may use symbols which are not contained in its own scope,
> and, because of that, may end up compiled with relocations that will
> only be resolved during module load. Yet, when the referenced symbols
> are not exported, solving this relocation requires information on the
> object that holds the symbol (either vmlinux or modules) and its
> position inside the object, as an object may contain multiple symbols
> with the same name. Providing such information must be done
> accordingly to what is specified in
> Documentation/livepatch/module-elf-format.txt.
>
> Currently, there is no trivial way to embed the required information
> as requested in the final livepatch elf object. klp-convert solves
> this problem in two different forms: (i) by relying on Symbols.list,
> which is built during kernel compilation, to automatically infer the
> relocation targeted symbol, and, when such inference is not possible
> (ii) by using annotations in the elf object to convert the relocation
> accordingly to the specification, enabling it to be handled by the
> livepatch loader.
>
> Given the above, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
>
> The core file of klp-convert is scripts/livepatch/klp-convert.c, which
> implements the heuristics used to solve the relocations and the
> conversion of unresolved symbols into the expected format, as defined
> in [1].
>
> klp-convert receives as arguments the Symbols.list file, an input
> livepatch module to be converted and the output name for the converted
> livepatch. When it starts running, klp-convert parses Symbols.list and
> builds two internal lists of symbols, one containing the exported and
> another containing the non-exported symbols. Then, by parsing the rela
> sections in the elf object, klp-convert identifies which symbols must
> be converted, which are those unresolved and that do not have a
> corresponding exported symbol, and attempts to convert them
> accordingly to the specification.
>
> By using Symbols.list, klp-convert identifies which symbols have names
> that only appear in a single kernel object, thus being capable of
> resolving these cases without the intervention of the developer. When
> various homonymous symbols exist through kernel objects, it is not
> possible to infer the right one, thus klp-convert falls back into
> using developer annotations. If these were not provided, then the tool
> will print a list with all acceptable targets for the symbol being
> processed.
>
> Annotations in the context of klp-convert are accessible as struct
> klp_module_reloc entries in sections named
> .klp.module_relocs.<objname>. These entries are pairs of symbol
> references and positions which are to be resolved against definitions
> in <objname>.
>
> Define the structure klp_module_reloc in
> include/linux/uapi/livepatch.h allowing developers to annotate the
> livepatch source code with it.
>
> klp-convert relies on libelf and on a list implementation. Add files
> scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a
> libelf interfacing layer and scripts/livepatch/list.h, which is a
> list implementation.
>
> Update Makefiles to correctly support the compilation of the new tool,
> update MAINTAINERS file and add a .gitignore file.
>
> [1] - Documentation/livepatch/module-elf-format.txt
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  MAINTAINERS                     |   1 +
>  include/uapi/linux/livepatch.h  |   5 +
>  scripts/Makefile                |   1 +
>  scripts/livepatch/.gitignore    |   1 +
>  scripts/livepatch/Makefile      |   7 +
>  scripts/livepatch/elf.c         | 753 ++++++++++++++++++++++++++++++++
>  scripts/livepatch/elf.h         |  73 ++++
>  scripts/livepatch/klp-convert.c | 713 ++++++++++++++++++++++++++++++
>  scripts/livepatch/klp-convert.h |  39 ++
>  scripts/livepatch/list.h        | 391 +++++++++++++++++
>  10 files changed, 1984 insertions(+)
>  create mode 100644 scripts/livepatch/.gitignore
>  create mode 100644 scripts/livepatch/Makefile
>  create mode 100644 scripts/livepatch/elf.c
>  create mode 100644 scripts/livepatch/elf.h
>  create mode 100644 scripts/livepatch/klp-convert.c
>  create mode 100644 scripts/livepatch/klp-convert.h
>  create mode 100644 scripts/livepatch/list.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 52842fa37261..c1587e1cc385 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9022,6 +9022,7 @@ F:        arch/x86/kernel/livepatch.c
>  F:     Documentation/livepatch/
>  F:     Documentation/ABI/testing/sysfs-kernel-livepatch
>  F:     samples/livepatch/
> +F:     scripts/livepatch/
>  F:     tools/testing/selftests/livepatch/
>  L:     live-patching@vger.kernel.org
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
> diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
> index e19430918a07..1c364d42d38e 100644
> --- a/include/uapi/linux/livepatch.h
> +++ b/include/uapi/linux/livepatch.h
> @@ -12,4 +12,9 @@
>  #define KLP_RELA_PREFIX                ".klp.rela."
>  #define KLP_SYM_PREFIX         ".klp.sym."
>
> +struct klp_module_reloc {
> +       void *sym;
> +       unsigned int sympos;
> +} __attribute__((packed));
> +
>  #endif /* _UAPI_LIVEPATCH_H */
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 9d442ee050bd..bf9ce74b70b0 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -39,6 +39,7 @@ build_unifdef: $(obj)/unifdef
>  subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
>  subdir-$(CONFIG_MODVERSIONS) += genksyms
>  subdir-$(CONFIG_SECURITY_SELINUX) += selinux
> +subdir-$(CONFIG_LIVEPATCH)   += livepatch
>
>  # Let clean descend into subdirs
>  subdir-        += basic dtc gdb kconfig mod package
> diff --git a/scripts/livepatch/.gitignore b/scripts/livepatch/.gitignore
> new file mode 100644
> index 000000000000..dc22fe4b6a5b
> --- /dev/null
> +++ b/scripts/livepatch/.gitignore
> @@ -0,0 +1 @@
> +klp-convert
> diff --git a/scripts/livepatch/Makefile b/scripts/livepatch/Makefile
> new file mode 100644
> index 000000000000..2842ecdba3fd
> --- /dev/null
> +++ b/scripts/livepatch/Makefile
> @@ -0,0 +1,7 @@
> +hostprogs-y                    := klp-convert
> +always                         := $(hostprogs-y)
> +
> +klp-convert-objs               := klp-convert.o elf.o
> +
> +HOST_EXTRACFLAGS               := -g -I$(INSTALL_HDR_PATH)/include -Wall

This looks strange.

Theoretically, you cannot include headers in $(INSTALL_HDR_PATH)/include
from host programs.

headers_install works for the target architecture, not host architecture.
This may cause a strange result when you are cross-compiling the kernel.

BTW, which header in $(INSTALL_HDR_PATH)/include do you need to include ?


Also, -Wall is redundant because it is set by the top-level Makefile.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 03/10] livepatch: Add klp-convert tool

[Masahiro Yamada]

On Wed, Jul 31, 2019 at 11:50 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> >
> > Livepatches may use symbols which are not contained in its own scope,
> > and, because of that, may end up compiled with relocations that will
> > only be resolved during module load. Yet, when the referenced symbols
> > are not exported, solving this relocation requires information on the
> > object that holds the symbol (either vmlinux or modules) and its
> > position inside the object, as an object may contain multiple symbols
> > with the same name. Providing such information must be done
> > accordingly to what is specified in
> > Documentation/livepatch/module-elf-format.txt.
> >
> > Currently, there is no trivial way to embed the required information
> > as requested in the final livepatch elf object. klp-convert solves
> > this problem in two different forms: (i) by relying on Symbols.list,
> > which is built during kernel compilation, to automatically infer the
> > relocation targeted symbol, and, when such inference is not possible
> > (ii) by using annotations in the elf object to convert the relocation
> > accordingly to the specification, enabling it to be handled by the
> > livepatch loader.
> >
> > Given the above, create scripts/livepatch to hold tools developed for
> > livepatches and add source files for klp-convert there.
> >
> > The core file of klp-convert is scripts/livepatch/klp-convert.c, which
> > implements the heuristics used to solve the relocations and the
> > conversion of unresolved symbols into the expected format, as defined
> > in [1].
> >
> > klp-convert receives as arguments the Symbols.list file, an input
> > livepatch module to be converted and the output name for the converted
> > livepatch. When it starts running, klp-convert parses Symbols.list and
> > builds two internal lists of symbols, one containing the exported and
> > another containing the non-exported symbols. Then, by parsing the rela
> > sections in the elf object, klp-convert identifies which symbols must
> > be converted, which are those unresolved and that do not have a
> > corresponding exported symbol, and attempts to convert them
> > accordingly to the specification.
> >
> > By using Symbols.list, klp-convert identifies which symbols have names
> > that only appear in a single kernel object, thus being capable of
> > resolving these cases without the intervention of the developer. When
> > various homonymous symbols exist through kernel objects, it is not
> > possible to infer the right one, thus klp-convert falls back into
> > using developer annotations. If these were not provided, then the tool
> > will print a list with all acceptable targets for the symbol being
> > processed.
> >
> > Annotations in the context of klp-convert are accessible as struct
> > klp_module_reloc entries in sections named
> > .klp.module_relocs.<objname>. These entries are pairs of symbol
> > references and positions which are to be resolved against definitions
> > in <objname>.
> >
> > Define the structure klp_module_reloc in
> > include/linux/uapi/livepatch.h allowing developers to annotate the
> > livepatch source code with it.
> >
> > klp-convert relies on libelf and on a list implementation. Add files
> > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a
> > libelf interfacing layer and scripts/livepatch/list.h, which is a
> > list implementation.
> >
> > Update Makefiles to correctly support the compilation of the new tool,
> > update MAINTAINERS file and add a .gitignore file.
> >
> > [1] - Documentation/livepatch/module-elf-format.txt
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> > Signed-off-by: Joao Moreira <jmoreira@suse.de>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> >  MAINTAINERS                     |   1 +
> >  include/uapi/linux/livepatch.h  |   5 +
> >  scripts/Makefile                |   1 +
> >  scripts/livepatch/.gitignore    |   1 +
> >  scripts/livepatch/Makefile      |   7 +
> >  scripts/livepatch/elf.c         | 753 ++++++++++++++++++++++++++++++++
> >  scripts/livepatch/elf.h         |  73 ++++
> >  scripts/livepatch/klp-convert.c | 713 ++++++++++++++++++++++++++++++
> >  scripts/livepatch/klp-convert.h |  39 ++
> >  scripts/livepatch/list.h        | 391 +++++++++++++++++
> >  10 files changed, 1984 insertions(+)
> >  create mode 100644 scripts/livepatch/.gitignore
> >  create mode 100644 scripts/livepatch/Makefile
> >  create mode 100644 scripts/livepatch/elf.c
> >  create mode 100644 scripts/livepatch/elf.h
> >  create mode 100644 scripts/livepatch/klp-convert.c
> >  create mode 100644 scripts/livepatch/klp-convert.h
> >  create mode 100644 scripts/livepatch/list.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 52842fa37261..c1587e1cc385 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9022,6 +9022,7 @@ F:        arch/x86/kernel/livepatch.c
> >  F:     Documentation/livepatch/
> >  F:     Documentation/ABI/testing/sysfs-kernel-livepatch
> >  F:     samples/livepatch/
> > +F:     scripts/livepatch/
> >  F:     tools/testing/selftests/livepatch/
> >  L:     live-patching@vger.kernel.org
> >  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
> > diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
> > index e19430918a07..1c364d42d38e 100644
> > --- a/include/uapi/linux/livepatch.h
> > +++ b/include/uapi/linux/livepatch.h
> > @@ -12,4 +12,9 @@
> >  #define KLP_RELA_PREFIX                ".klp.rela."
> >  #define KLP_SYM_PREFIX         ".klp.sym."
> >
> > +struct klp_module_reloc {
> > +       void *sym;
> > +       unsigned int sympos;
> > +} __attribute__((packed));
> > +
> >  #endif /* _UAPI_LIVEPATCH_H */
> > diff --git a/scripts/Makefile b/scripts/Makefile
> > index 9d442ee050bd..bf9ce74b70b0 100644
> > --- a/scripts/Makefile
> > +++ b/scripts/Makefile
> > @@ -39,6 +39,7 @@ build_unifdef: $(obj)/unifdef
> >  subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
> >  subdir-$(CONFIG_MODVERSIONS) += genksyms
> >  subdir-$(CONFIG_SECURITY_SELINUX) += selinux
> > +subdir-$(CONFIG_LIVEPATCH)   += livepatch
> >
> >  # Let clean descend into subdirs
> >  subdir-        += basic dtc gdb kconfig mod package
> > diff --git a/scripts/livepatch/.gitignore b/scripts/livepatch/.gitignore
> > new file mode 100644
> > index 000000000000..dc22fe4b6a5b
> > --- /dev/null
> > +++ b/scripts/livepatch/.gitignore
> > @@ -0,0 +1 @@
> > +klp-convert
> > diff --git a/scripts/livepatch/Makefile b/scripts/livepatch/Makefile
> > new file mode 100644
> > index 000000000000..2842ecdba3fd
> > --- /dev/null
> > +++ b/scripts/livepatch/Makefile
> > @@ -0,0 +1,7 @@
> > +hostprogs-y                    := klp-convert
> > +always                         := $(hostprogs-y)
> > +
> > +klp-convert-objs               := klp-convert.o elf.o
> > +
> > +HOST_EXTRACFLAGS               := -g -I$(INSTALL_HDR_PATH)/include -Wall
>
> This looks strange.
>
> Theoretically, you cannot include headers in $(INSTALL_HDR_PATH)/include
> from host programs.
>
> headers_install works for the target architecture, not host architecture.
> This may cause a strange result when you are cross-compiling the kernel.
>
> BTW, which header in $(INSTALL_HDR_PATH)/include do you need to include ?
>
>
> Also, -Wall is redundant because it is set by the top-level Makefile.


I deleted HOST_EXTRACFLAGS entirely,
and I was still able to build klp-convert.


What is the purpose of '-g' ?
If it is only needed for local debugging,
it should be removed from the upstream code, in my opinion.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 03/10] livepatch: Add klp-convert tool

[Joe Lawrence]

On Wed, Jul 31, 2019 at 12:36:05PM +0900, Masahiro Yamada wrote:
> On Wed, Jul 31, 2019 at 11:50 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >
> > > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > >
> > > Livepatches may use symbols which are not contained in its own scope,
> > > and, because of that, may end up compiled with relocations that will
> > > only be resolved during module load. Yet, when the referenced symbols
> > > are not exported, solving this relocation requires information on the
> > > object that holds the symbol (either vmlinux or modules) and its
> > > position inside the object, as an object may contain multiple symbols
> > > with the same name. Providing such information must be done
> > > accordingly to what is specified in
> > > Documentation/livepatch/module-elf-format.txt.
> > >
> > > Currently, there is no trivial way to embed the required information
> > > as requested in the final livepatch elf object. klp-convert solves
> > > this problem in two different forms: (i) by relying on Symbols.list,
> > > which is built during kernel compilation, to automatically infer the
> > > relocation targeted symbol, and, when such inference is not possible
> > > (ii) by using annotations in the elf object to convert the relocation
> > > accordingly to the specification, enabling it to be handled by the
> > > livepatch loader.
> > >
> > > Given the above, create scripts/livepatch to hold tools developed for
> > > livepatches and add source files for klp-convert there.
> > >
> > > The core file of klp-convert is scripts/livepatch/klp-convert.c, which
> > > implements the heuristics used to solve the relocations and the
> > > conversion of unresolved symbols into the expected format, as defined
> > > in [1].
> > >
> > > klp-convert receives as arguments the Symbols.list file, an input
> > > livepatch module to be converted and the output name for the converted
> > > livepatch. When it starts running, klp-convert parses Symbols.list and
> > > builds two internal lists of symbols, one containing the exported and
> > > another containing the non-exported symbols. Then, by parsing the rela
> > > sections in the elf object, klp-convert identifies which symbols must
> > > be converted, which are those unresolved and that do not have a
> > > corresponding exported symbol, and attempts to convert them
> > > accordingly to the specification.
> > >
> > > By using Symbols.list, klp-convert identifies which symbols have names
> > > that only appear in a single kernel object, thus being capable of
> > > resolving these cases without the intervention of the developer. When
> > > various homonymous symbols exist through kernel objects, it is not
> > > possible to infer the right one, thus klp-convert falls back into
> > > using developer annotations. If these were not provided, then the tool
> > > will print a list with all acceptable targets for the symbol being
> > > processed.
> > >
> > > Annotations in the context of klp-convert are accessible as struct
> > > klp_module_reloc entries in sections named
> > > .klp.module_relocs.<objname>. These entries are pairs of symbol
> > > references and positions which are to be resolved against definitions
> > > in <objname>.
> > >
> > > Define the structure klp_module_reloc in
> > > include/linux/uapi/livepatch.h allowing developers to annotate the
> > > livepatch source code with it.
> > >
> > > klp-convert relies on libelf and on a list implementation. Add files
> > > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a
> > > libelf interfacing layer and scripts/livepatch/list.h, which is a
> > > list implementation.
> > >
> > > Update Makefiles to correctly support the compilation of the new tool,
> > > update MAINTAINERS file and add a .gitignore file.
> > >
> > > [1] - Documentation/livepatch/module-elf-format.txt
> > >
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> > > Signed-off-by: Joao Moreira <jmoreira@suse.de>
> > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > > ---
> > >  MAINTAINERS                     |   1 +
> > >  include/uapi/linux/livepatch.h  |   5 +
> > >  scripts/Makefile                |   1 +
> > >  scripts/livepatch/.gitignore    |   1 +
> > >  scripts/livepatch/Makefile      |   7 +
> > >  scripts/livepatch/elf.c         | 753 ++++++++++++++++++++++++++++++++
> > >  scripts/livepatch/elf.h         |  73 ++++
> > >  scripts/livepatch/klp-convert.c | 713 ++++++++++++++++++++++++++++++
> > >  scripts/livepatch/klp-convert.h |  39 ++
> > >  scripts/livepatch/list.h        | 391 +++++++++++++++++
> > >  10 files changed, 1984 insertions(+)
> > >  create mode 100644 scripts/livepatch/.gitignore
> > >  create mode 100644 scripts/livepatch/Makefile
> > >  create mode 100644 scripts/livepatch/elf.c
> > >  create mode 100644 scripts/livepatch/elf.h
> > >  create mode 100644 scripts/livepatch/klp-convert.c
> > >  create mode 100644 scripts/livepatch/klp-convert.h
> > >  create mode 100644 scripts/livepatch/list.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 52842fa37261..c1587e1cc385 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9022,6 +9022,7 @@ F:        arch/x86/kernel/livepatch.c
> > >  F:     Documentation/livepatch/
> > >  F:     Documentation/ABI/testing/sysfs-kernel-livepatch
> > >  F:     samples/livepatch/
> > > +F:     scripts/livepatch/
> > >  F:     tools/testing/selftests/livepatch/
> > >  L:     live-patching@vger.kernel.org
> > >  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
> > > diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
> > > index e19430918a07..1c364d42d38e 100644
> > > --- a/include/uapi/linux/livepatch.h
> > > +++ b/include/uapi/linux/livepatch.h
> > > @@ -12,4 +12,9 @@
> > >  #define KLP_RELA_PREFIX                ".klp.rela."
> > >  #define KLP_SYM_PREFIX         ".klp.sym."
> > >
> > > +struct klp_module_reloc {
> > > +       void *sym;
> > > +       unsigned int sympos;
> > > +} __attribute__((packed));
> > > +
> > >  #endif /* _UAPI_LIVEPATCH_H */
> > > diff --git a/scripts/Makefile b/scripts/Makefile
> > > index 9d442ee050bd..bf9ce74b70b0 100644
> > > --- a/scripts/Makefile
> > > +++ b/scripts/Makefile
> > > @@ -39,6 +39,7 @@ build_unifdef: $(obj)/unifdef
> > >  subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
> > >  subdir-$(CONFIG_MODVERSIONS) += genksyms
> > >  subdir-$(CONFIG_SECURITY_SELINUX) += selinux
> > > +subdir-$(CONFIG_LIVEPATCH)   += livepatch
> > >
> > >  # Let clean descend into subdirs
> > >  subdir-        += basic dtc gdb kconfig mod package
> > > diff --git a/scripts/livepatch/.gitignore b/scripts/livepatch/.gitignore
> > > new file mode 100644
> > > index 000000000000..dc22fe4b6a5b
> > > --- /dev/null
> > > +++ b/scripts/livepatch/.gitignore
> > > @@ -0,0 +1 @@
> > > +klp-convert
> > > diff --git a/scripts/livepatch/Makefile b/scripts/livepatch/Makefile
> > > new file mode 100644
> > > index 000000000000..2842ecdba3fd
> > > --- /dev/null
> > > +++ b/scripts/livepatch/Makefile
> > > @@ -0,0 +1,7 @@
> > > +hostprogs-y                    := klp-convert
> > > +always                         := $(hostprogs-y)
> > > +
> > > +klp-convert-objs               := klp-convert.o elf.o
> > > +
> > > +HOST_EXTRACFLAGS               := -g -I$(INSTALL_HDR_PATH)/include -Wall
> >
> > This looks strange.
> >
> > Theoretically, you cannot include headers in $(INSTALL_HDR_PATH)/include
> > from host programs.
> >
> > headers_install works for the target architecture, not host architecture.
> > This may cause a strange result when you are cross-compiling the kernel.
> >
> > BTW, which header in $(INSTALL_HDR_PATH)/include do you need to include ?
> >
> >
> > Also, -Wall is redundant because it is set by the top-level Makefile.
> 
> 
> I deleted HOST_EXTRACFLAGS entirely,
> and I was still able to build klp-convert.
> 
> 
> What is the purpose of '-g' ?
> If it is only needed for local debugging,
> it should be removed from the upstream code, in my opinion.
> 

HOST_EXTRACFLAGS looks like it was present in the patchset from the
early RFC days and inherited through each revision.

These are the files that the klp-convert code includes, mostly typical C
usercode headers like stdio.h and a few local headers like elf.h:

  % grep -h '^#include' scripts/livepatch/*.{c,h} | sort -u
  #include "elf.h"
  #include <fcntl.h>
  #include <gelf.h>
  #include "klp-convert.h"
  #include "list.h"
  #include <stdbool.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <sys/stat.h>
  #include <sys/types.h>
  #include <unistd.h>

If HOST_EXTRACFLAGS is really unneeded, we can easily drop it in the
next patchset version.

I haven't tried cross-compiling yet, but that is a good thing to note
for future testing.

Thanks,

-- Joe

Re: [PATCH v4 03/10] livepatch: Add klp-convert tool

[Masahiro Yamada]

On Sat, Aug 10, 2019 at 3:42 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:

> > > > diff --git a/scripts/livepatch/Makefile b/scripts/livepatch/Makefile
> > > > new file mode 100644
> > > > index 000000000000..2842ecdba3fd
> > > > --- /dev/null
> > > > +++ b/scripts/livepatch/Makefile
> > > > @@ -0,0 +1,7 @@
> > > > +hostprogs-y                    := klp-convert
> > > > +always                         := $(hostprogs-y)
> > > > +
> > > > +klp-convert-objs               := klp-convert.o elf.o
> > > > +
> > > > +HOST_EXTRACFLAGS               := -g -I$(INSTALL_HDR_PATH)/include -Wall
> > >
> > > This looks strange.
> > >
> > > Theoretically, you cannot include headers in $(INSTALL_HDR_PATH)/include
> > > from host programs.
> > >
> > > headers_install works for the target architecture, not host architecture.
> > > This may cause a strange result when you are cross-compiling the kernel.
> > >
> > > BTW, which header in $(INSTALL_HDR_PATH)/include do you need to include ?
> > >
> > >
> > > Also, -Wall is redundant because it is set by the top-level Makefile.
> >
> >
> > I deleted HOST_EXTRACFLAGS entirely,
> > and I was still able to build klp-convert.
> >
> >
> > What is the purpose of '-g' ?
> > If it is only needed for local debugging,
> > it should be removed from the upstream code, in my opinion.
> >
>
> HOST_EXTRACFLAGS looks like it was present in the patchset from the
> early RFC days and inherited through each revision.
>
> These are the files that the klp-convert code includes, mostly typical C
> usercode headers like stdio.h and a few local headers like elf.h:
>
>   % grep -h '^#include' scripts/livepatch/*.{c,h} | sort -u
>   #include "elf.h"
>   #include <fcntl.h>
>   #include <gelf.h>
>   #include "klp-convert.h"
>   #include "list.h"
>   #include <stdbool.h>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
>   #include <sys/stat.h>
>   #include <sys/types.h>
>   #include <unistd.h>
>
> If HOST_EXTRACFLAGS is really unneeded, we can easily drop it in the
> next patchset version.

Yes, please do so.

Thanks.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Masahiro Yamada]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

Hi Joe,


On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> From: Miroslav Benes <mbenes@suse.cz>
>
> Currently, livepatch infrastructure in the kernel relies on
> MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
> kernel module loader knows a module is indeed livepatch module and can
> behave accordingly.
>
> klp-convert, on the other hand relies on LIVEPATCH_* statement in the
> module's Makefile for exactly the same reason.
>
> Remove dependency on modinfo and generate MODULE_INFO flag
> automatically in modpost when LIVEPATCH_* is defined in the module's
> Makefile. Generate a list of all built livepatch modules based on
> the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
> this list as an argument for modpost which will use it to identify
> livepatch modules.
>
> As MODULE_INFO is no longer needed, remove it.


I do not understand this patch.
This makes the implementation so complicated.

I think MODULE_INFO(livepatch, "Y") is cleaner than
LIVEPATCH_* in Makefile.


How about this approach?


[1] Make modpost generate the list of livepatch modules.
    (livepatch-modules)

[2] Generate Symbols.list in scripts/Makefile.modpost
    (vmlinux + modules excluding livepatch-modules)

[3] Run klp-convert for modules in livepatch-modules.


If you do this, you can remove most of the build system hacks
can't you?


I attached an example implementation for [1].

Please check whether this works.

Thanks.



-- 
Best Regards
Masahiro Yamada

[-- Attachment #2: 0001-livepatch-make-modpost-generate-the-list-of-livepatc.patch --]
[-- Type: text/x-patch, Size: 3983 bytes --]

From 85571430aa12cd19a75cbc856da1092199876e6a Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <yamada.masahiro@socionext.com>
Date: Wed, 31 Jul 2019 14:51:29 +0900
Subject: [PATCH] livepatch: make modpost generate the list of livepatch
 modules

Reverse the livepatch-modules direction.

The modpost generates the livepatch-modules file instead of
Makefile feeding it to modpost.

The implementation just mimics write_dump().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 scripts/Makefile.modpost |  3 ++-
 scripts/mod/modpost.c    | 28 ++++++++++++++++++++++++++--
 scripts/mod/modpost.h    |  1 +
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 92ed02d7cd5e..c884b7b709ca 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -56,7 +56,8 @@ MODPOST = scripts/mod/modpost						\
 	$(if $(KBUILD_EXTMOD),$(addprefix -e ,$(KBUILD_EXTRA_SYMBOLS)))	\
 	$(if $(KBUILD_EXTMOD),-o $(modulesymfile))			\
 	$(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)			\
-	$(if $(KBUILD_MODPOST_WARN),-w)
+	$(if $(KBUILD_MODPOST_WARN),-w)					\
+	$(if $(CONFIG_LIVEPATCH),-l livepatch-modules)
 
 ifdef MODPOST_VMLINUX
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3e6d36ddfcdf..e3f637f225e4 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1976,6 +1976,10 @@ static void read_symbols(const char *modname)
 		license = get_next_modinfo(&info, "license", license);
 	}
 
+	/* Livepatch modules have unresolved symbols resolved by klp-convert */
+	if (get_modinfo(&info, "livepatch"))
+		mod->livepatch = 1;
+
 	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
 		symname = remove_dot(info.strtab + sym->st_name);
 
@@ -2118,7 +2122,7 @@ static int check_exports(struct module *mod)
 		const char *basename;
 		exp = find_symbol(s->name);
 		if (!exp || exp->module == mod) {
-			if (have_vmlinux && !s->weak) {
+			if (have_vmlinux && !s->weak && !mod->livepatch) {
 				if (warn_unresolved) {
 					warn("\"%s\" [%s.ko] undefined!\n",
 					     s->name, mod->name);
@@ -2429,6 +2433,20 @@ static void write_dump(const char *fname)
 	free(buf.p);
 }
 
+static void write_livepatch_modules(const char *fname)
+{
+	struct buffer buf = { };
+	struct module *mod;
+
+	for (mod = modules; mod; mod = mod->next) {
+		if (mod->livepatch)
+			buf_printf(&buf, "%s\n", mod->name);
+	}
+
+	write_if_changed(&buf, fname);
+	free(buf.p);
+}
+
 struct ext_sym_list {
 	struct ext_sym_list *next;
 	const char *file;
@@ -2440,13 +2458,14 @@ int main(int argc, char **argv)
 	struct buffer buf = { };
 	char *kernel_read = NULL, *module_read = NULL;
 	char *dump_write = NULL, *files_source = NULL;
+	char *livepatch_modules = NULL;
 	int opt;
 	int err;
 	int n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;
 
-	while ((opt = getopt(argc, argv, "i:I:e:mnsT:o:awE")) != -1) {
+	while ((opt = getopt(argc, argv, "i:I:e:l:mnsT:o:awE")) != -1) {
 		switch (opt) {
 		case 'i':
 			kernel_read = optarg;
@@ -2463,6 +2482,9 @@ int main(int argc, char **argv)
 			extsym_iter->file = optarg;
 			extsym_start = extsym_iter;
 			break;
+		case 'l':
+			livepatch_modules = optarg;
+			break;
 		case 'm':
 			modversions = 1;
 			break;
@@ -2535,6 +2557,8 @@ int main(int argc, char **argv)
 	}
 	if (dump_write)
 		write_dump(dump_write);
+	if (livepatch_modules)
+		write_livepatch_modules(livepatch_modules);
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 8453d6ac2f77..2acfaae064ec 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -118,6 +118,7 @@ struct module {
 	int skip;
 	int has_init;
 	int has_cleanup;
+	int livepatch;
 	struct buffer dev_table_buf;
 	char	     srcversion[25];
 	int is_dot_o;
-- 
2.17.1

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Joe Lawrence]

On Wed, Jul 31, 2019 at 02:58:27PM +0900, Masahiro Yamada wrote:
> Hi Joe,
> 
> 
> On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > From: Miroslav Benes <mbenes@suse.cz>
> >
> > Currently, livepatch infrastructure in the kernel relies on
> > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
> > kernel module loader knows a module is indeed livepatch module and can
> > behave accordingly.
> >
> > klp-convert, on the other hand relies on LIVEPATCH_* statement in the
> > module's Makefile for exactly the same reason.
> >
> > Remove dependency on modinfo and generate MODULE_INFO flag
> > automatically in modpost when LIVEPATCH_* is defined in the module's
> > Makefile. Generate a list of all built livepatch modules based on
> > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
> > this list as an argument for modpost which will use it to identify
> > livepatch modules.
> >
> > As MODULE_INFO is no longer needed, remove it.
> 
> 
> I do not understand this patch.
> This makes the implementation so complicated.
> 
> I think MODULE_INFO(livepatch, "Y") is cleaner than
> LIVEPATCH_* in Makefile.
> 
> 
> How about this approach?
> 
> 
> [1] Make modpost generate the list of livepatch modules.
>     (livepatch-modules)
> 
> [2] Generate Symbols.list in scripts/Makefile.modpost
>     (vmlinux + modules excluding livepatch-modules)
> 
> [3] Run klp-convert for modules in livepatch-modules.
> 
> 
> If you do this, you can remove most of the build system hacks
> can't you?
> 
> 
> I attached an example implementation for [1].
> 
> Please check whether this works.
> 

Hi Masahiro, 

I tested and step [1] that you attached did create the livepatch-modules
as expected.  Thanks for that example, it does look cleaner that what
we had in the patchset.

I'm admittedly out of my element with kbuild changes, but here are my
naive attempts at steps [2] and [3]...


[step 2] generate Symbols.list - I tacked this on as a dependency of the
$(modules:.ko=.mod.o), but there is probably a better more logical place
to put it.  Also used grep -Fxv to exclude the livepatch-modules list
from the modules.order list of modules to process.

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

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 3eca7fccadd4..5409bbc212bb 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -111,7 +111,23 @@ quiet_cmd_cc_o_c = CC      $@
       cmd_cc_o_c = $(CC) $(c_flags) $(KBUILD_CFLAGS_MODULE) $(CFLAGS_MODULE) \
 		   -c -o $@ $<
 
-$(modules:.ko=.mod.o): %.mod.o: %.mod.c FORCE
+quiet_cmd_klp_map = KLP     Symbols.list
+SLIST = $(objtree)/Symbols.list
+
+define cmd_symbols_list
+	$(shell echo "klp-convert-symbol-data.0.1" > $(objtree)/Symbols.list)			\
+	$(shell echo "*vmlinux" >> $(objtree)/Symbols.list)					\
+	$(shell nm -f posix $(objtree)/vmlinux | cut -d\  -f1 >> $(objtree)/Symbols.list)	\
+	$(foreach ko, $(sort $(shell grep -Fxv -f livepatch-modules modules.order)),		\
+		$(shell echo "*$(shell basename -s .ko $(ko))" >> $(objtree)/Symbols.list)	\
+		$(shell nm -f posix $(patsubst %.ko,%.o,$(ko)) | cut -d\  -f1 >> $(objtree)/Symbols.list))
+endef
+
+Symbols.list: __modpost
+	$(if $(CONFIG_LIVEPATCH), $(call cmd,symbols_list))
+
+
+$(modules:.ko=.mod.o): %.mod.o: %.mod.c Symbols.list FORCE
 	$(call if_changed_dep,cc_o_c)
 
 targets += $(modules:.ko=.mod.o)
-- 
2.18.1

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



[step 3] klp-convert the livepatch-modules - more or less what existed
in the patchset already, however used the grep -Fx trick to process only
modules found in livepatch-modules file:

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

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 73e80b917f12..f085644c2b97 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -223,6 +223,8 @@ endif
 # (needed for the shell)
 make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,$$$$,$(cmd_$(1)))))
 
+save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
+
 # Find any prerequisites that is newer than target or that does not exist.
 # PHONY targets skipped in both cases.
 any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^)
@@ -230,7 +232,7 @@ any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^)
 # Execute command if command has changed or prerequisite(s) are updated.
 if_changed = $(if $(any-prereq)$(cmd-check),                                 \
 	$(cmd);                                                              \
-	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
+	$(save-cmd), @:)
 
 # Execute the command and also postprocess generated .d dependencies file.
 if_changed_dep = $(if $(any-prereq)$(cmd-check),$(cmd_and_fixdep),@:)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 5409bbc212bb..bc3b7b9dd8fa 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -142,8 +142,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
                  -o $@ $(real-prereqs) ;                                \
 	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
 
+SLIST = $(objtree)/Symbols.list
+KLP_CONVERT = scripts/livepatch/klp-convert
+quiet_cmd_klp_convert = KLP     $@
+      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);				\
+			$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
+
+define rule_ld_ko_o
+	$(Q)$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
+	$(call save-cmd,ld_ko_o) ;						\
+	$(if $(CONFIG_LIVEPATCH),						\
+		$(if $(shell grep -Fx "$@" livepatch-modules),			\
+			$(call echo-cmd,klp_convert) $(cmd_klp_convert)))
+endef
+
 $(modules): %.ko :%.o %.mod.o FORCE
-	+$(call if_changed,ld_ko_o)
+	+$(call if_changed_rule,ld_ko_o)
 
 targets += $(modules)
 
-- 
2.18.1

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


Thanks,

-- Joe

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Masahiro Yamada]

Hi Joe,

On Tue, Aug 13, 2019 at 12:56 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On Wed, Jul 31, 2019 at 02:58:27PM +0900, Masahiro Yamada wrote:
> > Hi Joe,
> >
> >
> > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >
> > > From: Miroslav Benes <mbenes@suse.cz>
> > >
> > > Currently, livepatch infrastructure in the kernel relies on
> > > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
> > > kernel module loader knows a module is indeed livepatch module and can
> > > behave accordingly.
> > >
> > > klp-convert, on the other hand relies on LIVEPATCH_* statement in the
> > > module's Makefile for exactly the same reason.
> > >
> > > Remove dependency on modinfo and generate MODULE_INFO flag
> > > automatically in modpost when LIVEPATCH_* is defined in the module's
> > > Makefile. Generate a list of all built livepatch modules based on
> > > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
> > > this list as an argument for modpost which will use it to identify
> > > livepatch modules.
> > >
> > > As MODULE_INFO is no longer needed, remove it.
> >
> >
> > I do not understand this patch.
> > This makes the implementation so complicated.
> >
> > I think MODULE_INFO(livepatch, "Y") is cleaner than
> > LIVEPATCH_* in Makefile.
> >
> >
> > How about this approach?
> >
> >
> > [1] Make modpost generate the list of livepatch modules.
> >     (livepatch-modules)
> >
> > [2] Generate Symbols.list in scripts/Makefile.modpost
> >     (vmlinux + modules excluding livepatch-modules)
> >
> > [3] Run klp-convert for modules in livepatch-modules.
> >
> >
> > If you do this, you can remove most of the build system hacks
> > can't you?
> >
> >
> > I attached an example implementation for [1].
> >
> > Please check whether this works.
> >
>
> Hi Masahiro,
>
> I tested and step [1] that you attached did create the livepatch-modules
> as expected.  Thanks for that example, it does look cleaner that what
> we had in the patchset.
>
> I'm admittedly out of my element with kbuild changes, but here are my
> naive attempts at steps [2] and [3]...
>
>
> [step 2] generate Symbols.list - I tacked this on as a dependency of the
> $(modules:.ko=.mod.o), but there is probably a better more logical place
> to put it.  Also used grep -Fxv to exclude the livepatch-modules list
> from the modules.order list of modules to process.
>
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 3eca7fccadd4..5409bbc212bb 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -111,7 +111,23 @@ quiet_cmd_cc_o_c = CC      $@
>        cmd_cc_o_c = $(CC) $(c_flags) $(KBUILD_CFLAGS_MODULE) $(CFLAGS_MODULE) \
>                    -c -o $@ $<
>
> -$(modules:.ko=.mod.o): %.mod.o: %.mod.c FORCE
> +quiet_cmd_klp_map = KLP     Symbols.list
> +SLIST = $(objtree)/Symbols.list
> +
> +define cmd_symbols_list
> +       $(shell echo "klp-convert-symbol-data.0.1" > $(objtree)/Symbols.list)                   \
> +       $(shell echo "*vmlinux" >> $(objtree)/Symbols.list)                                     \
> +       $(shell nm -f posix $(objtree)/vmlinux | cut -d\  -f1 >> $(objtree)/Symbols.list)       \
> +       $(foreach ko, $(sort $(shell grep -Fxv -f livepatch-modules modules.order)),            \
> +               $(shell echo "*$(shell basename -s .ko $(ko))" >> $(objtree)/Symbols.list)      \
> +               $(shell nm -f posix $(patsubst %.ko,%.o,$(ko)) | cut -d\  -f1 >> $(objtree)/Symbols.list))
> +endef


All the $(shell ...) calls are pointless.


     $(shell echo "hello" > Symbols.list)

is equivalent to

     echo "hello" > Symbols.list


> +
> +Symbols.list: __modpost
> +       $(if $(CONFIG_LIVEPATCH), $(call cmd,symbols_list))
> +
> +
> +$(modules:.ko=.mod.o): %.mod.o: %.mod.c Symbols.list FORCE
>         $(call if_changed_dep,cc_o_c)
>
>  targets += $(modules:.ko=.mod.o)
> --
> 2.18.1
>
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>
>
>
> [step 3] klp-convert the livepatch-modules - more or less what existed
> in the patchset already, however used the grep -Fx trick to process only
> modules found in livepatch-modules file:
>
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index 73e80b917f12..f085644c2b97 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -223,6 +223,8 @@ endif
>  # (needed for the shell)
>  make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,$$$$,$(cmd_$(1)))))
>
> +save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
> +
>  # Find any prerequisites that is newer than target or that does not exist.
>  # PHONY targets skipped in both cases.
>  any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^)
> @@ -230,7 +232,7 @@ any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^)
>  # Execute command if command has changed or prerequisite(s) are updated.
>  if_changed = $(if $(any-prereq)$(cmd-check),                                 \
>         $(cmd);                                                              \
> -       printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
> +       $(save-cmd), @:)
>
>  # Execute the command and also postprocess generated .d dependencies file.
>  if_changed_dep = $(if $(any-prereq)$(cmd-check),$(cmd_and_fixdep),@:)
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 5409bbc212bb..bc3b7b9dd8fa 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -142,8 +142,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>                   -o $@ $(real-prereqs) ;                                \
>         $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>
> +SLIST = $(objtree)/Symbols.list
> +KLP_CONVERT = scripts/livepatch/klp-convert
> +quiet_cmd_klp_convert = KLP     $@
> +      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);                         \
> +                       $(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
> +
> +define rule_ld_ko_o
> +       $(Q)$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ;                           \
> +       $(call save-cmd,ld_ko_o) ;                                              \
> +       $(if $(CONFIG_LIVEPATCH),                                               \
> +               $(if $(shell grep -Fx "$@" livepatch-modules),                  \
> +                       $(call echo-cmd,klp_convert) $(cmd_klp_convert)))
> +endef

This does not correctly detect the command change of cmd_klp_convert.


I cleaned up the build system, and pushed it based on my
kbuild tree.

Please see:

git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
klp-cleanup


Thanks.


> +
>  $(modules): %.ko :%.o %.mod.o FORCE
> -       +$(call if_changed,ld_ko_o)
> +       +$(call if_changed_rule,ld_ko_o)
>
>  targets += $(modules)
>
> --
> 2.18.1
>
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>
>
> Thanks,
>
> -- Joe
--
Best Regards
Masahiro Yamada

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Miroslav Benes]

Hi,

> I cleaned up the build system, and pushed it based on my
> kbuild tree.
> 
> Please see:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
> klp-cleanup

This indeed looks much simpler and cleaner (as far as I can judge with my 
limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, 
"Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and 
work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy 
module which is then livepatched by lib/livepatch/test_klp_convert1.c).

Thanks a lot!

Miroslav

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Joe Lawrence]

On 8/16/19 4:19 AM, Miroslav Benes wrote:
> Hi,
> 
>> I cleaned up the build system, and pushed it based on my
>> kbuild tree.
>>
>> Please see:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
>> klp-cleanup
> 
> This indeed looks much simpler and cleaner (as far as I can judge with my
> limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch,
> "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and
> work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy
> module which is then livepatched by lib/livepatch/test_klp_convert1.c).
> 

Yeah, Masahiro this is great, thanks for reworking this!

I did tweak one module like Miroslav mentioned and I think a few of the 
newly generated files need to be cleaned up as part of "make clean", but 
all said, this is a nice improvement.

Are you targeting the next merge window for your kbuild branch?  (This 
appears to be what the klp-cleanup branch was based on.)

Thanks,

-- Joe

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Joe Lawrence]

On 8/16/19 8:43 AM, Joe Lawrence wrote:
> On 8/16/19 4:19 AM, Miroslav Benes wrote:
>> Hi,
>>
>>> I cleaned up the build system, and pushed it based on my
>>> kbuild tree.
>>>
>>> Please see:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
>>> klp-cleanup
>>
>> This indeed looks much simpler and cleaner (as far as I can judge with my
>> limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch,
>> "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and
>> work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy
>> module which is then livepatched by lib/livepatch/test_klp_convert1.c).
>>
> 
> Yeah, Masahiro this is great, thanks for reworking this!
> 
> I did tweak one module like Miroslav mentioned and I think a few of the
> newly generated files need to be cleaned up as part of "make clean", but
> all said, this is a nice improvement.
> 

Well actually, now I see this comment in the top-level Makefile:

# Cleaning is done on three levels. 

# make clean     Delete most generated files 

#                Leave enough to build external modules 

# make mrproper  Delete the current configuration, and all generated 
files
# make distclean Remove editor backup files, patch leftover files and 
the like

I didn't realize that we're supposed to be able to still build external 
modules after "make clean".  If that's the case, then one might want to 
build an external klp-module after doing that.

With that in mind, shouldn't Symbols.list to persist until mrproper? 
And I think modules-livepatch could go away during clean, what do you think?

-- Joe

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Masahiro Yamada]

Hi Joe,

On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 8/16/19 8:43 AM, Joe Lawrence wrote:
> > On 8/16/19 4:19 AM, Miroslav Benes wrote:
> >> Hi,
> >>
> >>> I cleaned up the build system, and pushed it based on my
> >>> kbuild tree.
> >>>
> >>> Please see:
> >>>
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
> >>> klp-cleanup
> >>
> >> This indeed looks much simpler and cleaner (as far as I can judge with my
> >> limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch,
> >> "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and
> >> work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy
> >> module which is then livepatched by lib/livepatch/test_klp_convert1.c).
> >>
> >
> > Yeah, Masahiro this is great, thanks for reworking this!
> >
> > I did tweak one module like Miroslav mentioned and I think a few of the
> > newly generated files need to be cleaned up as part of "make clean", but
> > all said, this is a nice improvement.
> >
>
> Well actually, now I see this comment in the top-level Makefile:
>
> # Cleaning is done on three levels.
>
> # make clean     Delete most generated files
>
> #                Leave enough to build external modules
>
> # make mrproper  Delete the current configuration, and all generated
> files
> # make distclean Remove editor backup files, patch leftover files and
> the like
>
> I didn't realize that we're supposed to be able to still build external
> modules after "make clean".  If that's the case, then one might want to
> build an external klp-module after doing that.

Yes. 'make clean' must keep all the build artifacts
needed for building external modules.


> With that in mind, shouldn't Symbols.list to persist until mrproper?
> And I think modules-livepatch could go away during clean, what do you think?
>
> -- Joe


Symbols.list should be kept by the time mrproper is run.
So, please add it to MRROPER_FILES instead of CLEAN_FILES.

modules.livepatch is a temporary file, so you can add it to
CLEAN_FILES.

How is this feature supposed to work for external modules?

klp-convert receives:
"symbols from vmlinux" + "symbols from no-klp in-tree modules"
+ "symbols from no-klp external modules" ??



BTW, 'Symbols.list' sounds like a file to list out symbols
for generic purposes, but in fact, the
file format is very specific for the klp-convert tool.
Perhaps, is it better to rename it so it infers
this is for livepatching? What do you think?


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Joe Lawrence]

On 8/18/19 11:50 PM, Masahiro Yamada wrote:
> Hi Joe,
> 
> On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>
>>
>> I didn't realize that we're supposed to be able to still build external
>> modules after "make clean".  If that's the case, then one might want to
>> build an external klp-module after doing that.
> 
> Yes. 'make clean' must keep all the build artifacts
> needed for building external modules.
> 
> 
>> With that in mind, shouldn't Symbols.list to persist until mrproper?
>> And I think modules-livepatch could go away during clean, what do you think?
>>
>> -- Joe
> 
> 
> Symbols.list should be kept by the time mrproper is run.
> So, please add it to MRROPER_FILES instead of CLEAN_FILES.
> 
> modules.livepatch is a temporary file, so you can add it to
> CLEAN_FILES.
> 

OK, I'll add those to their respective lists.

> How is this feature supposed to work for external modules?
> 
> klp-convert receives:
> "symbols from vmlinux" + "symbols from no-klp in-tree modules"
> + "symbols from no-klp external modules" ??
> 

I don't think that this use-case has been previously thought out 
(Miroslav, correct me if I'm wrong here.)

I did just run an external build of a copy of 
samples/livepatch/livepatch-annotated-sample.c:

  - modules.livepatch is generated in external dir
  - klp-convert is invoked for the livepatch module
  - the external livepatch module successfully loads

But that was only testing external livepatch modules.

I don't know if we need/want to support general external modules 
supplementing Symbols.list, at least for the initial klp-convert commit. 
  I suppose external livepatch modules would then need to specify 
additional Symbols.list(s) files somehow as well.

> 
> BTW, 'Symbols.list' sounds like a file to list out symbols
> for generic purposes, but in fact, the
> file format is very specific for the klp-convert tool.
> Perhaps, is it better to rename it so it infers
> this is for livepatching? What do you think?
> 

I don't know if the "Symbols.list" name and leading uppercase was based 
on any convention, but something like symbols.klp would be fine with me.

Thanks,

-- Joe

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Miroslav Benes]

> > How is this feature supposed to work for external modules?
> > 
> > klp-convert receives:
> > "symbols from vmlinux" + "symbols from no-klp in-tree modules"
> > + "symbols from no-klp external modules" ??
> > 
> 
> I don't think that this use-case has been previously thought out (Miroslav,
> correct me if I'm wrong here.)
> 
> I did just run an external build of a copy of
> samples/livepatch/livepatch-annotated-sample.c:
> 
>  - modules.livepatch is generated in external dir
>  - klp-convert is invoked for the livepatch module
>  - the external livepatch module successfully loads
> 
> But that was only testing external livepatch modules.
> 
> I don't know if we need/want to support general external modules supplementing
> Symbols.list, at least for the initial klp-convert commit.  I suppose external
> livepatch modules would then need to specify additional Symbols.list(s) files
> somehow as well.

I think we discussed it briefly and decided to postpone it for later 
improvements. External modules are not so important in my opinion.
 
> > 
> > BTW, 'Symbols.list' sounds like a file to list out symbols
> > for generic purposes, but in fact, the
> > file format is very specific for the klp-convert tool.
> > Perhaps, is it better to rename it so it infers
> > this is for livepatching? What do you think?
> > 
> 
> I don't know if the "Symbols.list" name and leading uppercase was based on any
> convention, but something like symbols.klp would be fine with me.

symbols.klp looks ok

Miroslav

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Masahiro Yamada]

On Fri, Aug 16, 2019 at 9:43 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 8/16/19 4:19 AM, Miroslav Benes wrote:
> > Hi,
> >
> >> I cleaned up the build system, and pushed it based on my
> >> kbuild tree.
> >>
> >> Please see:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
> >> klp-cleanup
> >
> > This indeed looks much simpler and cleaner (as far as I can judge with my
> > limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch,
> > "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and
> > work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy
> > module which is then livepatched by lib/livepatch/test_klp_convert1.c).
> >
>
> Yeah, Masahiro this is great, thanks for reworking this!
>
> I did tweak one module like Miroslav mentioned and I think a few of the
> newly generated files need to be cleaned up as part of "make clean", but
> all said, this is a nice improvement.
>
> Are you targeting the next merge window for your kbuild branch?  (This
> appears to be what the klp-cleanup branch was based on.)


I can review this series from the build system point of view,
but I am not familiar enough with live-patching itself.

Some possibilities:

[1] Merge this series thru the live-patch tree after the
    kbuild base patches land.
    This requires one extra development cycle (targeting for 5.5-rc1)
    but I think this is the official way if you do not rush into it.

[2] Create an immutable branch in kbuild tree, and the live-patch
    tree will use it as the basis for queuing this series.
    We will have to coordinate the pull request order, but
    we can merge this feature for 5.4-rc1

[3] Apply this series to my kbuild tree, with proper Acked-by
    from the livepatch maintainers.
    I am a little bit uncomfortable with applying patches I
    do not understand, though...


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Miroslav Benes]

On Mon, 19 Aug 2019, Masahiro Yamada wrote:

> On Fri, Aug 16, 2019 at 9:43 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > On 8/16/19 4:19 AM, Miroslav Benes wrote:
> > > Hi,
> > >
> > >> I cleaned up the build system, and pushed it based on my
> > >> kbuild tree.
> > >>
> > >> Please see:
> > >>
> > >> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
> > >> klp-cleanup
> > >
> > > This indeed looks much simpler and cleaner (as far as I can judge with my
> > > limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch,
> > > "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and
> > > work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy
> > > module which is then livepatched by lib/livepatch/test_klp_convert1.c).
> > >
> >
> > Yeah, Masahiro this is great, thanks for reworking this!
> >
> > I did tweak one module like Miroslav mentioned and I think a few of the
> > newly generated files need to be cleaned up as part of "make clean", but
> > all said, this is a nice improvement.
> >
> > Are you targeting the next merge window for your kbuild branch?  (This
> > appears to be what the klp-cleanup branch was based on.)
> 
> 
> I can review this series from the build system point of view,
> but I am not familiar enough with live-patching itself.
> 
> Some possibilities:
> 
> [1] Merge this series thru the live-patch tree after the
>     kbuild base patches land.
>     This requires one extra development cycle (targeting for 5.5-rc1)
>     but I think this is the official way if you do not rush into it.

I'd prefer this option. There is no real rush and I think we can wait one 
extra development cycle.

Joe, could you submit one more revision with all the recent changes (once 
kbuild improvements settle down), please? We should take a look at the 
whole thing one more time? What do you think?
 
> [2] Create an immutable branch in kbuild tree, and the live-patch
>     tree will use it as the basis for queuing this series.
>     We will have to coordinate the pull request order, but
>     we can merge this feature for 5.4-rc1
> 
> [3] Apply this series to my kbuild tree, with proper Acked-by
>     from the livepatch maintainers.
>     I am a little bit uncomfortable with applying patches I
>     do not understand, though...

Thanks
Miroslav

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Joe Lawrence]

On 8/19/19 3:31 AM, Miroslav Benes wrote:
> On Mon, 19 Aug 2019, Masahiro Yamada wrote:
> 
>>
>> I can review this series from the build system point of view,
>> but I am not familiar enough with live-patching itself.
>>
>> Some possibilities:
>>
>> [1] Merge this series thru the live-patch tree after the
>>      kbuild base patches land.
>>      This requires one extra development cycle (targeting for 5.5-rc1)
>>      but I think this is the official way if you do not rush into it.
> 
> I'd prefer this option. There is no real rush and I think we can wait one
> extra development cycle.

Agreed.  I'm in no hurry and was only curious about the kbuild changes 
that this patchset is now dependent on -- how to note them for other 
reviewers or anyone wishing to test.

> Joe, could you submit one more revision with all the recent changes (once
> kbuild improvements settle down), please? We should take a look at the
> whole thing one more time? What do you think?
>   

Definitely, yes.  I occasionally force a push to:
https://github.com/joe-lawrence/linux/tree/klp-convert-v5-expanded

as I've been updating and collecting feedback from v4.  Once updates 
settle, I'll send out a new v5 set.

-- Joe

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Masahiro Yamada]

Hi Joe,

On Tue, Aug 20, 2019 at 1:02 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 8/19/19 3:31 AM, Miroslav Benes wrote:
> > On Mon, 19 Aug 2019, Masahiro Yamada wrote:
> >
> >>
> >> I can review this series from the build system point of view,
> >> but I am not familiar enough with live-patching itself.
> >>
> >> Some possibilities:
> >>
> >> [1] Merge this series thru the live-patch tree after the
> >>      kbuild base patches land.
> >>      This requires one extra development cycle (targeting for 5.5-rc1)
> >>      but I think this is the official way if you do not rush into it.
> >
> > I'd prefer this option. There is no real rush and I think we can wait one
> > extra development cycle.
>
> Agreed.  I'm in no hurry and was only curious about the kbuild changes
> that this patchset is now dependent on -- how to note them for other
> reviewers or anyone wishing to test.
>
> > Joe, could you submit one more revision with all the recent changes (once
> > kbuild improvements settle down), please? We should take a look at the
> > whole thing one more time? What do you think?
> >
>
> Definitely, yes.  I occasionally force a push to:
> https://github.com/joe-lawrence/linux/tree/klp-convert-v5-expanded
>
> as I've been updating and collecting feedback from v4.  Once updates
> settle, I'll send out a new v5 set.
>
> -- Joe

When you send v5, please squash the following clean-up too:



diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
index 89eaef0d3efc..9e77246d84e3 100644
--- a/scripts/Makefile.modfinal
+++ b/scripts/Makefile.modfinal
@@ -47,7 +47,7 @@ targets += $(modules) $(modules:.ko=.mod.o)
 # Live Patch
 # ---------------------------------------------------------------------------

-$(modules-klp:.ko=.tmp.ko): %.tmp.ko: %.o %.mod.o Symbols.list FORCE
+%.tmp.ko: %.o %.mod.o Symbols.list FORCE
        +$(call if_changed,ld_ko_o)

 quiet_cmd_klp_convert = KLP     $@




Thanks.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules

[Miroslav Benes]

On Wed, 31 Jul 2019, Masahiro Yamada wrote:

> Hi Joe,
> 
> 
> On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > From: Miroslav Benes <mbenes@suse.cz>
> >
> > Currently, livepatch infrastructure in the kernel relies on
> > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
> > kernel module loader knows a module is indeed livepatch module and can
> > behave accordingly.
> >
> > klp-convert, on the other hand relies on LIVEPATCH_* statement in the
> > module's Makefile for exactly the same reason.
> >
> > Remove dependency on modinfo and generate MODULE_INFO flag
> > automatically in modpost when LIVEPATCH_* is defined in the module's
> > Makefile. Generate a list of all built livepatch modules based on
> > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
> > this list as an argument for modpost which will use it to identify
> > livepatch modules.
> >
> > As MODULE_INFO is no longer needed, remove it.
> 
> 
> I do not understand this patch.
> This makes the implementation so complicated.
> 
> I think MODULE_INFO(livepatch, "Y") is cleaner than
> LIVEPATCH_* in Makefile.
> 
> 
> How about this approach?
> 
> 
> [1] Make modpost generate the list of livepatch modules.
>     (livepatch-modules)
> 
> [2] Generate Symbols.list in scripts/Makefile.modpost
>     (vmlinux + modules excluding livepatch-modules)
> 
> [3] Run klp-convert for modules in livepatch-modules.
> 
> 
> If you do this, you can remove most of the build system hacks
> can't you?
> 
> 
> I attached an example implementation for [1].
> 
> Please check whether this works.

Yes, it sounds like a better approach. I've never liked LIVEPATCH_* in 
Makefile much, so I'm all for dropping it.

Thanks
Miroslav

Re: [PATCH v4 07/10] livepatch: Add sample livepatch module

[Masahiro Yamada]

Hi Joe,

On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
>
> Add a new livepatch sample in samples/livepatch/ to make use of
> symbols that must be post-processed to enable load-time relocation
> resolution. As the new sample is to be used as an example, it is
> annotated with KLP_MODULE_RELOC and with KLP_SYMPOS macros.
>
> The livepatch sample updates the function cmdline_proc_show to
> print the string referenced by the symbol saved_command_line
> appended by the string "livepatch=1".
>
> Update livepatch-sample.c to remove livepatch MODULE_INFO
> statement.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---


> --- /dev/null
> +++ b/samples/livepatch/livepatch-annotated-sample.c
> @@ -0,0 +1,102 @@
> +/*
> + * livepatch-annotated-sample.c - Kernel Live Patching Sample Module
> + *
> + * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.

Please use SPDX instead of the license boilerplate.

Thanks.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 07/10] livepatch: Add sample livepatch module

[Joe Lawrence]

On 8/16/19 7:35 AM, Masahiro Yamada wrote:
> Hi Joe,
> 
> On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>
>> --- /dev/null
>> +++ b/samples/livepatch/livepatch-annotated-sample.c
>> @@ -0,0 +1,102 @@

> 
> Please use SPDX instead of the license boilerplate.
> 
> Thanks.
> 

Good eye, this revision was spun before ("treewide: Replace GPLv2 
boilerplate/reference with SPDX - rule 13"), so I will add this to the 
v5 TODO list.

Thanks,

-- Joe