linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 00/10] klp-convert livepatch build tooling
@ 2019-05-09 14:38 Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 01/10] livepatch: Create and include UAPI headers Joe Lawrence
                   ` (10 more replies)
  0 siblings, 11 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

Hi all,

Thanks for the feedback to v3, I've incorporated a few more bug fixes
and style/spelling nitpicks for the series.  v4 is a bit of a resting
place to collect loose ends before considering some heavier future work,
namely arch-specific special section support.  See the TODO section
below for more details.


Previous versions
-----------------

RFC:
  https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/
v2:
  https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/
v3:
  https://lore.kernel.org/lkml/20190410155058.9437-1-joe.lawrence@redhat.com/


Summary
-------

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 a symbol map, 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, add support for symbol mapping in the form of
Symbols.list file; add klp-convert tool; integrate klp-convert tool into
kbuild; make livepatch modules discernible during kernel compilation
pipeline; add data-structure and macros to enable users to annotate
livepatch source code; make modpost stage compatible with livepatches;
update livepatch-sample and update documentation.

The patch was tested under three use-cases:

use-case 1: There is a relocation in the lp that can be automatically
resolved by klp-convert.  For example. see the saved_command_line
variable in lib/livepatch/test_klp_convert2.c.

use-case 2: There is a relocation in the lp that cannot be automatically
resolved, as the name of the respective symbol appears in multiple
objects. The livepatch contains an annotation to enable a correct
relocation.  See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
in lib/livepatch/test_klp_convert{1,2}.c.

use-case 3: There is a relocation in the lp that cannot be automatically
resolved similarly as 2, but no annotation was provided in the
livepatch, triggering an error during compilation.  Reproducible by
removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
lib/livepatch/test_klp_convert{1,2}.c.


Branches
--------

For review purposes, I posted two branches up to github:

  1 - an expanded branch with changes separate from the original
  https://github.com/joe-lawrence/linux/tree/klp-convert-v4-expanded

  2 - a squashed branch of (1) that comprises v4:
  https://github.com/joe-lawrence/linux/tree/klp-convert-v4

Non-trivial commits in the expanded branch have some extra commentary
and details for debugging in the commit message that were dropped when
squashing into their respective parent commits.


TODO
----

Summarized from the v3 thread, thanks to Miroslav, Joao and Josh for
feedback and parsing my long braindumps.

- Special (architecture specific) section support:

  .altinstructions, .altinst_replacement
  .parainstructions
   __jump_table

  We want to apply livepatch relocations *before* these sections are
  processed.  Or more precisely, the special section data structures
  entries which directly or indirectly involve livepatch relocations.
  Those need to be extracted into "klp.arch" sections, a corresponding
  rela section generated, and the kernel needs supporting code to handle
  deferred processing.

  Note that there is already x86-arch code present for handling
  .altinstruction and .parainstructions sections in
  arch/x86/kernel/livepatch.c.

- Support for multiple homonym <object, name> symbols with unique
  <position> values.  Consider a target module with symbol table that
  looks like:

     29: ... FILE    LOCAL  DEFAULT      ABS test_klp_convert_mod_a.c
     32: ... FUNC    LOCAL  DEFAULT        3 get_homonym_string
     33: ... OBJECT  LOCAL  DEFAULT        5 homonym_string
     37: ... FILE    LOCAL  DEFAULT      ABS test_klp_convert_mod_b.c
     38: ... FUNC    LOCAL  DEFAULT        3 get_homonym_string
     39: ... OBJECT  LOCAL  DEFAULT       11 homonym_string

- The BFD library is incapable of handling two rela sections with
  identical sh_info values *as relocation sections*.  This affects
  binutils and related programs like gdb, objdump, crash utility, etc.
  which fail to process klp-converted .ko files.

  https://sourceware.org/bugzilla/show_bug.cgi?id=24456
  https://sourceware.org/ml/binutils/2019-04/msg00194.html


Changelogs
----------

livepatch: Create and include UAPI headers
  v2:
  - jmoreira: split up and changelog
  v3:
  - joe: convert to SPDX license tags
  v4:
  - joe: from Masahiro, update UAPI headers with "GPL-2.0 WITH
    Linux-syscall-note"
  - joe: from Masahiro, types.h isn't needed by UAPI header

kbuild: Support for Symbols.list creation
  v3:
  - jmoreira: adjust for multiobject livepatch
  - joe: add klpclean to PHONY
  - joe: align KLP prefix
  - joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag
  v4:
  - joe: from Miroslav, update the samples and self-test Makefiles with
    the LIVEPATCH_ build prefix.
  - joe: from Artem, use $(SLIST) in klpclean and $(call cmd,livepatch)
    instead of $(call cmd_livepatch)

livepatch: Add klp-convert tool
  v2:
  - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
    at the end
  - jmoreira: add support to automatic relocation conversion in
    klp-convert.c, changelog
  v3:
  - joe: convert to SPDX license tags
  - jmoreira: add rela symbol name to WARNs
  - jmoreira: ignore relocations to .TOC and symbols with index 0
  - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks
  - joe: fix symbol use-after-frees
  - joe: fix remaining valgrind leak complaints (non-error paths only)
  - joe: checkpatch nits
  v4:
  - joe: spelling nits s/Insuficient/Insufficient and s/clasic/classic
  - joe: from Miroslav, tweak klp-convert usage msg
  - joe: don't move multiple list elements in convert_rela() 
  - joe: relax duplicate user symbol check

livepatch: Add klp-convert annotation helpers
  v2:
  - jmoreira: split up: move KLP_MODULE_RELOC from previous patch to
    here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to
    include/linux/livepatch.h
  v3:
  - joe: from Josh, KLP_MODULE_RELOC macro should 4-byte align
    klp_module_reloc structures
  v4:
  - joe: remove the ',' struct array delimiter from KLP_SYMPOS

modpost: Integrate klp-convert
  v2:
  - khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule
    doesn't do that.f
  - khlebnikov: fix bashisms for debian where /bin/sh is a symlink to
    /bin/dash
  - khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise
    arg-check inside if_changed_rule compares cmd_link_module and
    cmd_ld_ko_o.
  - khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is
    true.
  - mbenes: remove modinfo call. LIVEPATCH_ in Makefile
  - jmoreira: split up: move the .livepatch file-based scheme for
    identifying livepatches to a previous patch, as it was required for
    correctly building Symbols.list there.
  v3:
  - joe: adjust rule_ld_ko_o to call echo-cmd
  - joe: rebase for v5.1
  - joe: align KLP prefix
  v4:
  - joe: rule_ld_ko_o should include $(Q) to honor build verbosity

modpost: Add modinfo flag to livepatch modules
  v2:
  - jmoreira: fix modpost.c (add_livepatch_flag) to update module
    structure with livepatch flag and prevent modpost from breaking due to
    unresolved symbols
  v3:
  - joe: adjust modpost.c::get_modinfo() call for v5.0 version
  - joe: from Miroslav: remove MODULE_INFO(livepatch, "Y") from samples

livepatch: Add sample livepatch module
  v2:
  - jmoreira: update module to use KLP_SYMPOS
  - jmoreira: Comments on symbol resolution scheme
  - jmoreira: Update Makefile
  - jmoreira: Remove MODULE_INFO statement
  - jmoreira: Changelog
  v3:
  - joe: rebase for v5.1
  - joe: code shuffle to better match original source file

documentation: Update on livepatch elf format
  v3:
  - joe: clarify sympos=0 and sympos=1..n

livepatch/selftests: add klp-convert
  v3 (new)
  v4:
  - joe: Add a ',' struct array delimiter after KLP_SYMPOS

livepatch/klp-convert: abort on special sections
  v4 (new)


Git boilerplate
---------------

Joao Moreira (2):
  kbuild: Support for Symbols.list creation
  documentation: Update on livepatch elf format

Joe Lawrence (2):
  livepatch/selftests: add klp-convert
  livepatch/klp-convert: abort on special sections

Josh Poimboeuf (5):
  livepatch: Create and include UAPI headers
  livepatch: Add klp-convert tool
  livepatch: Add klp-convert annotation helpers
  modpost: Integrate klp-convert
  livepatch: Add sample livepatch module

Miroslav Benes (1):
  modpost: Add modinfo flag to livepatch modules

 .gitignore                                    |   1 +
 Documentation/livepatch/livepatch.txt         |   3 +
 Documentation/livepatch/module-elf-format.txt |  50 +-
 MAINTAINERS                                   |   2 +
 Makefile                                      |  30 +-
 include/linux/livepatch.h                     |  13 +
 include/uapi/linux/livepatch.h                |  20 +
 kernel/livepatch/core.c                       |   4 +-
 lib/livepatch/Makefile                        |  15 +
 lib/livepatch/test_klp_atomic_replace.c       |   1 -
 lib/livepatch/test_klp_callbacks_demo.c       |   1 -
 lib/livepatch/test_klp_callbacks_demo2.c      |   1 -
 lib/livepatch/test_klp_convert1.c             | 106 +++
 lib/livepatch/test_klp_convert2.c             | 103 +++
 lib/livepatch/test_klp_convert_mod_a.c        |  25 +
 lib/livepatch/test_klp_convert_mod_b.c        |  13 +
 lib/livepatch/test_klp_livepatch.c            |   1 -
 samples/livepatch/Makefile                    |   6 +
 .../livepatch/livepatch-annotated-sample.c    | 102 +++
 samples/livepatch/livepatch-callbacks-demo.c  |   1 -
 samples/livepatch/livepatch-sample.c          |   1 -
 samples/livepatch/livepatch-shadow-fix1.c     |   1 -
 samples/livepatch/livepatch-shadow-fix2.c     |   1 -
 scripts/Kbuild.include                        |   4 +-
 scripts/Makefile                              |   1 +
 scripts/Makefile.build                        |   7 +
 scripts/Makefile.modpost                      |  24 +-
 scripts/livepatch/.gitignore                  |   1 +
 scripts/livepatch/Makefile                    |   7 +
 scripts/livepatch/elf.c                       | 753 ++++++++++++++++++
 scripts/livepatch/elf.h                       |  73 ++
 scripts/livepatch/klp-convert.c               | 731 +++++++++++++++++
 scripts/livepatch/klp-convert.h               |  39 +
 scripts/livepatch/list.h                      | 391 +++++++++
 scripts/mod/modpost.c                         |  82 +-
 scripts/mod/modpost.h                         |   1 +
 .../selftests/livepatch/test-livepatch.sh     |  64 ++
 37 files changed, 2653 insertions(+), 26 deletions(-)
 create mode 100644 include/uapi/linux/livepatch.h
 create mode 100644 lib/livepatch/test_klp_convert1.c
 create mode 100644 lib/livepatch/test_klp_convert2.c
 create mode 100644 lib/livepatch/test_klp_convert_mod_a.c
 create mode 100644 lib/livepatch/test_klp_convert_mod_b.c
 create mode 100644 samples/livepatch/livepatch-annotated-sample.c
 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

-- 
2.20.1


^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v4 01/10] livepatch: Create and include UAPI headers
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 02/10] kbuild: Support for Symbols.list creation Joe Lawrence
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

From: Josh Poimboeuf <jpoimboe@redhat.com>

Define klp prefixes in include/uapi/linux/livepatch.h, and use them for
replacing hard-coded values in kernel/livepatch/core.c.

Update MAINTAINERS.

Note: Add defines to uapi as these are also to be used by a newly
introduced klp-convert script.

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>
---
 MAINTAINERS                    |  1 +
 include/linux/livepatch.h      |  1 +
 include/uapi/linux/livepatch.h | 15 +++++++++++++++
 kernel/livepatch/core.c        |  4 ++--
 4 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 include/uapi/linux/livepatch.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 09f43f1bdd15..52842fa37261 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9016,6 +9016,7 @@ R:	Joe Lawrence <joe.lawrence@redhat.com>
 S:	Maintained
 F:	kernel/livepatch/
 F:	include/linux/livepatch.h
+F:	include/uapi/linux/livepatch.h
 F:	arch/x86/include/asm/livepatch.h
 F:	arch/x86/kernel/livepatch.c
 F:	Documentation/livepatch/
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..16b48e8b29a2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -25,6 +25,7 @@
 #include <linux/ftrace.h>
 #include <linux/completion.h>
 #include <linux/list.h>
+#include <uapi/linux/livepatch.h>
 
 #if IS_ENABLED(CONFIG_LIVEPATCH)
 
diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
new file mode 100644
index 000000000000..e19430918a07
--- /dev/null
+++ b/include/uapi/linux/livepatch.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+/*
+ * livepatch.h - Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@redhat.com>
+ */
+
+#ifndef _UAPI_LIVEPATCH_H
+#define _UAPI_LIVEPATCH_H
+
+#define KLP_RELA_PREFIX		".klp.rela."
+#define KLP_SYM_PREFIX		".klp.sym."
+
+#endif /* _UAPI_LIVEPATCH_H */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb0ee10a1981..3d9ed895b252 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -235,7 +235,7 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
 
 		/* Format: .klp.sym.objname.symname,sympos */
 		cnt = sscanf(strtab + sym->st_name,
-			     ".klp.sym.%55[^.].%127[^,],%lu",
+			     KLP_SYM_PREFIX "%55[^.].%127[^,],%lu",
 			     objname, symname, &sympos);
 		if (cnt != 3) {
 			pr_err("symbol %s has an incorrectly formatted name\n",
@@ -281,7 +281,7 @@ static int klp_write_object_relocations(struct module *pmod,
 		 * See comment in klp_resolve_symbols() for an explanation
 		 * of the selected field width value.
 		 */
-		cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
+		cnt = sscanf(secname, KLP_RELA_PREFIX "%55[^.]", sec_objname);
 		if (cnt != 1) {
 			pr_err("section %s has an incorrectly formatted name\n",
 			       secname);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 02/10] kbuild: Support for Symbols.list creation
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 01/10] livepatch: Create and include UAPI headers Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-05-21 13:48   ` Masahiro Yamada
  2019-05-09 14:38 ` [PATCH v4 03/10] livepatch: Add klp-convert tool Joe Lawrence
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

From: Joao Moreira <jmoreira@suse.de>

For automatic resolution of livepatch relocations, a file called
Symbols.list is used. This file maps symbols within every compiled
kernel object allowing the identification of symbols whose name is
unique, thus relocation can be automatically inferred, or providing
information that helps developers when code annotation is required for
solving the matter.

Add support for creating Symbols.list in the main Makefile. First,
ensure that built-in is compiled when CONFIG_LIVEPATCH is enabled (as
required to achieve a complete Symbols.list file). Define the command to
build Symbols.list (cmd_klp_map) and hook it in the modules rule.

As it is undesirable to have symbols from livepatch objects inside
Symbols.list, make livepatches discernible by modifying
scripts/Makefile.build to create a .livepatch file for each livepatch
in $(MODVERDIR). This file then used by cmd_klp_map to identify and
bypass livepatches.

For identifying livepatches during the build process, a flag variable
LIVEPATCH_$(basetarget).o is considered in scripts/Makefile.build. This
way, set this flag for the livepatch sample Makefile in
samples/livepatch/Makefile.

Finally, Add a clean rule to ensure that Symbols.list is removed during
clean.

Notes:

To achieve a correct Symbols.list file, all kernel objects must be
considered, thus, its construction require these objects to be priorly
built. On the other hand, invoking scripts/Makefile.modpost without
having a complete Symbols.list in place would occasionally lead to
in-tree livepatches being post-processed incorrectly. To prevent this
from becoming a circular dependency, the construction of Symbols.list
uses non-post-processed kernel objects and such does not cause harm as
the symbols normally referenced from within livepatches are visible at
this stage. Also due to these requirements, the spot in-between modules
compilation and the invocation of scripts/Makefile.modpost was picked
for hooking cmd_klp_map.

The approach based on .livepatch files was proposed as an alternative
to using MODULE_INFO statements. This approach was originally
proposed by Miroslav Benes as a workaround for identifying livepathes
without depending on modinfo during the modpost stage. It was moved to
this patch as the approach also shown to be useful while building
Symbols.list.

Signed-off-by: Joao Moreira <jmoreira@suse.de>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 .gitignore                 |  1 +
 Makefile                   | 30 ++++++++++++++++++++++++++----
 lib/livepatch/Makefile     |  5 +++++
 samples/livepatch/Makefile |  4 ++++
 scripts/Makefile.build     |  7 +++++++
 5 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/.gitignore b/.gitignore
index a20ac26aa2f5..5cd5758f5ffe 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,6 +45,7 @@
 *.xz
 Module.symvers
 modules.builtin
+Symbols.list
 
 #
 # Top-level generic files
diff --git a/Makefile b/Makefile
index abe13538a8c0..98089f9d44fe 100644
--- a/Makefile
+++ b/Makefile
@@ -574,10 +574,13 @@ KBUILD_BUILTIN := 1
 # If we have only "make modules", don't compile built-in objects.
 # When we're building modules with modversions, we need to consider
 # the built-in objects during the descend as well, in order to
-# make sure the checksums are up to date before we record them.
+# make sure the checksums are up to date before we record them. The
+# same applies for building livepatches, as built-in objects may hold
+# symbols which are referenced from livepatches and are required by
+# klp-convert post-processing tool for resolving these cases.
 
 ifeq ($(MAKECMDGOALS),modules)
-  KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1)
+  KBUILD_BUILTIN := $(if $(or $(CONFIG_MODVERSIONS), $(CONFIG_LIVEPATCH)),1)
 endif
 
 # If we have "make <whatever> modules", compile modules
@@ -1261,9 +1264,25 @@ all: modules
 # duplicate lines in modules.order files.  Those are removed
 # using awk while concatenating to the final file.
 
+quiet_cmd_klp_map = KLP     Symbols.list
+SLIST = $(objtree)/Symbols.list
+
+define cmd_klp_map
+	$(shell echo "klp-convert-symbol-data.0.1" > $(SLIST))				\
+	$(shell echo "*vmlinux" >> $(SLIST))						\
+	$(shell nm -f posix $(objtree)/vmlinux | cut -d\  -f1 >> $(SLIST))		\
+	$(foreach m, $(wildcard $(MODVERDIR)/*.mod),					\
+		$(eval mod = $(patsubst %.ko,%.o,$(shell head -n1 $(m))))		\
+		$(if $(wildcard $(MODVERDIR)/$(shell basename -s .o $(mod)).livepatch),,\
+			$(eval fmod = $(subst $(quote),_,$(subst -,_,$(mod))))		\
+			$(shell echo "*$(shell basename -s .o $(fmod))" >> $(SLIST))	\
+			$(shell nm -f posix $(mod) | cut -d\  -f1 >> $(SLIST))))
+endef
+
 PHONY += modules
 modules: $(vmlinux-dirs) $(if $(KBUILD_BUILTIN),vmlinux) modules.builtin
 	$(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > $(objtree)/modules.order
+	$(if $(CONFIG_LIVEPATCH), $(call cmd,klp_map))
 	@$(kecho) '  Building modules, stage 2.';
 	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
 
@@ -1350,7 +1369,7 @@ clean: rm-dirs  := $(CLEAN_DIRS)
 clean: rm-files := $(CLEAN_FILES)
 clean-dirs      := $(addprefix _clean_, . $(vmlinux-alldirs) Documentation samples)
 
-PHONY += $(clean-dirs) clean archclean vmlinuxclean
+PHONY += $(clean-dirs) clean archclean vmlinuxclean klpclean
 $(clean-dirs):
 	$(Q)$(MAKE) $(clean)=$(patsubst _clean_%,%,$@)
 
@@ -1358,7 +1377,10 @@ vmlinuxclean:
 	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
 	$(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
 
-clean: archclean vmlinuxclean
+klpclean:
+	$(Q) rm -f $(SLIST)
+
+clean: archclean vmlinuxclean klpclean
 
 # mrproper - Delete all generated files, including .config
 #
diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
index 26900ddaef82..513d200b7942 100644
--- a/lib/livepatch/Makefile
+++ b/lib/livepatch/Makefile
@@ -2,6 +2,11 @@
 #
 # 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_livepatch := y
+
 obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
 				test_klp_callbacks_demo.o \
 				test_klp_callbacks_demo2.o \
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 2472ce39a18d..514c8156f979 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1,3 +1,7 @@
+LIVEPATCH_livepatch-sample := y
+LIVEPATCH_livepatch-shadow-fix1 := y
+LIVEPATCH_livepatch-shadow-fix2 := y
+LIVEPATCH_livepatch-callbacks-demo := y
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 76ca30cc4791..2d8adefd12a5 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -246,6 +246,11 @@ cmd_gen_ksymdeps = \
 	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
 endif
 
+ifdef CONFIG_LIVEPATCH
+cmd_livepatch = $(if $(LIVEPATCH_$(basetarget)),		\
+	$(shell touch $(MODVERDIR)/$(basetarget).livepatch))
+endif
+
 define rule_cc_o_c
 	$(call cmd,checksrc)
 	$(call cmd_and_fixdep,cc_o_c)
@@ -280,6 +285,7 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
 $(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
 	$(call cmd,force_checksrc)
 	$(call if_changed_rule,cc_o_c)
+	$(call cmd,livepatch)
 	@{ echo $(@:.o=.ko); echo $@; \
 	   $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
 
@@ -456,6 +462,7 @@ cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ $(filter %.o,$^) $(cmd_secanalysis
 
 $(multi-used-m): FORCE
 	$(call if_changed,link_multi-m)
+	$(call cmd,livepatch)
 	@{ echo $(@:.o=.ko); echo $(filter %.o,$^); \
 	   $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
 $(call multi_depend, $(multi-used-m), .o, -objs -y -m)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 03/10] livepatch: Add klp-convert tool
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 01/10] livepatch: Create and include UAPI headers Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 02/10] kbuild: Support for Symbols.list creation Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-07-31  2:50   ` Masahiro Yamada
  2019-05-09 14:38 ` [PATCH v4 04/10] livepatch: Add klp-convert annotation helpers Joe Lawrence
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

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
+HOSTLDLIBS_klp-convert		:= -lelf
diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
new file mode 100644
index 000000000000..11746bccecf2
--- /dev/null
+++ b/scripts/livepatch/elf.c
@@ -0,0 +1,753 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * elf.c - ELF access library
+ *
+ * Adapted from kpatch (https://github.com/dynup/kpatch):
+ * Copyright (C) 2013-2016 Josh Poimboeuf <jpoimboe@redhat.com>
+ * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "elf.h"
+
+#define WARN(format, ...) \
+	fprintf(stderr, "%s: " format "\n", elf->name, ##__VA_ARGS__)
+
+/*
+ * Fallback for systems without this "read, mmaping if possible" cmd.
+ */
+#ifndef ELF_C_READ_MMAP
+#define ELF_C_READ_MMAP ELF_C_READ
+#endif
+
+bool is_rela_section(struct section *sec)
+{
+	return (sec->sh.sh_type == SHT_RELA);
+}
+
+struct section *find_section_by_name(struct elf *elf, const char *name)
+{
+	struct section *sec;
+
+	list_for_each_entry(sec, &elf->sections, list)
+		if (!strcmp(sec->name, name))
+			return sec;
+
+	return NULL;
+}
+
+static struct section *find_section_by_index(struct elf *elf,
+					     unsigned int idx)
+{
+	struct section *sec;
+
+	list_for_each_entry(sec, &elf->sections, list)
+		if (sec->idx == idx)
+			return sec;
+
+	return NULL;
+}
+
+static struct symbol *find_symbol_by_index(struct elf *elf, unsigned int idx)
+{
+	struct symbol *sym;
+
+	list_for_each_entry(sym, &elf->symbols, list)
+		if (sym->idx == idx)
+			return sym;
+
+	return NULL;
+}
+
+static int read_sections(struct elf *elf)
+{
+	Elf_Scn *s = NULL;
+	struct section *sec;
+	size_t shstrndx, sections_nr;
+	int i;
+
+	if (elf_getshdrnum(elf->elf, &sections_nr)) {
+		perror("elf_getshdrnum");
+		return -1;
+	}
+
+	if (elf_getshdrstrndx(elf->elf, &shstrndx)) {
+		perror("elf_getshdrstrndx");
+		return -1;
+	}
+
+	for (i = 0; i < sections_nr; i++) {
+		sec = malloc(sizeof(*sec));
+		if (!sec) {
+			perror("malloc");
+			return -1;
+		}
+		memset(sec, 0, sizeof(*sec));
+
+		INIT_LIST_HEAD(&sec->relas);
+
+		list_add_tail(&sec->list, &elf->sections);
+
+		s = elf_getscn(elf->elf, i);
+		if (!s) {
+			perror("elf_getscn");
+			return -1;
+		}
+
+		sec->idx = elf_ndxscn(s);
+
+		if (!gelf_getshdr(s, &sec->sh)) {
+			perror("gelf_getshdr");
+			return -1;
+		}
+
+		sec->name = elf_strptr(elf->elf, shstrndx, sec->sh.sh_name);
+		if (!sec->name) {
+			perror("elf_strptr");
+			return -1;
+		}
+
+		sec->elf_data = elf_getdata(s, NULL);
+		if (!sec->elf_data) {
+			perror("elf_getdata");
+			return -1;
+		}
+
+		if (sec->elf_data->d_off != 0 ||
+		    sec->elf_data->d_size != sec->sh.sh_size) {
+			WARN("unexpected data attributes for %s", sec->name);
+			return -1;
+		}
+
+		sec->data = sec->elf_data->d_buf;
+		sec->size = sec->elf_data->d_size;
+	}
+
+	/* sanity check, one more call to elf_nextscn() should return NULL */
+	if (elf_nextscn(elf->elf, s)) {
+		WARN("section entry mismatch");
+		return -1;
+	}
+
+	return 0;
+}
+
+static int read_symbols(struct elf *elf)
+{
+	struct section *symtab;
+	struct symbol *sym;
+	int symbols_nr, i;
+
+	symtab = find_section_by_name(elf, ".symtab");
+	if (!symtab) {
+		WARN("missing symbol table");
+		return -1;
+	}
+
+	symbols_nr = symtab->sh.sh_size / symtab->sh.sh_entsize;
+
+	for (i = 0; i < symbols_nr; i++) {
+		sym = malloc(sizeof(*sym));
+		if (!sym) {
+			perror("malloc");
+			return -1;
+		}
+		memset(sym, 0, sizeof(*sym));
+
+		sym->idx = i;
+
+		if (!gelf_getsym(symtab->elf_data, i, &sym->sym)) {
+			perror("gelf_getsym");
+			goto err;
+		}
+
+		sym->name = elf_strptr(elf->elf, symtab->sh.sh_link,
+				       sym->sym.st_name);
+		if (!sym->name) {
+			perror("elf_strptr");
+			goto err;
+		}
+
+		sym->type = GELF_ST_TYPE(sym->sym.st_info);
+		sym->bind = GELF_ST_BIND(sym->sym.st_info);
+
+		if (sym->sym.st_shndx > SHN_UNDEF &&
+		    sym->sym.st_shndx < SHN_LORESERVE) {
+			sym->sec = find_section_by_index(elf,
+							 sym->sym.st_shndx);
+			if (!sym->sec) {
+				WARN("couldn't find section for symbol %s",
+				     sym->name);
+				goto err;
+			}
+			if (sym->type == STT_SECTION) {
+				sym->name = sym->sec->name;
+				sym->sec->sym = sym;
+			}
+		}
+
+		sym->offset = sym->sym.st_value;
+		sym->size = sym->sym.st_size;
+
+		list_add_tail(&sym->list, &elf->symbols);
+	}
+
+	return 0;
+
+err:
+	free(sym);
+	return -1;
+}
+
+static int read_relas(struct elf *elf)
+{
+	struct section *sec;
+	struct rela *rela;
+	int i;
+	unsigned int symndx;
+
+	list_for_each_entry(sec, &elf->sections, list) {
+		if (sec->sh.sh_type != SHT_RELA)
+			continue;
+
+		sec->base = find_section_by_name(elf, sec->name + 5);
+		if (!sec->base) {
+			WARN("can't find base section for rela section %s",
+			     sec->name);
+			return -1;
+		}
+
+		sec->base->rela = sec;
+
+		for (i = 0; i < sec->sh.sh_size / sec->sh.sh_entsize; i++) {
+			rela = malloc(sizeof(*rela));
+			if (!rela) {
+				perror("malloc");
+				return -1;
+			}
+			memset(rela, 0, sizeof(*rela));
+
+			if (!gelf_getrela(sec->elf_data, i, &rela->rela)) {
+				perror("gelf_getrela");
+				return -1;
+			}
+
+			rela->type = GELF_R_TYPE(rela->rela.r_info);
+			rela->addend = rela->rela.r_addend;
+			rela->offset = rela->rela.r_offset;
+			symndx = GELF_R_SYM(rela->rela.r_info);
+			rela->sym = find_symbol_by_index(elf, symndx);
+			if (!rela->sym) {
+				WARN("can't find rela entry symbol %d for %s",
+				     symndx, sec->name);
+				return -1;
+			}
+
+			list_add_tail(&rela->list, &sec->relas);
+		}
+	}
+
+	return 0;
+}
+
+struct section *create_rela_section(struct elf *elf, const char *name,
+				    struct section *base)
+{
+	struct section *sec;
+
+	sec = malloc(sizeof(*sec));
+	if (!sec) {
+		WARN("malloc failed");
+		return NULL;
+	}
+	memset(sec, 0, sizeof(*sec));
+	INIT_LIST_HEAD(&sec->relas);
+
+	sec->base = base;
+	sec->name = strdup(name);
+	if (!sec->name) {
+		WARN("strdup failed");
+		return NULL;
+	}
+	sec->sh.sh_name = -1;
+	sec->sh.sh_type = SHT_RELA;
+	sec->sh.sh_entsize = sizeof(GElf_Rela);
+	sec->sh.sh_addralign = 8;
+	sec->sh.sh_flags = SHF_ALLOC;
+
+	sec->elf_data = malloc(sizeof(*sec->elf_data));
+	if (!sec->elf_data) {
+		WARN("malloc failed");
+		return NULL;
+	}
+	memset(sec->elf_data, 0, sizeof(*sec->elf_data));
+	sec->elf_data->d_type = ELF_T_RELA;
+
+	list_add_tail(&sec->list, &elf->sections);
+
+	return sec;
+}
+
+static int update_shstrtab(struct elf *elf)
+{
+	struct section *shstrtab, *sec;
+	size_t orig_size, new_size = 0, offset, len;
+	char *buf;
+
+	shstrtab = find_section_by_name(elf, ".shstrtab");
+	if (!shstrtab) {
+		WARN("can't find .shstrtab");
+		return -1;
+	}
+
+	orig_size = new_size = shstrtab->size;
+
+	list_for_each_entry(sec, &elf->sections, list) {
+		if (sec->sh.sh_name != -1)
+			continue;
+		new_size += strlen(sec->name) + 1;
+	}
+
+	if (new_size == orig_size)
+		return 0;
+
+	buf = malloc(new_size);
+	if (!buf) {
+		WARN("malloc failed");
+		return -1;
+	}
+	memcpy(buf, (void *)shstrtab->data, orig_size);
+
+	offset = orig_size;
+	list_for_each_entry(sec, &elf->sections, list) {
+		if (sec->sh.sh_name != -1)
+			continue;
+		sec->sh.sh_name = offset;
+		len = strlen(sec->name) + 1;
+		memcpy(buf + offset, sec->name, len);
+		offset += len;
+	}
+
+	shstrtab->elf_data->d_buf = shstrtab->data = buf;
+	shstrtab->elf_data->d_size = shstrtab->size = new_size;
+	shstrtab->sh.sh_size = new_size;
+
+	return 1;
+}
+
+static void free_shstrtab(struct elf *elf)
+{
+	struct section *shstrtab;
+
+	shstrtab = find_section_by_name(elf, ".shstrtab");
+	if (!shstrtab)
+		return;
+
+	free(shstrtab->elf_data->d_buf);
+}
+
+static int update_strtab(struct elf *elf)
+{
+	struct section *strtab;
+	struct symbol *sym;
+	size_t orig_size, new_size = 0, offset, len;
+	char *buf;
+
+	strtab = find_section_by_name(elf, ".strtab");
+	if (!strtab) {
+		WARN("can't find .strtab");
+		return -1;
+	}
+
+	orig_size = new_size = strtab->size;
+
+	list_for_each_entry(sym, &elf->symbols, list) {
+		if (sym->sym.st_name != -1)
+			continue;
+		new_size += strlen(sym->name) + 1;
+	}
+
+	if (new_size == orig_size)
+		return 0;
+
+	buf = malloc(new_size);
+	if (!buf) {
+		WARN("malloc failed");
+		return -1;
+	}
+	memcpy(buf, (void *)strtab->data, orig_size);
+
+	offset = orig_size;
+	list_for_each_entry(sym, &elf->symbols, list) {
+		if (sym->sym.st_name != -1)
+			continue;
+		sym->sym.st_name = offset;
+		len = strlen(sym->name) + 1;
+		memcpy(buf + offset, sym->name, len);
+		offset += len;
+	}
+
+	strtab->elf_data->d_buf = strtab->data = buf;
+	strtab->elf_data->d_size = strtab->size = new_size;
+	strtab->sh.sh_size = new_size;
+
+	return 1;
+}
+
+static void free_strtab(struct elf *elf)
+{
+	struct section *strtab;
+
+	strtab = find_section_by_name(elf, ".strtab");
+	if (!strtab)
+		return;
+
+	if (strtab->elf_data)
+		free(strtab->elf_data->d_buf);
+}
+
+static int update_symtab(struct elf *elf)
+{
+	struct section *symtab, *sec;
+	struct symbol *sym;
+	char *buf;
+	size_t size;
+	int offset = 0, nr_locals = 0, idx, nr_syms;
+
+	idx = 0;
+	list_for_each_entry(sec, &elf->sections, list)
+		sec->idx = idx++;
+
+	idx = 0;
+	list_for_each_entry(sym, &elf->symbols, list) {
+		sym->idx = idx++;
+		if (sym->sec)
+			sym->sym.st_shndx = sym->sec->idx;
+	}
+	nr_syms = idx;
+
+	symtab = find_section_by_name(elf, ".symtab");
+	if (!symtab) {
+		WARN("can't find symtab");
+		return -1;
+	}
+
+	symtab->sh.sh_link = find_section_by_name(elf, ".strtab")->idx;
+
+	/* create new symtab buffer */
+	size = nr_syms * symtab->sh.sh_entsize;
+	buf = malloc(size);
+	if (!buf) {
+		WARN("malloc failed");
+		return -1;
+	}
+	memset(buf, 0, size);
+
+	offset = 0;
+	list_for_each_entry(sym, &elf->symbols, list) {
+		memcpy(buf + offset, &sym->sym, symtab->sh.sh_entsize);
+		offset += symtab->sh.sh_entsize;
+
+		if (sym->bind == STB_LOCAL)
+			nr_locals++;
+	}
+
+	symtab->elf_data->d_buf = symtab->data = buf;
+	symtab->elf_data->d_size = symtab->size = size;
+	symtab->sh.sh_size = size;
+
+	/* update symtab section header */
+	symtab->sh.sh_info = nr_locals;
+
+	return 1;
+}
+
+static void free_symtab(struct elf *elf)
+{
+	struct section *symtab;
+
+	symtab = find_section_by_name(elf, ".symtab");
+	if (!symtab)
+		return;
+
+	free(symtab->elf_data->d_buf);
+}
+
+static int update_relas(struct elf *elf)
+{
+	struct section *sec, *symtab;
+	struct rela *rela;
+	int nr_relas, idx, size;
+	GElf_Rela *relas;
+
+	symtab = find_section_by_name(elf, ".symtab");
+
+	list_for_each_entry(sec, &elf->sections, list) {
+		if (!is_rela_section(sec))
+			continue;
+
+		sec->sh.sh_link = symtab->idx;
+		if (sec->base)
+			sec->sh.sh_info = sec->base->idx;
+
+		nr_relas = 0;
+		list_for_each_entry(rela, &sec->relas, list)
+			nr_relas++;
+
+		size = nr_relas * sizeof(*relas);
+		relas = malloc(size);
+		if (!relas) {
+			WARN("malloc failed");
+			return -1;
+		}
+
+		sec->elf_data->d_buf = sec->data = relas;
+		sec->elf_data->d_size = sec->size = size;
+		sec->sh.sh_size = size;
+
+		idx = 0;
+		list_for_each_entry(rela, &sec->relas, list) {
+			relas[idx].r_offset = rela->offset;
+			relas[idx].r_addend = rela->addend;
+			relas[idx].r_info = GELF_R_INFO(rela->sym->idx,
+							rela->type);
+			idx++;
+		}
+	}
+
+	return 1;
+}
+
+static void free_relas(struct elf *elf)
+{
+	struct section *sec, *symtab;
+
+	symtab = find_section_by_name(elf, ".symtab");
+	if (!symtab)
+		return;
+
+	list_for_each_entry(sec, &elf->sections, list) {
+		if (!is_rela_section(sec))
+			continue;
+
+		free(sec->elf_data->d_buf);
+	}
+}
+
+static int write_file(struct elf *elf, const char *file)
+{
+	int fd;
+	Elf *e;
+	GElf_Ehdr eh, ehout;
+	Elf_Scn *scn;
+	Elf_Data *data;
+	GElf_Shdr sh;
+	struct section *sec;
+
+	fd = creat(file, 0664);
+	if (fd == -1) {
+		WARN("couldn't create %s", file);
+		return -1;
+	}
+
+	e = elf_begin(fd, ELF_C_WRITE, NULL);
+	if (!e) {
+		WARN("elf_begin failed");
+		return -1;
+	}
+
+	if (!gelf_newehdr(e, gelf_getclass(elf->elf))) {
+		WARN("gelf_newehdr failed");
+		return -1;
+	}
+
+	if (!gelf_getehdr(e, &ehout)) {
+		WARN("gelf_getehdr failed");
+		return -1;
+	}
+
+	if (!gelf_getehdr(elf->elf, &eh)) {
+		WARN("gelf_getehdr failed");
+		return -1;
+	}
+
+	memset(&ehout, 0, sizeof(ehout));
+	ehout.e_ident[EI_DATA] = eh.e_ident[EI_DATA];
+	ehout.e_machine = eh.e_machine;
+	ehout.e_type = eh.e_type;
+	ehout.e_version = EV_CURRENT;
+	ehout.e_shstrndx = find_section_by_name(elf, ".shstrtab")->idx;
+
+	list_for_each_entry(sec, &elf->sections, list) {
+		if (!sec->idx)
+			continue;
+		scn = elf_newscn(e);
+		if (!scn) {
+			WARN("elf_newscn failed");
+			return -1;
+		}
+
+		data = elf_newdata(scn);
+		if (!data) {
+			WARN("elf_newdata failed");
+			return -1;
+		}
+
+		if (!elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY)) {
+			WARN("elf_flagdata failed");
+			return -1;
+		}
+
+		data->d_type = sec->elf_data->d_type;
+		data->d_buf = sec->elf_data->d_buf;
+		data->d_size = sec->elf_data->d_size;
+
+		if (!gelf_getshdr(scn, &sh)) {
+			WARN("gelf_getshdr failed");
+			return -1;
+		}
+
+		sh = sec->sh;
+
+		if (!gelf_update_shdr(scn, &sh)) {
+			WARN("gelf_update_shdr failed");
+			return -1;
+		}
+	}
+
+	if (!gelf_update_ehdr(e, &ehout)) {
+		WARN("gelf_update_ehdr failed");
+		return -1;
+	}
+
+	if (elf_update(e, ELF_C_WRITE) < 0) {
+		fprintf(stderr, "%s\n", elf_errmsg(-1));
+		WARN("elf_update failed");
+		return -1;
+	}
+
+	elf_end(e);
+
+	return 0;
+}
+
+int elf_write_file(struct elf *elf, const char *file)
+{
+	int ret_shstrtab;
+	int ret_strtab;
+	int ret_symtab;
+	int ret_relas;
+	int ret;
+
+	ret_shstrtab = update_shstrtab(elf);
+	if (ret_shstrtab < 0)
+		return ret_shstrtab;
+
+	ret_strtab = update_strtab(elf);
+	if (ret_strtab < 0)
+		return ret_strtab;
+
+	ret_symtab = update_symtab(elf);
+	if (ret_symtab < 0)
+		return ret_symtab;
+
+	ret_relas = update_relas(elf);
+	if (ret_relas < 0)
+		return ret_relas;
+
+	ret = write_file(elf, file);
+	if (ret)
+		return ret;
+
+	if (ret_relas > 0)
+		free_relas(elf);
+	if (ret_symtab > 0)
+		free_symtab(elf);
+	if (ret_strtab > 0)
+		free_strtab(elf);
+	if (ret_shstrtab > 0)
+		free_shstrtab(elf);
+
+	return 0;
+}
+
+struct elf *elf_open(const char *name)
+{
+	struct elf *elf;
+
+	elf_version(EV_CURRENT);
+
+	elf = malloc(sizeof(*elf));
+	if (!elf) {
+		perror("malloc");
+		return NULL;
+	}
+	memset(elf, 0, sizeof(*elf));
+
+	INIT_LIST_HEAD(&elf->sections);
+	INIT_LIST_HEAD(&elf->symbols);
+
+	elf->fd = open(name, O_RDONLY);
+	if (elf->fd == -1) {
+		perror("open");
+		goto err;
+	}
+
+	elf->elf = elf_begin(elf->fd, ELF_C_READ_MMAP, NULL);
+	if (!elf->elf) {
+		perror("elf_begin");
+		goto err;
+	}
+
+	if (!gelf_getehdr(elf->elf, &elf->ehdr)) {
+		perror("gelf_getehdr");
+		goto err;
+	}
+
+	if (read_sections(elf))
+		goto err;
+
+	if (read_symbols(elf))
+		goto err;
+
+	if (read_relas(elf))
+		goto err;
+
+	return elf;
+
+err:
+	elf_close(elf);
+	return NULL;
+}
+
+void elf_close(struct elf *elf)
+{
+	struct section *sec, *tmpsec;
+	struct symbol *sym, *tmpsym;
+	struct rela *rela, *tmprela;
+
+	list_for_each_entry_safe(sym, tmpsym, &elf->symbols, list) {
+		list_del(&sym->list);
+		free(sym);
+	}
+	list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
+		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+			list_del(&rela->list);
+			free(rela);
+		}
+		list_del(&sec->list);
+		free(sec);
+	}
+	if (elf->fd > 0)
+		close(elf->fd);
+	if (elf->elf)
+		elf_end(elf->elf);
+	free(elf);
+}
diff --git a/scripts/livepatch/elf.h b/scripts/livepatch/elf.h
new file mode 100644
index 000000000000..755140917585
--- /dev/null
+++ b/scripts/livepatch/elf.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2015-2016 Josh Poimboeuf <jpoimboe@redhat.com>
+ */
+
+#ifndef _KLP_POST_ELF_H
+#define _KLP_POST_ELF_H
+
+#include <stdio.h>
+#include <stdbool.h>
+#include <gelf.h>
+#include "list.h"
+
+#ifdef LIBELF_USE_DEPRECATED
+# define elf_getshdrnum    elf_getshnum
+# define elf_getshdrstrndx elf_getshstrndx
+#endif
+
+struct section {
+	struct list_head list;
+	GElf_Shdr sh;
+	struct section *base, *rela;
+	struct list_head relas;
+	struct symbol *sym;
+	Elf_Data *elf_data;
+	char *name;
+	int idx;
+	void *data;
+	unsigned int size;
+};
+
+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;
+	unsigned long offset;
+	unsigned int size;
+};
+
+struct rela {
+	struct list_head list;
+	GElf_Rela rela;
+	struct symbol *sym;
+	unsigned int type;
+	unsigned long offset;
+	int addend;
+};
+
+struct elf {
+	Elf *elf;
+	GElf_Ehdr ehdr;
+	int fd;
+	char *name;
+	struct list_head sections;
+	struct list_head symbols;
+};
+
+
+struct elf *elf_open(const char *name);
+bool is_rela_section(struct section *sec);
+struct section *find_section_by_name(struct elf *elf, const char *name);
+struct section *create_rela_section(struct elf *elf, const char *name,
+				    struct section *base);
+
+void elf_close(struct elf *elf);
+int elf_write_file(struct elf *elf, const char *file);
+
+
+#endif /* _KLP_POST_ELF_H */
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
new file mode 100644
index 000000000000..72b65428e738
--- /dev/null
+++ b/scripts/livepatch/klp-convert.c
@@ -0,0 +1,713 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@redhat.com>
+ * Copyright (C) 2017 Joao Moreira   <jmoreira@suse.de>
+ */
+
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include "elf.h"
+#include "list.h"
+#include "klp-convert.h"
+
+/*
+ * Symbols parsed from Symbols.list are kept in two lists:
+ * - symbols: keeps non-exported symbols
+ * - exp_symbols: keeps exported symbols (__ksymtab_prefixed)
+ */
+static LIST_HEAD(symbols);
+static LIST_HEAD(exp_symbols);
+
+/* In-livepatch user-provided symbol positions are kept in list usr_symbols */
+static LIST_HEAD(usr_symbols);
+
+static void free_syms_lists(void)
+{
+	struct symbol_entry *entry, *aux;
+	struct sympos *sp, *sp_aux;
+
+	list_for_each_entry_safe(entry, aux, &symbols, list) {
+		free(entry->object_name);
+		free(entry->symbol_name);
+		list_del(&entry->list);
+		free(entry);
+	}
+
+	list_for_each_entry_safe(entry, aux, &exp_symbols, list) {
+		free(entry->object_name);
+		free(entry->symbol_name);
+		list_del(&entry->list);
+		free(entry);
+	}
+
+	list_for_each_entry_safe(sp, sp_aux, &usr_symbols, list) {
+		free(sp->object_name);
+		free(sp->symbol_name);
+		list_del(&sp->list);
+		free(sp);
+	}
+}
+
+/* Parses file and fill symbols and exp_symbols list */
+static bool load_syms_lists(const char *symbols_list)
+{
+	FILE *fsyms;
+	struct symbol_entry *entry;
+	size_t len = 0;
+	ssize_t n;
+	char *obj = NULL, *sym = NULL;
+
+	fsyms = fopen(symbols_list, "r");
+	if (!fsyms) {
+		WARN("Unable to open Symbol list: %s", symbols_list);
+		return false;
+	}
+
+	/* read file format version */
+	n = getline(&sym, &len, fsyms);
+	if (n <= 0) {
+		WARN("Unable to read Symbol list: %s", symbols_list);
+		return false;
+	}
+
+	if (strncmp(sym, "klp-convert-symbol-data.0.1", 27) != 0) {
+		WARN("Symbol list is in unknown format.");
+		return false;
+	}
+
+	len = 0;
+	free(sym);
+	sym = NULL;
+
+	/* read file */
+	n = getline(&sym, &len, fsyms);
+	while (n > 0) {
+		if (sym[n-1] == '\n')
+			sym[n-1] = '\0';
+
+		/* Objects in Symbols.list are flagged with '*' */
+		if (sym[0] == '*') {
+			if (obj)
+				free(obj);
+			obj = strdup(sym+1);
+			if (!obj) {
+				WARN("Unable to allocate object name\n");
+				return false;
+			}
+			free(sym);
+		} else {
+			entry = calloc(1, sizeof(struct symbol_entry));
+			if (!entry) {
+				WARN("Unable to allocate Symbol entry\n");
+				return false;
+			}
+
+			entry->object_name = strdup(obj);
+			if (!entry->object_name) {
+				WARN("Unable to allocate entry object name\n");
+				return false;
+			}
+
+			entry->symbol_name = sym;
+			if (strncmp(entry->symbol_name, "__ksymtab_", 10) == 0)
+				list_add(&entry->list, &exp_symbols);
+			else
+				list_add(&entry->list, &symbols);
+		}
+		len = 0;
+		sym = NULL;
+		n = getline(&sym, &len, fsyms);
+	}
+	free(sym);
+	free(obj);
+	fclose(fsyms);
+	return true;
+}
+
+/* Searches for sympos of specific symbol in usr_symbols list */
+static bool get_usr_sympos(struct symbol *s, struct sympos *sp)
+{
+	struct sympos *aux;
+
+	list_for_each_entry(aux, &usr_symbols, list) {
+		if (strcmp(aux->symbol_name, s->name) == 0) {
+			sp->symbol_name = aux->symbol_name;
+			sp->object_name = aux->object_name;
+			sp->pos = aux->pos;
+			return true;
+		}
+	}
+	return false;
+}
+
+/* Removes symbols used for sympos annotation from livepatch elf object */
+static void clear_sympos_symbols(struct section *sec, struct elf *klp_elf)
+{
+	struct symbol *sym, *aux;
+
+	list_for_each_entry_safe(sym, aux, &klp_elf->symbols, list) {
+		if (sym->sec == sec) {
+
+			struct section *sec;
+			struct rela *rela, *tmprela;
+
+			list_for_each_entry(sec, &klp_elf->sections, list) {
+				list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+					if (rela->sym == sym) {
+						list_del(&rela->list);
+						free(rela);
+					}
+				}
+			}
+
+			list_del(&sym->list);
+			free(sym);
+		}
+	}
+}
+
+/* Removes annotation from livepatch elf object */
+static void clear_sympos_annontations(struct elf *klp_elf)
+{
+	struct section *sec, *aux;
+
+	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+		if (strncmp(sec->name, ".klp.module_relocs.", 19) == 0) {
+			clear_sympos_symbols(sec, klp_elf);
+			list_del(&sec->list);
+			free(sec);
+			continue;
+		}
+		if (strncmp(sec->name, ".rela.klp.module_relocs.", 24) == 0) {
+
+			struct rela *rela, *tmprela;
+
+			list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+				list_del(&rela->list);
+				free(rela);
+			}
+			list_del(&sec->list);
+			free(sec);
+			continue;
+		}
+	}
+}
+
+/*
+ * Checks if two or more elements in usr_symbols have the same
+ * object and name, but different symbol position
+ */
+static bool sympos_sanity_check(void)
+{
+	bool sane = true;
+	struct sympos *sp, *aux;
+
+	list_for_each_entry(sp, &usr_symbols, list) {
+		aux = list_next_entry(sp, list);
+		list_for_each_entry_from(aux, &usr_symbols, list) {
+			if (sp->pos != aux->pos &&
+			    strcmp(sp->object_name, aux->object_name) == 0 &&
+			    strcmp(sp->symbol_name, aux->symbol_name) == 0) {
+				WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.",
+				sp->object_name, sp->symbol_name, sp->pos,
+				aux->object_name, aux->symbol_name, aux->pos);
+				sane = false;
+			}
+		}
+	}
+	return sane;
+}
+
+/* Parses the livepatch elf object and fills usr_symbols */
+static bool load_usr_symbols(struct elf *klp_elf)
+{
+	char objname[MODULE_NAME_LEN];
+	struct sympos *sp;
+	struct section *sec, *aux, *relasec;
+	struct rela *rela;
+	struct klp_module_reloc *reloc;
+	int i, nr_entries;
+
+	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+		if (sscanf(sec->name, ".klp.module_relocs.%55s", objname) != 1)
+			continue;
+
+		relasec = sec->rela;
+		reloc = sec->data;
+		i = 0;
+		nr_entries = sec->size / sizeof(*reloc);
+		list_for_each_entry(rela, &relasec->relas, list) {
+			if (i >= nr_entries) {
+				WARN("section %s length beyond nr_entries\n",
+						relasec->name);
+				return false;
+			}
+			sp = calloc(1, sizeof(struct sympos));
+			if (!sp) {
+				WARN("Unable to allocate sympos memory\n");
+				return false;
+			}
+			sp->object_name = strdup(objname);
+			if (!sp->object_name) {
+				WARN("Unable to allocate object name\n");
+				return false;
+			}
+			sp->symbol_name = strdup(rela->sym->name);
+			if (!sp->symbol_name) {
+				WARN("Unable to allocate symbol name\n");
+				return false;
+			}
+			sp->pos = reloc[i].sympos;
+			list_add(&sp->list, &usr_symbols);
+			i++;
+		}
+		if (i != nr_entries) {
+			WARN("nr_entries mismatch (%d != %d) for %s\n",
+					i, nr_entries, relasec->name);
+			return false;
+		}
+	}
+	clear_sympos_annontations(klp_elf);
+	return sympos_sanity_check();
+}
+
+/* prints list of valid sympos for symbol with provided name */
+static void print_valid_module_relocs(char *name)
+{
+	struct symbol_entry *e;
+	char *cur_obj = "";
+	int counter = 0;
+	bool first = true;
+
+	/* Symbols from the same object are locally gathered in the list */
+	fprintf(stderr, "Valid KLP_SYMPOS for symbol %s:\n", name);
+	fprintf(stderr, "-------------------------------------------------\n");
+	list_for_each_entry(e, &symbols, list) {
+		if (strcmp(e->object_name, cur_obj) != 0) {
+			cur_obj = e->object_name;
+			counter = 0;
+		}
+		if (strcmp(e->symbol_name, name) == 0) {
+			if (counter == 0) {
+				if (!first)
+					fprintf(stderr, "}\n");
+
+				fprintf(stderr, "KLP_MODULE_RELOC(%s){\n",
+						cur_obj);
+				first = false;
+			}
+			fprintf(stderr, "\tKLP_SYMPOS(%s,%d)\n", name, counter);
+			counter++;
+		}
+	}
+	fprintf(stderr, "-------------------------------------------------\n");
+}
+
+/*
+ * Searches for symbol in symbols list and returns its sympos if it is unique,
+ * otherwise prints a list with all considered valid sympos
+ */
+static struct symbol_entry *find_sym_entry_by_name(char *name)
+{
+	struct symbol_entry *found = NULL;
+	struct symbol_entry *e;
+
+	list_for_each_entry(e, &symbols, list) {
+		if (strcmp(e->symbol_name, name) == 0) {
+
+			/*
+			 * If there exist multiple symbols with the same
+			 * name then user-provided sympos is required
+			 */
+			if (found) {
+				WARN("Define KLP_SYMPOS for the symbol: %s",
+						e->symbol_name);
+
+				print_valid_module_relocs(name);
+				return NULL;
+			}
+			found = e;
+		}
+	}
+	if (found)
+		return found;
+
+	return NULL;
+}
+
+/* Checks if sympos is valid, otherwise prints valid sympos list */
+static bool valid_sympos(struct sympos *sp)
+{
+	struct symbol_entry *e;
+
+	if (sp->pos == 0) {
+
+		/*
+		 * sympos of 0 is reserved for uniquely named obj:sym,
+		 * verify that this is the case
+		 */
+		int counter = 0;
+
+		list_for_each_entry(e, &symbols, list) {
+			if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
+			    (strcmp(e->object_name, sp->object_name) == 0)) {
+				counter++;
+			}
+		}
+		if (counter == 1)
+			return true;
+
+		WARN("Provided KLP_SYMPOS of 0, but found %d symbols matching: %s.%s,%d",
+				counter, sp->object_name, sp->symbol_name,
+				sp->pos);
+
+	} else {
+
+		/*
+		 * sympos > 0 indicates a specific commonly-named obj:sym,
+		 * indexing starts with 1
+		 */
+		int index = 1;
+
+		list_for_each_entry(e, &symbols, list) {
+			if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
+			    (strcmp(e->object_name, sp->object_name) == 0)) {
+				if (index == sp->pos)
+					return true;
+				index++;
+			}
+		}
+
+		WARN("Provided KLP_SYMPOS does not match a symbol: %s.%s,%d",
+				sp->object_name, sp->symbol_name, sp->pos);
+	}
+
+	print_valid_module_relocs(sp->symbol_name);
+
+	return false;
+}
+
+/* Returns the right sympos respective to a symbol to be relocated */
+static bool find_missing_position(struct symbol *s, struct sympos *sp)
+{
+	struct symbol_entry *entry;
+
+	if (get_usr_sympos(s, sp)) {
+		if (valid_sympos(sp))
+			return true;
+		return false;
+	}
+
+	/* if no user-provided sympos, search symbol in symbols list */
+	entry = find_sym_entry_by_name(s->name);
+	if (entry) {
+		sp->symbol_name = entry->symbol_name;
+		sp->object_name = entry->object_name;
+		sp->pos = 0;
+		return true;
+	}
+	return false;
+}
+
+/*
+ * Finds or creates a klp rela section based on another given section (@oldsec)
+ * and sympos (@*sp), then returns it
+ */
+static struct section *get_or_create_klp_rela_section(struct section *oldsec,
+		struct sympos *sp, struct elf *klp_elf)
+{
+	char *name;
+	struct section *sec;
+	unsigned int length;
+
+	length = strlen(KLP_RELA_PREFIX) + strlen(sp->object_name)
+		 + strlen(oldsec->base->name) + 2;
+
+	name = calloc(1, length);
+	if (!name) {
+		WARN("Memory allocation failed (%s%s.%s)\n", KLP_RELA_PREFIX,
+				sp->object_name, oldsec->base->name);
+		return NULL;
+	}
+
+	if (snprintf(name, length, KLP_RELA_PREFIX "%s.%s", sp->object_name,
+				oldsec->base->name) >= length) {
+		WARN("Length error (%s)", name);
+		free(name);
+		return NULL;
+	}
+
+	sec = find_section_by_name(klp_elf, name);
+	if (!sec)
+		sec = create_rela_section(klp_elf, name, oldsec->base);
+
+	if (sec)
+		sec->sh.sh_flags |= SHF_RELA_LIVEPATCH;
+
+	free(name);
+	return sec;
+}
+
+/* Converts rela symbol names */
+static bool convert_klp_symbol(struct symbol *s, struct sympos *sp)
+{
+	char *name;
+	char pos[4];	/* assume that pos will never be > 999 */
+	unsigned int length;
+
+	if (snprintf(pos, sizeof(pos), "%d", sp->pos) > sizeof(pos)) {
+		WARN("Insufficient buffer for expanding sympos (%s.%s,%d)\n",
+				sp->object_name, sp->symbol_name, sp->pos);
+		return false;
+	}
+
+	length = strlen(KLP_SYM_PREFIX) + strlen(sp->object_name)
+		 + strlen(sp->symbol_name) + sizeof(pos) + 3;
+
+	name = calloc(1, length);
+	if (!name) {
+		WARN("Memory allocation failed (%s%s.%s,%s)\n", KLP_SYM_PREFIX,
+				sp->object_name, sp->symbol_name, pos);
+		return false;
+	}
+
+	if (snprintf(name, length, KLP_SYM_PREFIX "%s.%s,%s", sp->object_name,
+				sp->symbol_name, pos) >= length) {
+
+		WARN("Length error (%s%s.%s,%s)", KLP_SYM_PREFIX,
+				sp->object_name, sp->symbol_name, pos);
+
+		return false;
+	}
+
+	/*
+	 * Despite the memory waste, we don't mind freeing the original symbol
+	 * name memory chunk. Keeping it there is harmless and, since removing
+	 * bytes from the string section is non-trivial, it is unworthy.
+	 */
+	s->name = name;
+	s->sec = NULL;
+	s->sym.st_name = -1;
+	s->sym.st_shndx = SHN_LIVEPATCH;
+
+	return true;
+}
+
+/*
+ * Convert rela that cannot be resolved by the classic module loader
+ * to the special klp rela one.
+ */
+static bool convert_rela(struct section *oldsec, struct rela *r,
+		struct sympos *sp, struct elf *klp_elf)
+{
+	struct section *sec;
+	struct rela *r1, *r2;
+
+	sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf);
+	if (!sec) {
+		WARN("Can't create or access klp.rela section (%s.%s)\n",
+				sp->object_name, sp->symbol_name);
+		return false;
+	}
+
+	if (!convert_klp_symbol(r->sym, sp)) {
+		WARN("Unable to convert symbol name (%s.%s)\n", sec->name,
+				r->sym->name);
+		return false;
+	}
+
+	/*
+	 * Iterate through the rest of this section's relas and see if
+	 * there are similar symbols.  Set them up to move to the same
+	 * klp_rela_section, too.
+	 */
+
+	list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
+		if (r1->sym->name == r->sym->name) {
+			r1->sym->klp_rela_sec = sec;
+		}
+	}
+	return true;
+}
+
+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);
+}
+
+/* Checks if given symbol name matches a symbol in exp_symbols */
+static bool is_exported(char *sname)
+{
+	struct symbol_entry *e;
+
+	/*
+	 * exp_symbols itens are prefixed with __ksymtab_ - comparisons must
+	 * skip prefix and check if both are properly null-terminated
+	 */
+	list_for_each_entry(e, &exp_symbols, list) {
+		if (strcmp(e->symbol_name + 10, sname) == 0)
+			return true;
+	}
+	return false;
+}
+
+/* Checks if a symbol was previously klp-converted based on its name */
+static bool is_converted(char *sname)
+{
+	int len = strlen(KLP_SYM_PREFIX);
+
+	if (strncmp(sname, KLP_SYM_PREFIX, len) == 0)
+		return true;
+	return false;
+}
+
+/*
+ * Checks if symbol must be converted (conditions):
+ * not resolved, not already converted or isn't an exported symbol
+ */
+static bool must_convert(struct symbol *sym)
+{
+	/* already resolved? */
+	if (sym->sec)
+		return false;
+
+	/* skip symbol with index 0 */
+	if (!sym->idx)
+		return false;
+
+	/* we should not touch .TOC. on ppc64le */
+	if (strcmp(sym->name, ".TOC.") == 0)
+		return false;
+
+	return (!(is_converted(sym->name) || is_exported(sym->name)));
+}
+
+/* Checks if a section is a klp rela section */
+static bool is_klp_rela_section(char *sname)
+{
+	int len = strlen(KLP_RELA_PREFIX);
+
+	if (strncmp(sname, KLP_RELA_PREFIX, len) == 0)
+		return true;
+	return false;
+}
+
+/*
+ * Frees the new names and rela sections as created by convert_rela()
+ */
+static void free_converted_resources(struct elf *klp_elf)
+{
+	struct symbol *sym;
+	struct section *sec;
+
+	list_for_each_entry(sym, &klp_elf->symbols, list) {
+		if (sym->name && is_converted(sym->name)) {
+			free(sym->name);
+		}
+	}
+
+	list_for_each_entry(sec, &klp_elf->sections, list) {
+		if (is_klp_rela_section(sec->name)) {
+			free(sec->elf_data);
+			free(sec->name);
+		}
+	}
+}
+
+int main(int argc, const char **argv)
+{
+	const char *klp_in_module, *klp_out_module, *symbols_list;
+	struct rela *rela, *tmprela;
+	struct section *sec, *aux;
+	struct sympos sp;
+	struct elf *klp_elf;
+
+	if (argc != 4) {
+		WARN("Usage: %s <Symbols.list> <input.ko> <output.ko>", argv[0]);
+		return -1;
+	}
+
+	symbols_list = argv[1];
+	klp_in_module = argv[2];
+	klp_out_module = argv[3];
+
+	klp_elf = elf_open(klp_in_module);
+	if (!klp_elf) {
+		WARN("Unable to read elf file %s\n", klp_in_module);
+		return -1;
+	}
+
+	if (!load_syms_lists(symbols_list))
+		return -1;
+
+	if (!load_usr_symbols(klp_elf)) {
+		WARN("Unable to load user-provided sympos");
+		return -1;
+	}
+
+	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+		if (!is_rela_section(sec) ||
+		    is_klp_rela_section(sec->name))
+			continue;
+
+		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+			if (!must_convert(rela->sym))
+				continue;
+
+			if (!is_converted(rela->sym->name)) {
+				if (!find_missing_position(rela->sym, &sp)) {
+					WARN("Unable to find missing symbol: %s",
+							rela->sym->name);
+					return -1;
+				}
+				if (!convert_rela(sec, rela, &sp, klp_elf)) {
+					WARN("Unable to convert relocation: %s",
+							rela->sym->name);
+					return -1;
+				}
+			}
+
+			move_rela(rela);
+		}
+	}
+
+	free_syms_lists();
+	if (elf_write_file(klp_elf, klp_out_module))
+		return -1;
+
+	free_converted_resources(klp_elf);
+	elf_close(klp_elf);
+
+	return 0;
+}
+
+/* Functions kept commented since they might be useful for future debugging */
+
+/* Dumps sympos list (useful for debugging purposes)
+ * static void dump_sympos(void)
+ * {
+ *	struct sympos *sp;
+ *
+ *	fprintf(stderr, "BEGIN OF SYMPOS DUMP\n");
+ *	list_for_each_entry(sp, &usr_symbols, list) {
+ *		fprintf(stderr, "%s %s %d\n", sp->symbol_name, sp->object_name,
+ *				sp->pos);
+ *	}
+ *	fprintf(stderr, "END OF SYMPOS DUMP\n");
+ * }
+ *
+ *
+ * / Dump symbols list for debugging purposes /
+ * static void dump_symbols(void)
+ * {
+ *	struct symbol_entry *entry;
+ *
+ *	fprintf(stderr, "BEGIN OF SYMBOLS DUMP\n");
+ *	list_for_each_entry(entry, &symbols, list)
+ *		printf("%s %s\n", entry->object_name, entry->symbol_name);
+ *	fprintf(stderr, "END OF SYMBOLS DUMP\n");
+ * }
+ */
diff --git a/scripts/livepatch/klp-convert.h b/scripts/livepatch/klp-convert.h
new file mode 100644
index 000000000000..fb92b1ea4b52
--- /dev/null
+++ b/scripts/livepatch/klp-convert.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@redhat.com>
+ * Copyright (C) 2017 Joao Moreira   <jmoreira@suse.de>
+ *
+ */
+
+#define SHN_LIVEPATCH		0xff20
+#define SHF_RELA_LIVEPATCH	0x00100000
+#define MODULE_NAME_LEN		(64 - sizeof(GElf_Addr))
+#define WARN(format, ...) \
+	fprintf(stderr, "klp-convert: " format "\n", ##__VA_ARGS__)
+
+/*
+ * klp-convert uses macros defined in the linux sources package. To prevent the
+ * dependency when building locally, they are defined below. Also notice that
+ * these should match the definitions from  the targeted kernel.
+ */
+
+#define KLP_RELA_PREFIX		".klp.rela."
+#define KLP_SYM_PREFIX		".klp.sym."
+
+struct symbol_entry {
+	struct list_head list;
+	char *symbol_name;
+	char *object_name;
+};
+
+struct sympos {
+	struct list_head list;
+	char *symbol_name;
+	char *object_name;
+	int pos;
+};
+
+struct klp_module_reloc {
+	void *sym;
+	unsigned int sympos;
+} __attribute__((packed));
diff --git a/scripts/livepatch/list.h b/scripts/livepatch/list.h
new file mode 100644
index 000000000000..4d429120fabf
--- /dev/null
+++ b/scripts/livepatch/list.h
@@ -0,0 +1,391 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_LIST_H
+#define _LINUX_LIST_H
+
+/*
+ * Simple doubly linked list implementation.
+ *
+ * Some of the internal functions ("__xxx") are useful when
+ * manipulating whole lists rather than single entries, as
+ * sometimes we already know the next/prev entries and we can
+ * generate better code by using them directly rather than
+ * using the generic single-entry routines.
+ */
+
+#define WRITE_ONCE(a, b) (a = b)
+#define READ_ONCE(a) a
+
+#undef offsetof
+#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr:        the pointer to the member.
+ * @type:       the type of the container struct this is embedded in.
+ * @member:     the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({			\
+	const typeof(((type *)0)->member) * __mptr = (ptr);	\
+	(type *)((char *)__mptr - offsetof(type, member)); })
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+	struct list_head name = LIST_HEAD_INIT(name)
+
+static inline void INIT_LIST_HEAD(struct list_head *list)
+{
+	WRITE_ONCE(list->next, list);
+	list->prev = list;
+}
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_add(struct list_head *new,
+			      struct list_head *prev,
+			      struct list_head *next)
+{
+	next->prev = new;
+	new->next = next;
+	new->prev = prev;
+	WRITE_ONCE(prev->next, new);
+}
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+static inline void list_add(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head, head->next);
+}
+
+
+/**
+ * list_add_tail - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it before
+ *
+ * Insert a new entry before the specified head.
+ * This is useful for implementing queues.
+ */
+static inline void list_add_tail(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head->prev, head);
+}
+
+/*
+ * Delete a list entry by making the prev/next entries
+ * point to each other.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_del(struct list_head *prev, struct list_head *next)
+{
+	next->prev = prev;
+	WRITE_ONCE(prev->next, next);
+}
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty() on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+static inline void __list_del_entry(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+}
+
+static inline void list_del(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+}
+
+/**
+ * list_is_last - tests whether @list is the last entry in list @head
+ * @list: the entry to test
+ * @head: the head of the list
+ */
+static inline int list_is_last(const struct list_head *list,
+				const struct list_head *head)
+{
+	return list->next == head;
+}
+
+/**
+ * list_empty - tests whether a list is empty
+ * @head: the list to test.
+ */
+static inline int list_empty(const struct list_head *head)
+{
+	return READ_ONCE(head->next) == head;
+}
+
+/**
+ * list_entry - get the struct for this entry
+ * @ptr:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_entry(ptr, type, member) \
+	container_of(ptr, type, member)
+
+/**
+ * list_first_entry - get the first element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_first_entry(ptr, type, member) \
+	list_entry((ptr)->next, type, member)
+
+/**
+ * list_last_entry - get the last element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_last_entry(ptr, type, member) \
+	list_entry((ptr)->prev, type, member)
+
+/**
+ * list_first_entry_or_null - get the first element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Note that if the list is empty, it returns NULL.
+ */
+#define list_first_entry_or_null(ptr, type, member) \
+	(!list_empty(ptr) ? list_first_entry(ptr, type, member) : NULL)
+
+/**
+ * list_next_entry - get the next element in list
+ * @pos:	the type * to cursor
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_next_entry(pos, member) \
+	list_entry((pos)->member.next, typeof(*(pos)), member)
+
+/**
+ * list_prev_entry - get the prev element in list
+ * @pos:	the type * to cursor
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_prev_entry(pos, member) \
+	list_entry((pos)->member.prev, typeof(*(pos)), member)
+
+/**
+ * list_for_each	-	iterate over a list
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @head:	the head for your list.
+ */
+#define list_for_each(pos, head) \
+	for (pos = (head)->next; pos != (head); pos = pos->next)
+
+/**
+ * list_for_each_prev	-	iterate over a list backwards
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @head:	the head for your list.
+ */
+#define list_for_each_prev(pos, head) \
+	for (pos = (head)->prev; pos != (head); pos = pos->prev)
+
+/**
+ * list_for_each_safe - iterate over a list safe against removal of list entry
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @n:		another &struct list_head to use as temporary storage
+ * @head:	the head for your list.
+ */
+#define list_for_each_safe(pos, n, head) \
+	for (pos = (head)->next, n = pos->next; pos != (head); \
+		pos = n, n = pos->next)
+
+/**
+ * list_for_each_prev_safe - iterate over a list backwards safe against removal
+   of list entry
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @n:		another &struct list_head to use as temporary storage
+ * @head:	the head for your list.
+ */
+#define list_for_each_prev_safe(pos, n, head) \
+	for (pos = (head)->prev, n = pos->prev; \
+	     pos != (head); \
+	     pos = n, n = pos->prev)
+
+/**
+ * list_for_each_entry	-	iterate over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry(pos, head, member)				\
+	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	     &pos->member != (head);					\
+	     pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_reverse - iterate backwards over list of given type.
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry_reverse(pos, head, member)			\
+	for (pos = list_last_entry(head, typeof(*pos), member);		\
+	     &pos->member != (head);					\
+	     pos = list_prev_entry(pos, member))
+
+/**
+ * list_prepare_entry - prepare a pos entry for use in
+   list_for_each_entry_continue()
+ * @pos:	the type * to use as a start point
+ * @head:	the head of the list
+ * @member:	the name of the list_head within the struct.
+ *
+ * Prepares a pos entry for use as a start point in
+   list_for_each_entry_continue().
+ */
+#define list_prepare_entry(pos, head, member) \
+	((pos) ? : list_entry(head, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_continue - continue iteration over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Continue to iterate over list of given type, continuing after
+ * the current position.
+ */
+#define list_for_each_entry_continue(pos, head, member)			\
+	for (pos = list_next_entry(pos, member);			\
+	     &pos->member != (head);					\
+	     pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_continue_reverse - iterate backwards from the given point
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Start to iterate over list of given type backwards, continuing after
+ * the current position.
+ */
+#define list_for_each_entry_continue_reverse(pos, head, member)		\
+	for (pos = list_prev_entry(pos, member);			\
+	     &pos->member != (head);					\
+	     pos = list_prev_entry(pos, member))
+
+/**
+ * list_for_each_entry_from - iterate over list of given type from the current
+   point
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Iterate over list of given type, continuing from current position.
+ */
+#define list_for_each_entry_from(pos, head, member)			\
+	for (; &pos->member != (head);					\
+	     pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_safe - iterate over list of given type safe against
+   removal of list entry
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry_safe(pos, n, head, member)			\
+	for (pos = list_first_entry(head, typeof(*pos), member),	\
+		n = list_next_entry(pos, member);			\
+	     &pos->member != (head);					\
+	     pos = n, n = list_next_entry(n, member))
+
+/**
+ * list_for_each_entry_safe_continue - continue list iteration safe against
+ * removal
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Iterate over list of given type, continuing after current point,
+ * safe against removal of list entry.
+ */
+#define list_for_each_entry_safe_continue(pos, n, head, member)		\
+	for (pos = list_next_entry(pos, member),			\
+		n = list_next_entry(pos, member);			\
+	     &pos->member != (head);					\
+	     pos = n, n = list_next_entry(n, member))
+
+/**
+ * list_for_each_entry_safe_from - iterate over list from current point safe
+ * against removal
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Iterate over list of given type from current point, safe against
+ * removal of list entry.
+ */
+#define list_for_each_entry_safe_from(pos, n, head, member)		\
+	for (n = list_next_entry(pos, member);				\
+	     &pos->member != (head);					\
+	     pos = n, n = list_next_entry(n, member))
+
+/**
+ * list_for_each_entry_safe_reverse - iterate backwards over list safe against
+ * removal
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Iterate backwards over list of given type, safe against removal
+ * of list entry.
+ */
+#define list_for_each_entry_safe_reverse(pos, n, head, member)		\
+	for (pos = list_last_entry(head, typeof(*pos), member),		\
+		n = list_prev_entry(pos, member);			\
+	     &pos->member != (head);					\
+	     pos = n, n = list_prev_entry(n, member))
+
+/**
+ * list_safe_reset_next - reset a stale list_for_each_entry_safe loop
+ * @pos:	the loop cursor used in the list_for_each_entry_safe loop
+ * @n:		temporary storage used in list_for_each_entry_safe
+ * @member:	the name of the list_head within the struct.
+ *
+ * list_safe_reset_next is not safe to use in general if the list may be
+ * modified concurrently (eg. the lock is dropped in the loop body). An
+ * exception to this is if the cursor element (pos) is pinned in the list,
+ * and list_safe_reset_next is called after re-taking the lock and before
+ * completing the current iteration of the loop body.
+ */
+#define list_safe_reset_next(pos, n, member)				\
+	(n = list_next_entry(pos, member))
+
+#endif
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 04/10] livepatch: Add klp-convert annotation helpers
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (2 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 03/10] livepatch: Add klp-convert tool Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 05/10] modpost: Integrate klp-convert Joe Lawrence
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

From: Josh Poimboeuf <jpoimboe@redhat.com>

Define macros KLP_MODULE_RELOC and KLP_SYMPOS in
include/linux/livepatch.h to improve user-friendliness of the
livepatch annotation process.

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>
---
 include/linux/livepatch.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 16b48e8b29a2..3071aec8fd72 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -236,6 +236,18 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
 void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
 void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
 
+/* Used to annotate symbol relocations in livepatches */
+#define KLP_MODULE_RELOC(obj)						\
+	struct klp_module_reloc						\
+	__attribute__((__section__(".klp.module_relocs." #obj)))	\
+	__attribute__((aligned (4)))
+
+#define KLP_SYMPOS(symbol, pos)						\
+	{								\
+		.sym = &symbol,						\
+		.sympos = pos,						\
+	}
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 05/10] modpost: Integrate klp-convert
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (3 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 04/10] livepatch: Add klp-convert annotation helpers Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules Joe Lawrence
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

From: Josh Poimboeuf <jpoimboe@redhat.com>

Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
cmd_klp_convert invokes klp-convert with the right arguments for the
conversion of unresolved symbols inside a livepatch.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Joao Moreira <jmoreira@suse.de>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 scripts/Kbuild.include   |  4 +++-
 scripts/Makefile.modpost | 16 +++++++++++++++-
 scripts/mod/modpost.c    |  6 +++++-
 scripts/mod/modpost.h    |  1 +
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 7484b9d8272f..f8b7d12e44c9 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -236,6 +236,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 $^),$^)
@@ -243,7 +245,7 @@ any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
 # Execute command if command has changed or prerequisite(s) are updated.
 if_changed = $(if $(strip $(any-prereq) $(arg-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 $(strip $(any-prereq) $(arg-check)),$(cmd_and_fixdep),@:)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 6b7f354f189a..1bef611f99b6 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -124,8 +124,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 $(wildcard $(MODVERDIR)/$(basetarget).livepatch),	\
+	    $(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)
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..374b22c76ec5 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1974,6 +1974,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);
 
@@ -2101,7 +2105,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);
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.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (4 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 05/10] modpost: Integrate klp-convert Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-07-31  5:58   ` Masahiro Yamada
  2019-05-09 14:38 ` [PATCH v4 07/10] livepatch: Add sample livepatch module Joe Lawrence
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

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.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Joao Moreira <jmoreira@suse.de>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 lib/livepatch/test_klp_atomic_replace.c      |  1 -
 lib/livepatch/test_klp_callbacks_demo.c      |  1 -
 lib/livepatch/test_klp_callbacks_demo2.c     |  1 -
 lib/livepatch/test_klp_livepatch.c           |  1 -
 samples/livepatch/livepatch-callbacks-demo.c |  1 -
 samples/livepatch/livepatch-sample.c         |  1 -
 samples/livepatch/livepatch-shadow-fix1.c    |  1 -
 samples/livepatch/livepatch-shadow-fix2.c    |  1 -
 scripts/Makefile.modpost                     |  8 +-
 scripts/mod/modpost.c                        | 84 ++++++++++++++++++--
 10 files changed, 85 insertions(+), 15 deletions(-)

diff --git a/lib/livepatch/test_klp_atomic_replace.c b/lib/livepatch/test_klp_atomic_replace.c
index 5af7093ca00c..3bf08a1b7b12 100644
--- a/lib/livepatch/test_klp_atomic_replace.c
+++ b/lib/livepatch/test_klp_atomic_replace.c
@@ -52,6 +52,5 @@ static void test_klp_atomic_replace_exit(void)
 module_init(test_klp_atomic_replace_init);
 module_exit(test_klp_atomic_replace_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
 MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
 MODULE_DESCRIPTION("Livepatch test: atomic replace");
diff --git a/lib/livepatch/test_klp_callbacks_demo.c b/lib/livepatch/test_klp_callbacks_demo.c
index 3fd8fe1cd1cc..76e2f51a6771 100644
--- a/lib/livepatch/test_klp_callbacks_demo.c
+++ b/lib/livepatch/test_klp_callbacks_demo.c
@@ -116,6 +116,5 @@ static void test_klp_callbacks_demo_exit(void)
 module_init(test_klp_callbacks_demo_init);
 module_exit(test_klp_callbacks_demo_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
 MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
 MODULE_DESCRIPTION("Livepatch test: livepatch demo");
diff --git a/lib/livepatch/test_klp_callbacks_demo2.c b/lib/livepatch/test_klp_callbacks_demo2.c
index 5417573e80af..76db98fc3071 100644
--- a/lib/livepatch/test_klp_callbacks_demo2.c
+++ b/lib/livepatch/test_klp_callbacks_demo2.c
@@ -88,6 +88,5 @@ static void test_klp_callbacks_demo2_exit(void)
 module_init(test_klp_callbacks_demo2_init);
 module_exit(test_klp_callbacks_demo2_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
 MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
 MODULE_DESCRIPTION("Livepatch test: livepatch demo2");
diff --git a/lib/livepatch/test_klp_livepatch.c b/lib/livepatch/test_klp_livepatch.c
index aff08199de71..d94d0ae232db 100644
--- a/lib/livepatch/test_klp_livepatch.c
+++ b/lib/livepatch/test_klp_livepatch.c
@@ -46,6 +46,5 @@ static void test_klp_livepatch_exit(void)
 module_init(test_klp_livepatch_init);
 module_exit(test_klp_livepatch_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
 MODULE_AUTHOR("Seth Jennings <sjenning@redhat.com>");
 MODULE_DESCRIPTION("Livepatch test: livepatch module");
diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c
index 62d97953ad02..e78249d4bff8 100644
--- a/samples/livepatch/livepatch-callbacks-demo.c
+++ b/samples/livepatch/livepatch-callbacks-demo.c
@@ -205,4 +205,3 @@ static void livepatch_callbacks_demo_exit(void)
 module_init(livepatch_callbacks_demo_init);
 module_exit(livepatch_callbacks_demo_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index 01c9cf003ca2..8900a175046b 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -79,4 +79,3 @@ static void livepatch_exit(void)
 module_init(livepatch_init);
 module_exit(livepatch_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 67a73e5e986e..c5bae813aaab 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -169,4 +169,3 @@ static void livepatch_shadow_fix1_exit(void)
 module_init(livepatch_shadow_fix1_init);
 module_exit(livepatch_shadow_fix1_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index 91c21d52cfea..7cc3c3dc9509 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -141,4 +141,3 @@ static void livepatch_shadow_fix2_exit(void)
 module_init(livepatch_shadow_fix2_init);
 module_exit(livepatch_shadow_fix2_exit);
 MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 1bef611f99b6..f2aee6b8dcfd 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -65,6 +65,11 @@ MODLISTCMD := find $(MODVERDIR) -name '*.mod' | xargs -r grep -h '\.ko$$' | sort
 __modules := $(shell $(MODLISTCMD))
 modules   := $(patsubst %.o,%.ko, $(wildcard $(__modules:.ko=.o)))
 
+# find all .livepatch files listed in $(MODVERDIR)/
+ifdef CONFIG_LIVEPATCH
+$(shell find $(MODVERDIR) -name '*.livepatch' | xargs -r -I{} basename {} .livepatch > $(MODVERDIR)/livepatchmods)
+endif
+
 # Stop after building .o files if NOFINAL is set. Makes compile tests quicker
 _modpost: $(if $(KBUILD_MODPOST_NOFINAL), $(modules:.ko:.o),$(modules))
 
@@ -78,7 +83,8 @@ modpost = scripts/mod/modpost                    \
  $(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
  $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
  $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
- $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
+ $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w) \
+ $(if $(CONFIG_LIVEPATCH),-l $(MODVERDIR)/livepatchmods)
 
 MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 374b22c76ec5..b3ab17d9d834 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1974,10 +1974,6 @@ 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);
 
@@ -2416,6 +2412,76 @@ static void write_dump(const char *fname)
 	free(buf.p);
 }
 
+struct livepatch_mod_list {
+	struct livepatch_mod_list *next;
+	char *livepatch_mod;
+};
+
+static struct livepatch_mod_list *load_livepatch_mods(const char *fname)
+{
+	struct livepatch_mod_list *list_iter, *list_start = NULL;
+	unsigned long size, pos = 0;
+	void *file = grab_file(fname, &size);
+	char *line;
+
+	if (!file)
+		return NULL;
+
+	while ((line = get_next_line(&pos, file, size))) {
+		list_iter = NOFAIL(malloc(sizeof(*list_iter)));
+		list_iter->next = list_start;
+		list_iter->livepatch_mod = NOFAIL(strdup(line));
+		list_start = list_iter;
+	}
+	release_file(file, size);
+
+	return list_start;
+}
+
+static void free_livepatch_mods(struct livepatch_mod_list *list_start)
+{
+	struct livepatch_mod_list *list_iter;
+
+	while (list_start) {
+		list_iter = list_start->next;
+		free(list_start->livepatch_mod);
+		free(list_start);
+		list_start = list_iter;
+	}
+}
+
+static int is_livepatch_mod(const char *modname,
+		struct livepatch_mod_list *list)
+{
+	const char *myname;
+
+	if (!list)
+		return 0;
+
+	myname = strrchr(modname, '/');
+	if (myname)
+		myname++;
+	else
+		myname = modname;
+
+	while (list) {
+		if (!strcmp(myname, list->livepatch_mod))
+			return 1;
+		list = list->next;
+	}
+	return 0;
+}
+
+static void add_livepatch_flag(struct buffer *b, struct module *mod,
+		struct livepatch_mod_list *list)
+{
+	if (is_livepatch_mod(mod->name, list)) {
+		buf_printf(b, "\nMODULE_INFO(livepatch, \"Y\");\n");
+		mod->livepatch = 1;
+	}
+}
+
+
 struct ext_sym_list {
 	struct ext_sym_list *next;
 	const char *file;
@@ -2431,8 +2497,9 @@ int main(int argc, char **argv)
 	int err;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;
+	struct livepatch_mod_list *livepatch_mods = 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;
@@ -2449,6 +2516,9 @@ int main(int argc, char **argv)
 			extsym_iter->file = optarg;
 			extsym_start = extsym_iter;
 			break;
+		case 'l':
+			livepatch_mods = load_livepatch_mods(optarg);
+			break;
 		case 'm':
 			modversions = 1;
 			break;
@@ -2506,15 +2576,16 @@ int main(int argc, char **argv)
 		buf.pos = 0;
 
 		err |= check_modname_len(mod);
-		err |= check_exports(mod);
 		add_header(&buf, mod);
 		add_intree_flag(&buf, !external_module);
 		add_retpoline(&buf);
 		add_staging_flag(&buf, mod->name);
+		add_livepatch_flag(&buf, mod, livepatch_mods);
 		err |= add_versions(&buf, mod);
 		add_depends(&buf, mod);
 		add_moddevtable(&buf, mod);
 		add_srcversion(&buf, mod);
+		err |= check_exports(mod);
 
 		sprintf(fname, "%s.mod.c", mod->name);
 		write_if_changed(&buf, fname);
@@ -2524,6 +2595,7 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	free_livepatch_mods(livepatch_mods);
 	free(buf.p);
 
 	return err;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 07/10] livepatch: Add sample livepatch module
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (5 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-08-16 11:35   ` Masahiro Yamada
  2019-05-09 14:38 ` [PATCH v4 08/10] documentation: Update on livepatch elf format Joe Lawrence
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

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>
---
 samples/livepatch/Makefile                    |   2 +
 .../livepatch/livepatch-annotated-sample.c    | 102 ++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 samples/livepatch/livepatch-annotated-sample.c

diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 514c8156f979..1a92d6b58f33 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -2,6 +2,7 @@ LIVEPATCH_livepatch-sample := y
 LIVEPATCH_livepatch-shadow-fix1 := y
 LIVEPATCH_livepatch-shadow-fix2 := y
 LIVEPATCH_livepatch-callbacks-demo := y
+LIVEPATCH_livepatch-annotated-sample := y
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
@@ -9,3 +10,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-annotated-sample.o
diff --git a/samples/livepatch/livepatch-annotated-sample.c b/samples/livepatch/livepatch-annotated-sample.c
new file mode 100644
index 000000000000..556ce7e0bdab
--- /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/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+/*
+ * This (dumb) live patch overrides the function that prints the
+ * kernel boot cmdline when /proc/cmdline is read.
+ *
+ * This livepatch uses the symbol saved_command_line whose relocation
+ * must be resolved during load time. To enable that, this module
+ * must be post-processed by a tool called klp-convert, which embeds
+ * information to be used by the loader to solve the relocation.
+ *
+ * The module is annotated with KLP_MODULE_RELOC/KLP_SYMPOS macros.
+ * These annotations are used by klp-convert to infer that the symbol
+ * saved_command_line is in the object vmlinux.
+ *
+ * As saved_command_line has no other homonimous symbol across
+ * kernel objects, this annotation is not a requirement, and can be
+ * suppressed with no harm to klp-convert. Yet, it is kept here as an
+ * example on how to annotate livepatch modules that contain symbols
+ * whose names are used in more than one kernel object.
+ *
+ * Example:
+ *
+ * $ cat /proc/cmdline
+ * <your cmdline>
+ *
+ * $ insmod livepatch-sample.ko
+ * $ cat /proc/cmdline
+ * <your cmdline> livepatch=1
+ *
+ * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
+ * $ cat /proc/cmdline
+ * <your cmdline>
+ */
+
+extern char *saved_command_line;
+
+#include <linux/seq_file.h>
+static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "%s livepatch=1\n", saved_command_line);
+	return 0;
+}
+
+KLP_MODULE_RELOC(vmlinux) vmlinux_relocs[] = {
+	KLP_SYMPOS(saved_command_line, 0)
+};
+
+static struct klp_func funcs[] = {
+	{
+		.old_name = "cmdline_proc_show",
+		.new_func = livepatch_cmdline_proc_show,
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		/* name being NULL means vmlinux */
+		.funcs = funcs,
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+static int livepatch_init(void)
+{
+	return klp_enable_patch(&patch);
+}
+
+static void livepatch_exit(void)
+{
+}
+
+module_init(livepatch_init);
+module_exit(livepatch_exit);
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 08/10] documentation: Update on livepatch elf format
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (6 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 07/10] livepatch: Add sample livepatch module Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 09/10] livepatch/selftests: add klp-convert Joe Lawrence
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

From: Joao Moreira <jmoreira@suse.de>

Add a section to Documentation/livepatch/module-elf-format.txt
describing how klp-convert works for fixing relocations.

Signed-off-by: Joao Moreira <jmoreira@suse.de>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/livepatch.txt         |  3 ++
 Documentation/livepatch/module-elf-format.txt | 50 ++++++++++++++++---
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 4627b41ff02e..873c11aee038 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -274,6 +274,9 @@ into three levels:
     absolute position in the database, but rather the order it has been found
     only for a particular object ( vmlinux or a kernel module ). Note that
     kallsyms allows for searching symbols according to the object name.
+    Uniquely named symbols may use a symbol position of 0.  Non-unique
+    symbols need to specify their object / kallsyms position, starting
+    at position 1.
 
   + struct klp_object defines an array of patched functions (struct
     klp_func) in the same object. Where the object is either vmlinux
diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
index f21a5289a09c..7bef8432352a 100644
--- a/Documentation/livepatch/module-elf-format.txt
+++ b/Documentation/livepatch/module-elf-format.txt
@@ -2,7 +2,8 @@
 Livepatch module Elf format
 ===========================
 
-This document outlines the Elf format requirements that livepatch modules must follow.
+This document outlines the Elf format requirements that livepatch modules must
+follow.
 
 -----------------
 Table of Contents
@@ -25,8 +26,9 @@ Table of Contents
        3.3.2 Required name format
        3.3.3 Example livepatch symbol names
        3.3.4 Example `readelf --symbols` output
-4. Architecture-specific sections
-5. Symbol table and Elf section access
+4. Automatic conversion of unresolved relocations
+5. Architecture-specific sections
+6. Symbol table and Elf section access
 
 ----------------------------
 0. Background and motivation
@@ -270,7 +272,8 @@ Livepatch symbol names must conform to the following format:
 [D] The position of the symbol in the object (as according to kallsyms)
     This is used to differentiate duplicate symbols within the same
     object. The symbol position is expressed numerically (0, 1, 2...).
-    The symbol position of a unique symbol is 0.
+    The symbol position of a unique symbol is 0.  The symbol position of
+    the first non-unique symbol is 1, the second is 2, etc.
 
 3.3.3 Example livepatch symbol names:
 -------------------------------------
@@ -293,8 +296,43 @@ Symbol table '.symtab' contains 127 entries:
 [*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
     "OS" means OS-specific.
 
+--------------------------------------------------
+4.  Automatic conversion of unresolved relocations
+--------------------------------------------------
+Sometimes livepatches may operate on symbols which are not self-contained nor
+exported. When this happens, these symbols remain unresolved in the elf object
+and will trigger an error during the livepatch instantiation.
+
+Whenever possible, the kernel building infrastructure solves this problem
+automatically. First, a symbol database containing information on all compiled
+objects is built. Second, this database - a file named Symbols.list, placed in
+the kernel source root directory - is used to identify targets for unresolved
+relocations, converting them in the livepatch elf accordingly to the
+specifications above-described. While the first stage is fully handled by the
+building system, the second is done by a tool called klp-convert, which can be
+found in "scripts/livepatch".
+
+When an unresolved relocation has as target a symbol whose name is also used by
+different symbols throughout the kernel, the relocation cannot be resolved
+automatically. In these cases, the livepatch developer must add annotations to
+the livepatch, making it possible for the system to identify which is the
+correct target amongst multiple homonymous symbols. Such annotations must be
+done through a data structure as follows:
+
+struct KLP_MODULE_RELOC(object) data_structure_name[] = {
+	KLP_SYMPOS(symbol, pos)
+};
+
+In the above example, object refers to the object file which contains the
+symbol, being vmlinux or a module; symbol refers to the symbol name that will
+be relocated and pos is its position in the object.
+
+When a data structure like this is added to the livepatch, the resulting elf
+will hold symbols that will be identified by klp-convert and used to solve name
+ambiguities.
+
 ---------------------------------
-4. Architecture-specific sections
+5. Architecture-specific sections
 ---------------------------------
 Architectures may override arch_klp_init_object_loaded() to perform
 additional arch-specific tasks when a target module loads, such as applying
@@ -305,7 +343,7 @@ be easily identified when iterating through a patch module's Elf sections
 (See arch/x86/kernel/livepatch.c for a complete example).
 
 --------------------------------------
-5. Symbol table and Elf section access
+6. Symbol table and Elf section access
 --------------------------------------
 A livepatch module's symbol table is accessible through module->symtab.
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 09/10] livepatch/selftests: add klp-convert
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (7 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 08/10] documentation: Update on livepatch elf format Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-05-09 14:38 ` [PATCH v4 10/10] livepatch/klp-convert: abort on special sections Joe Lawrence
  2019-06-13 13:00 ` [PATCH v4 00/10] klp-convert livepatch build tooling Miroslav Benes
  10 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

Add a simple klp-convert livepatch selftest that exercises various
symbol homonym sympos scenarios.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 lib/livepatch/Makefile                        |  10 ++
 lib/livepatch/test_klp_convert1.c             | 106 ++++++++++++++++++
 lib/livepatch/test_klp_convert2.c             | 103 +++++++++++++++++
 lib/livepatch/test_klp_convert_mod_a.c        |  25 +++++
 lib/livepatch/test_klp_convert_mod_b.c        |  13 +++
 .../selftests/livepatch/test-livepatch.sh     |  64 +++++++++++
 6 files changed, 321 insertions(+)
 create mode 100644 lib/livepatch/test_klp_convert1.c
 create mode 100644 lib/livepatch/test_klp_convert2.c
 create mode 100644 lib/livepatch/test_klp_convert_mod_a.c
 create mode 100644 lib/livepatch/test_klp_convert_mod_b.c

diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
index 513d200b7942..b19253c72d72 100644
--- a/lib/livepatch/Makefile
+++ b/lib/livepatch/Makefile
@@ -5,6 +5,8 @@
 LIVEPATCH_test_klp_atomic_replace := y
 LIVEPATCH_test_klp_callbacks_demo := y
 LIVEPATCH_test_klp_callbacks_demo2 := y
+LIVEPATCH_test_klp_convert1 := y
+LIVEPATCH_test_klp_convert2 := y
 LIVEPATCH_test_klp_livepatch := y
 
 obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
@@ -12,9 +14,17 @@ obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
 				test_klp_callbacks_demo2.o \
 				test_klp_callbacks_busy.o \
 				test_klp_callbacks_mod.o \
+				test_klp_convert1.o \
+				test_klp_convert2.o \
+				test_klp_convert_mod.o \
 				test_klp_livepatch.o \
 				test_klp_shadow_vars.o
 
+test_klp_convert_mod-y := \
+	test_klp_convert_mod_a.o \
+	test_klp_convert_mod_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)
diff --git a/lib/livepatch/test_klp_convert1.c b/lib/livepatch/test_klp_convert1.c
new file mode 100644
index 000000000000..a359f69a8fdc
--- /dev/null
+++ b/lib/livepatch/test_klp_convert1.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <joe.lawrence@redhat.com>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+/* klp-convert symbols - vmlinux */
+extern char *saved_command_line;
+/* klp-convert symbols - test_klp_convert_mod.ko */
+extern char driver_name[];
+extern char homonym_string[];
+extern const char *get_homonym_string(void);
+extern const char *get_driver_name(void);
+
+void print_saved_command_line(void)
+{
+	pr_info("saved_command_line, 0: %s\n", saved_command_line);
+}
+
+void print_driver_name(void)
+{
+	pr_info("driver_name, 0: %s\n", driver_name);
+	pr_info("get_driver_name(), 0: %s\n", get_driver_name());
+}
+
+void print_homonym_string(void)
+{
+	pr_info("homonym_string, 1: %s\n", homonym_string);
+	pr_info("get_homonym_string(), 1: %s\n", get_homonym_string());
+}
+
+/*
+ * saved_command_line is a unique symbol, so the sympos annotation is
+ * optional.  Provide to test that sympos=0 works correctly.
+ */
+KLP_MODULE_RELOC(vmlinux) vmlinux_relocs[] = {
+	KLP_SYMPOS(saved_command_line, 0)
+};
+
+/*
+ * driver_name symbols can be found in vmlinux (multiple) and also
+ * test_klp_convert_mod, therefore the annotation is required to
+ * clarify that we want the one from test_klp_convert_mod.
+ *
+ * test_klp_convert_mod contains multiple homonym_string and
+ * get_homonym_string symbols, test resolving the first set here and
+ *  the others in test_klp_convert2.c
+ *
+ * get_driver_name is a uniquely named symbol, test that sympos=0
+ * work correctly.
+ */
+KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
+	KLP_SYMPOS(driver_name, 0),
+	KLP_SYMPOS(homonym_string, 1),
+	KLP_SYMPOS(get_homonym_string, 1),
+	KLP_SYMPOS(get_driver_name, 0),
+};
+
+static struct klp_func funcs[] = {
+	{
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		/* name being NULL means vmlinux */
+		.funcs = funcs,
+	},
+	{
+		.name = "test_klp_convert_mod",
+		.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;
+
+	print_saved_command_line();
+	print_driver_name();
+	print_homonym_string();
+
+	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@redhat.com>");
+MODULE_DESCRIPTION("Livepatch test: klp-convert1");
diff --git a/lib/livepatch/test_klp_convert2.c b/lib/livepatch/test_klp_convert2.c
new file mode 100644
index 000000000000..7be9b2184fe1
--- /dev/null
+++ b/lib/livepatch/test_klp_convert2.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <joe.lawrence@redhat.com>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+/* klp-convert symbols - vmlinux */
+extern char *saved_command_line;
+/* klp-convert symbols - test_klp_convert_mod.ko */
+extern char driver_name[];
+extern char homonym_string[];
+extern const char *get_homonym_string(void);
+extern const char *get_driver_name(void);
+
+void print_saved_command_line(void)
+{
+	pr_info("saved_command_line (auto): %s\n", saved_command_line);
+}
+
+void print_driver_name(void)
+{
+	pr_info("driver_name, 0: %s\n", driver_name);
+	pr_info("get_driver_name(), (auto): %s\n", get_driver_name());
+}
+
+void print_homonym_string(void)
+{
+	pr_info("homonym_string, 2: %s\n", homonym_string);
+	pr_info("get_homonym_string(), 2: %s\n", get_homonym_string());
+}
+
+/*
+ * saved_command_line is a uniquely named symbol, so the sympos
+ * annotation is optional.  Skip it and test that klp-convert can
+ * resolve the symbol on its own.
+ */
+
+/*
+ * driver_name symbols can be found in vmlinux (multiple) and also
+ * test_klp_convert_mod, therefore the annotation is required to
+ * clarify that we want the one from test_klp_convert_mod.
+ *
+ * test_klp_convert_mod contains multiple homonym_string symbols,
+ * test_klp_convert1.c resolved to the first one, resolve to the
+ * second one here.
+ *
+ * get_driver_name is a uniquely named symbol, test klp-convert can
+ * resolve it automatically.
+ */
+KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
+	KLP_SYMPOS(driver_name, 0),
+	KLP_SYMPOS(homonym_string, 2),
+	KLP_SYMPOS(get_homonym_string, 2),
+};
+
+static struct klp_func funcs[] = {
+	{
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		/* name being NULL means vmlinux */
+		.funcs = funcs,
+	},
+	{
+		.name = "test_klp_convert_mod",
+		.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;
+
+	print_saved_command_line();
+	print_driver_name();
+	print_homonym_string();
+
+	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@redhat.com>");
+MODULE_DESCRIPTION("Livepatch test: klp-convert2");
diff --git a/lib/livepatch/test_klp_convert_mod_a.c b/lib/livepatch/test_klp_convert_mod_a.c
new file mode 100644
index 000000000000..b2db9942fbd2
--- /dev/null
+++ b/lib/livepatch/test_klp_convert_mod_a.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <joe.lawrence@redhat.com>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+
+/* Unique symbols that don't need sympos annotation */
+static const char driver_name[] = KBUILD_MODNAME;
+__used static const char *get_driver_name(void)
+{
+	return driver_name;
+}
+
+/* Common symbol names that need sympos */
+static const char homonym_string[] = "homonym string A";
+__used static const char *get_homonym_string(void)
+{
+	return homonym_string;
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
+MODULE_DESCRIPTION("Livepatch test: klp-convert module");
diff --git a/lib/livepatch/test_klp_convert_mod_b.c b/lib/livepatch/test_klp_convert_mod_b.c
new file mode 100644
index 000000000000..a3d529ff5f1f
--- /dev/null
+++ b/lib/livepatch/test_klp_convert_mod_b.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <joe.lawrence@redhat.com>
+
+/*
+ * A second compilation unit to provide another set of similarly named
+ * symbols, forcing a livepatch to use sympos annotations.
+ */
+
+static char homonym_string[] = "homonym string B";
+__used static char *get_homonym_string(void)
+{
+	return homonym_string;
+}
diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index f05268aea859..9c3745e8ba1d 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -6,6 +6,9 @@
 
 MOD_LIVEPATCH=test_klp_livepatch
 MOD_REPLACE=test_klp_atomic_replace
+MOD_KLP_CONVERT_MOD=test_klp_convert_mod
+MOD_KLP_CONVERT1=test_klp_convert1
+MOD_KLP_CONVERT2=test_klp_convert2
 
 set_dynamic_debug
 
@@ -165,4 +168,65 @@ livepatch: '$MOD_REPLACE': unpatching complete
 % rmmod $MOD_REPLACE"
 
 
+# TEST: klp-convert symbols
+# - load a livepatch that modifies the output from /proc/cmdline
+#   including a reference to vmlinux-local symbol that klp-convert
+#   will process
+# - verify correct behavior
+# - unload the livepatch and make sure the patch was removed
+
+echo -n "TEST: klp-convert symbols ... "
+dmesg -C
+
+saved_cmdline=$(cat /proc/cmdline)
+
+load_mod $MOD_KLP_CONVERT_MOD
+
+load_lp $MOD_KLP_CONVERT1
+disable_lp $MOD_KLP_CONVERT1
+unload_lp $MOD_KLP_CONVERT1
+
+load_lp $MOD_KLP_CONVERT2
+disable_lp $MOD_KLP_CONVERT2
+unload_lp $MOD_KLP_CONVERT2
+
+unload_mod $MOD_KLP_CONVERT_MOD
+
+check_result "% modprobe $MOD_KLP_CONVERT_MOD
+% modprobe $MOD_KLP_CONVERT1
+livepatch: enabling patch '$MOD_KLP_CONVERT1'
+livepatch: '$MOD_KLP_CONVERT1': initializing patching transition
+livepatch: '$MOD_KLP_CONVERT1': starting patching transition
+$MOD_KLP_CONVERT1: saved_command_line, 0: $saved_cmdline
+$MOD_KLP_CONVERT1: driver_name, 0: $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT1: get_driver_name(), 0: $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT1: homonym_string, 1: homonym string A
+$MOD_KLP_CONVERT1: get_homonym_string(), 1: homonym string A
+livepatch: '$MOD_KLP_CONVERT1': completing patching transition
+livepatch: '$MOD_KLP_CONVERT1': patching complete
+% echo 0 > /sys/kernel/livepatch/$MOD_KLP_CONVERT1/enabled
+livepatch: '$MOD_KLP_CONVERT1': initializing unpatching transition
+livepatch: '$MOD_KLP_CONVERT1': starting unpatching transition
+livepatch: '$MOD_KLP_CONVERT1': completing unpatching transition
+livepatch: '$MOD_KLP_CONVERT1': unpatching complete
+% rmmod $MOD_KLP_CONVERT1
+% modprobe $MOD_KLP_CONVERT2
+livepatch: enabling patch '$MOD_KLP_CONVERT2'
+livepatch: '$MOD_KLP_CONVERT2': initializing patching transition
+livepatch: '$MOD_KLP_CONVERT2': starting patching transition
+$MOD_KLP_CONVERT2: saved_command_line (auto): $saved_cmdline
+$MOD_KLP_CONVERT2: driver_name, 0: $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT2: get_driver_name(), (auto): $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT2: homonym_string, 2: homonym string B
+$MOD_KLP_CONVERT2: get_homonym_string(), 2: homonym string B
+livepatch: '$MOD_KLP_CONVERT2': completing patching transition
+livepatch: '$MOD_KLP_CONVERT2': patching complete
+% echo 0 > /sys/kernel/livepatch/$MOD_KLP_CONVERT2/enabled
+livepatch: '$MOD_KLP_CONVERT2': initializing unpatching transition
+livepatch: '$MOD_KLP_CONVERT2': starting unpatching transition
+livepatch: '$MOD_KLP_CONVERT2': completing unpatching transition
+livepatch: '$MOD_KLP_CONVERT2': unpatching complete
+% rmmod $MOD_KLP_CONVERT2
+% rmmod $MOD_KLP_CONVERT_MOD"
+
 exit 0
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH v4 10/10] livepatch/klp-convert: abort on special sections
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (8 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 09/10] livepatch/selftests: add klp-convert Joe Lawrence
@ 2019-05-09 14:38 ` Joe Lawrence
  2019-06-13 13:00 ` [PATCH v4 00/10] klp-convert livepatch build tooling Miroslav Benes
  10 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-05-09 14:38 UTC (permalink / raw)
  To: linux-kernel, live-patching, linux-kbuild

To properly convert alternatives, paravirt ops, and static keys,
klp-convert needs to implement "klp.arch" sections as supported by
d4c3e6e1b193 (“livepatch/x86: apply alternatives and paravirt patches
after relocations”).

There is some amount of ELF section bookkeeping required for this (ie,
moving data structure entries and relocations to their ".klp.arch"
equivalents) but the hardest part will be determining klp_object
relationships.  This may require some larger changes to livepatch API,
so defer support for now.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 scripts/livepatch/klp-convert.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 72b65428e738..b5873abecefb 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -617,6 +617,18 @@ static void free_converted_resources(struct elf *klp_elf)
 	}
 }
 
+/* Check for special sections that klp-convert doesn't support */
+static bool is_section_supported(char *sname)
+{
+	if (strcmp(sname, ".rela.altinstructions") == 0)
+		return false;
+	if (strcmp(sname, ".rela.parainstructions") == 0)
+		return false;
+	if (strcmp(sname, ".rela__jump_table") == 0)
+		return false;
+	return true;
+}
+
 int main(int argc, const char **argv)
 {
 	const char *klp_in_module, *klp_out_module, *symbols_list;
@@ -649,6 +661,12 @@ int main(int argc, const char **argv)
 	}
 
 	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+		if (!is_section_supported(sec->name)) {
+			WARN("Special ELF section: %s not supported",
+				sec->name);
+			return -1;
+		}
+
 		if (!is_rela_section(sec) ||
 		    is_klp_rela_section(sec->name))
 			continue;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 02/10] kbuild: Support for Symbols.list creation
  2019-05-09 14:38 ` [PATCH v4 02/10] kbuild: Support for Symbols.list creation Joe Lawrence
@ 2019-05-21 13:48   ` Masahiro Yamada
  0 siblings, 0 replies; 42+ messages in thread
From: Masahiro Yamada @ 2019-05-21 13:48 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

On Thu, May 9, 2019 at 11:40 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> From: Joao Moreira <jmoreira@suse.de>
>
> For automatic resolution of livepatch relocations, a file called
> Symbols.list is used. This file maps symbols within every compiled
> kernel object allowing the identification of symbols whose name is
> unique, thus relocation can be automatically inferred, or providing
> information that helps developers when code annotation is required for
> solving the matter.
>
> Add support for creating Symbols.list in the main Makefile. First,
> ensure that built-in is compiled when CONFIG_LIVEPATCH is enabled (as
> required to achieve a complete Symbols.list file). Define the command to
> build Symbols.list (cmd_klp_map) and hook it in the modules rule.
>
> As it is undesirable to have symbols from livepatch objects inside
> Symbols.list, make livepatches discernible by modifying
> scripts/Makefile.build to create a .livepatch file for each livepatch
> in $(MODVERDIR). This file then used by cmd_klp_map to identify and
> bypass livepatches.
>
> For identifying livepatches during the build process, a flag variable
> LIVEPATCH_$(basetarget).o is considered in scripts/Makefile.build. This
> way, set this flag for the livepatch sample Makefile in
> samples/livepatch/Makefile.
>
> Finally, Add a clean rule to ensure that Symbols.list is removed during
> clean.
>
> Notes:
>
> To achieve a correct Symbols.list file, all kernel objects must be
> considered, thus, its construction require these objects to be priorly
> built. On the other hand, invoking scripts/Makefile.modpost without
> having a complete Symbols.list in place would occasionally lead to
> in-tree livepatches being post-processed incorrectly. To prevent this
> from becoming a circular dependency, the construction of Symbols.list
> uses non-post-processed kernel objects and such does not cause harm as
> the symbols normally referenced from within livepatches are visible at
> this stage. Also due to these requirements, the spot in-between modules
> compilation and the invocation of scripts/Makefile.modpost was picked
> for hooking cmd_klp_map.
>
> The approach based on .livepatch files was proposed as an alternative
> to using MODULE_INFO statements. This approach was originally
> proposed by Miroslav Benes as a workaround for identifying livepathes
> without depending on modinfo during the modpost stage. It was moved to
> this patch as the approach also shown to be useful while building
> Symbols.list.
>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  .gitignore                 |  1 +
>  Makefile                   | 30 ++++++++++++++++++++++++++----
>  lib/livepatch/Makefile     |  5 +++++
>  samples/livepatch/Makefile |  4 ++++
>  scripts/Makefile.build     |  7 +++++++
>  5 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index a20ac26aa2f5..5cd5758f5ffe 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -45,6 +45,7 @@
>  *.xz
>  Module.symvers
>  modules.builtin
> +Symbols.list


Symbols.list is created only in the top directory.

Please move this to

# Top-level generic files

section with a leading slash.



Also, you need to add it to
Documentation/dontdiff




>  #
>  # Top-level generic files
> diff --git a/Makefile b/Makefile
> index abe13538a8c0..98089f9d44fe 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -574,10 +574,13 @@ KBUILD_BUILTIN := 1
>  # If we have only "make modules", don't compile built-in objects.
>  # When we're building modules with modversions, we need to consider
>  # the built-in objects during the descend as well, in order to
> -# make sure the checksums are up to date before we record them.
> +# make sure the checksums are up to date before we record them. The
> +# same applies for building livepatches, as built-in objects may hold
> +# symbols which are referenced from livepatches and are required by
> +# klp-convert post-processing tool for resolving these cases.
>
>  ifeq ($(MAKECMDGOALS),modules)
> -  KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1)
> +  KBUILD_BUILTIN := $(if $(or $(CONFIG_MODVERSIONS), $(CONFIG_LIVEPATCH)),1)
>  endif
>
>  # If we have "make <whatever> modules", compile modules
> @@ -1261,9 +1264,25 @@ all: modules
>  # duplicate lines in modules.order files.  Those are removed
>  # using awk while concatenating to the final file.
>
> +quiet_cmd_klp_map = KLP     Symbols.list
> +SLIST = $(objtree)/Symbols.list


Please do not define SLIST.

Use Symbols.list directly.




> +
> +define cmd_klp_map
> +       $(shell echo "klp-convert-symbol-data.0.1" > $(SLIST))                          \
> +       $(shell echo "*vmlinux" >> $(SLIST))                                            \
> +       $(shell nm -f posix $(objtree)/vmlinux | cut -d\  -f1 >> $(SLIST))              \
> +       $(foreach m, $(wildcard $(MODVERDIR)/*.mod),                                    \
> +               $(eval mod = $(patsubst %.ko,%.o,$(shell head -n1 $(m))))               \
> +               $(if $(wildcard $(MODVERDIR)/$(shell basename -s .o $(mod)).livepatch),,\
> +                       $(eval fmod = $(subst $(quote),_,$(subst -,_,$(mod))))          \
> +                       $(shell echo "*$(shell basename -s .o $(fmod))" >> $(SLIST))    \
> +                       $(shell nm -f posix $(mod) | cut -d\  -f1 >> $(SLIST))))
> +endef
> +
>  PHONY += modules
>  modules: $(vmlinux-dirs) $(if $(KBUILD_BUILTIN),vmlinux) modules.builtin
>         $(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > $(objtree)/modules.order
> +       $(if $(CONFIG_LIVEPATCH), $(call cmd,klp_map))
>         @$(kecho) '  Building modules, stage 2.';
>         $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
>
> @@ -1350,7 +1369,7 @@ clean: rm-dirs  := $(CLEAN_DIRS)
>  clean: rm-files := $(CLEAN_FILES)
>  clean-dirs      := $(addprefix _clean_, . $(vmlinux-alldirs) Documentation samples)
>
> -PHONY += $(clean-dirs) clean archclean vmlinuxclean
> +PHONY += $(clean-dirs) clean archclean vmlinuxclean klpclean
>  $(clean-dirs):
>         $(Q)$(MAKE) $(clean)=$(patsubst _clean_%,%,$@)
>
> @@ -1358,7 +1377,10 @@ vmlinuxclean:
>         $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
>         $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
>
> -clean: archclean vmlinuxclean
> +klpclean:
> +       $(Q) rm -f $(SLIST)


klpclean is unneeded.

Add it to CLEAN_FILES

CLEAN_FILES += modules.builtin.modinfo Systems.list




-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
                   ` (9 preceding siblings ...)
  2019-05-09 14:38 ` [PATCH v4 10/10] livepatch/klp-convert: abort on special sections Joe Lawrence
@ 2019-06-13 13:00 ` Miroslav Benes
  2019-06-13 13:15   ` Joe Lawrence
  10 siblings, 1 reply; 42+ messages in thread
From: Miroslav Benes @ 2019-06-13 13:00 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: linux-kernel, live-patching, linux-kbuild

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?

Regards,
Miroslav

PS: it is probably not a coincidence that I come across selftests failures 
right before I leave for a holiday...

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-13 13:00 ` [PATCH v4 00/10] klp-convert livepatch build tooling Miroslav Benes
@ 2019-06-13 13:15   ` Joe Lawrence
  2019-06-13 20:48     ` Joe Lawrence
  0 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-06-13 13:15 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: linux-kernel, live-patching, linux-kbuild

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.

> Regards,
> Miroslav
> 
> PS: it is probably not a coincidence that I come across selftests failures
> right before I leave for a holiday...
> 

Are you volunteering to go on holidays for each patchset until all of 
the selftests pass? :)

-- Joe

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-13 13:15   ` Joe Lawrence
@ 2019-06-13 20:48     ` Joe Lawrence
  2019-06-14  8:34       ` Petr Mladek
  0 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-06-13 20:48 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: linux-kernel, live-patching, linux-kbuild

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()")


FWIW, my test_klp_convert1.ko has these klp-converted relocations:

Relocation section [36] '.klp.rela.vmlinux..text.unlikely' for section [ 5] '.text.unlikely' at offset 0x4a6b8 contains 1 entry:                                   
  Offset              Type            Value               Addend Name                                                                                              
  0x0000000000000003  X86_64_PC32     000000000000000000      -4 .klp.sym.vmlinux.saved_command_line,0 

Relocation section [37] '.klp.rela.test_klp_convert_mod..text.unlikely' for section [ 5] '.text.unlikely' at offset 0x4a6d0 contains 4 entries:                    
  Offset              Type            Value               Addend Name                                                                                              
  0x000000000000004e  X86_64_PC32     000000000000000000      -4 .klp.sym.test_klp_convert_mod.get_homonym_string,1                                                
  0x000000000000003d  X86_64_32S      000000000000000000      +0 .klp.sym.test_klp_convert_mod.homonym_string,1                                                    
  0x0000000000000027  X86_64_PC32     000000000000000000      -4 .klp.sym.test_klp_convert_mod.get_driver_name,0                                                   
  0x0000000000000016  X86_64_32S      000000000000000000      +0 .klp.sym.test_klp_convert_mod.driver_name,0

-- Joe

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-13 20:48     ` Joe Lawrence
@ 2019-06-14  8:34       ` Petr Mladek
  2019-06-14 14:20         ` Joe Lawrence
  2019-06-25 11:36         ` Miroslav Benes
  0 siblings, 2 replies; 42+ messages in thread
From: Petr Mladek @ 2019-06-14  8:34 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: Miroslav Benes, linux-kernel, live-patching, linux-kbuild

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 ;-)

Anyway, I am curious about one thing. I saw:

function __load_mod() {
	local mod="$1"; shift

	local msg="% modprobe $mod $*"
	log "${msg%% }"
	ret=$(modprobe "$mod" "$@" 2>&1)
	if [[ "$ret" != "" ]]; then
		die "$ret"
	fi

	# Wait for module in sysfs ...
	loop_until '[[ -e "/sys/module/$mod" ]]' ||
		die "failed to load module $mod"
}

Is the waiting for sysfs really necessary here?

Note that it is /sys/module and not /sys/kernel/livepatch/.

My understanding is that modprobe waits until the module succesfully
loaded. mod_sysfs_setup() is called before the module init callback.
Therefore the sysfs interface should be read before modprobe returns.
Do I miss something?

If it works different way then there might be some races because
mod_sysfs_setup() is called before the module is alive.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-14  8:34       ` Petr Mladek
@ 2019-06-14 14:20         ` Joe Lawrence
  2019-06-14 16:36           ` Libor Pechacek
  2019-06-25 11:36         ` Miroslav Benes
  1 sibling, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-06-14 14:20 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Miroslav Benes, linux-kernel, live-patching, linux-kbuild,
	Libor Pecháček

On 6/14/19 4:34 AM, 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 ;-)
> 
> Anyway, I am curious about one thing. I saw:
> 
> function __load_mod() {
> 	local mod="$1"; shift
> 
> 	local msg="% modprobe $mod $*"
> 	log "${msg%% }"
> 	ret=$(modprobe "$mod" "$@" 2>&1)
> 	if [[ "$ret" != "" ]]; then
> 		die "$ret"
> 	fi
> 
> 	# Wait for module in sysfs ...
> 	loop_until '[[ -e "/sys/module/$mod" ]]' ||
> 		die "failed to load module $mod"
> }
> 
> Is the waiting for sysfs really necessary here?
> 
> Note that it is /sys/module and not /sys/kernel/livepatch/.

I can't remember if that was just paranoid-protective-bash coding or 
actually required.  Libor provided great feedback on the initial patch 
series that introduced the self-tests, perhaps he remembers.

> My understanding is that modprobe waits until the module succesfully
> loaded. mod_sysfs_setup() is called before the module init callback.
> Therefore the sysfs interface should be read before modprobe returns.
> Do I miss something?
 >
> If it works different way then there might be some races because
> mod_sysfs_setup() is called before the module is alive.

All of this is called from a single bash script function, so in a call 
stack fashion, something like this would occur when loading a livepatch 
module:

   [ mod_sysfs_setup() ]
   modprobe waits for:         .init complete, MODULE_STATE_LIVE
   __load_mod() waits for:     /sys/module/$mod
   load_lp_nowait() waits for: /sys/kernel/livepatch/$mod
   load_lp() waits for:        /sys/kernel/livepatch/$mod/transition = 0
   test-script.sh

So I would think that by calling modprobe, we ensure that the module 
code is ready to go.  The /sys/module/$mod check might be redundant as 
you say, but because modprobe completed, we should be safe, no?

The only "nowait" function we have is load_lp_nowait(), which would let 
us march onward before the livepatch transition may have completed.

-- Joe

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-14 14:20         ` Joe Lawrence
@ 2019-06-14 16:36           ` Libor Pechacek
  0 siblings, 0 replies; 42+ messages in thread
From: Libor Pechacek @ 2019-06-14 16:36 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Petr Mladek, Miroslav Benes, linux-kernel, live-patching, linux-kbuild

On Fri 14-06-19 10:20:09, Joe Lawrence wrote:
> On 6/14/19 4:34 AM, Petr Mladek wrote:
[...]
> > Anyway, I am curious about one thing. I saw:
> > 
> > function __load_mod() {
> > 	local mod="$1"; shift
> > 
> > 	local msg="% modprobe $mod $*"
> > 	log "${msg%% }"
> > 	ret=$(modprobe "$mod" "$@" 2>&1)
> > 	if [[ "$ret" != "" ]]; then
> > 		die "$ret"
> > 	fi
> > 
> > 	# Wait for module in sysfs ...
> > 	loop_until '[[ -e "/sys/module/$mod" ]]' ||
> > 		die "failed to load module $mod"
> > }
> > 
> > Is the waiting for sysfs really necessary here?
> > 
> > Note that it is /sys/module and not /sys/kernel/livepatch/.
> 
> I can't remember if that was just paranoid-protective-bash coding or
> actually required.  Libor provided great feedback on the initial patch
> series that introduced the self-tests, perhaps he remembers.

I don't recall analyzing this spot in detail but looking at it now I don't see
anything wrong with it. While the check is likely superfluous, I'm not against
keeping it in place.

> > My understanding is that modprobe waits until the module succesfully
> > loaded. mod_sysfs_setup() is called before the module init callback.
> > Therefore the sysfs interface should be read before modprobe returns.
> > Do I miss something?
> >
> > If it works different way then there might be some races because
> > mod_sysfs_setup() is called before the module is alive.
> 
> All of this is called from a single bash script function, so in a call stack
> fashion, something like this would occur when loading a livepatch module:
> 
>   [ mod_sysfs_setup() ]
>   modprobe waits for:         .init complete, MODULE_STATE_LIVE
>   __load_mod() waits for:     /sys/module/$mod
>   load_lp_nowait() waits for: /sys/kernel/livepatch/$mod
>   load_lp() waits for:        /sys/kernel/livepatch/$mod/transition = 0
>   test-script.sh
> 
> So I would think that by calling modprobe, we ensure that the module code is
> ready to go.  The /sys/module/$mod check might be redundant as you say, but
> because modprobe completed, we should be safe, no?
> 
> The only "nowait" function we have is load_lp_nowait(), which would let us
> march onward before the livepatch transition may have completed.

And even that one is waiting for the live patch module name appear under
/sys/kernel/livepatch/. This is IMHO acceptable level of paranoia.

Libor
-- 
Libor Pechacek
SUSE Labs

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-14  8:34       ` Petr Mladek
  2019-06-14 14:20         ` Joe Lawrence
@ 2019-06-25 11:36         ` Miroslav Benes
  2019-06-25 13:24           ` Joe Lawrence
  2019-06-25 19:08           ` Joe Lawrence
  1 sibling, 2 replies; 42+ messages in thread
From: Miroslav Benes @ 2019-06-25 11:36 UTC (permalink / raw)
  To: Petr Mladek; +Cc: Joe Lawrence, linux-kernel, live-patching, linux-kbuild

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-25 11:36         ` Miroslav Benes
@ 2019-06-25 13:24           ` Joe Lawrence
  2019-06-25 19:08           ` Joe Lawrence
  1 sibling, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-06-25 13:24 UTC (permalink / raw)
  To: Miroslav Benes, Petr Mladek; +Cc: linux-kernel, live-patching, linux-kbuild

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-25 11:36         ` Miroslav Benes
  2019-06-25 13:24           ` Joe Lawrence
@ 2019-06-25 19:08           ` Joe Lawrence
  2019-06-26 10:27             ` Miroslav Benes
  1 sibling, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-06-25 19:08 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: Petr Mladek, linux-kernel, live-patching, linux-kbuild

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);
 		}
 	}
 

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 00/10] klp-convert livepatch build tooling
  2019-06-25 19:08           ` Joe Lawrence
@ 2019-06-26 10:27             ` Miroslav Benes
  0 siblings, 0 replies; 42+ messages in thread
From: Miroslav Benes @ 2019-06-26 10:27 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: Petr Mladek, linux-kernel, live-patching, linux-kbuild

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 03/10] livepatch: Add klp-convert tool
  2019-05-09 14:38 ` [PATCH v4 03/10] livepatch: Add klp-convert tool Joe Lawrence
@ 2019-07-31  2:50   ` Masahiro Yamada
  2019-07-31  3:36     ` Masahiro Yamada
  0 siblings, 1 reply; 42+ messages in thread
From: Masahiro Yamada @ 2019-07-31  2:50 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 03/10] livepatch: Add klp-convert tool
  2019-07-31  2:50   ` Masahiro Yamada
@ 2019-07-31  3:36     ` Masahiro Yamada
  2019-08-09 18:42       ` Joe Lawrence
  0 siblings, 1 reply; 42+ messages in thread
From: Masahiro Yamada @ 2019-07-31  3:36 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-05-09 14:38 ` [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules Joe Lawrence
@ 2019-07-31  5:58   ` Masahiro Yamada
  2019-08-12 15:56     ` Joe Lawrence
  2019-08-13 10:26     ` Miroslav Benes
  0 siblings, 2 replies; 42+ messages in thread
From: Masahiro Yamada @ 2019-07-31  5:58 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

[-- 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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 03/10] livepatch: Add klp-convert tool
  2019-07-31  3:36     ` Masahiro Yamada
@ 2019-08-09 18:42       ` Joe Lawrence
  2019-08-13  1:15         ` Masahiro Yamada
  0 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-08-09 18:42 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-07-31  5:58   ` Masahiro Yamada
@ 2019-08-12 15:56     ` Joe Lawrence
  2019-08-15 15:05       ` Masahiro Yamada
  2019-08-13 10:26     ` Miroslav Benes
  1 sibling, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-08-12 15:56 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 03/10] livepatch: Add klp-convert tool
  2019-08-09 18:42       ` Joe Lawrence
@ 2019-08-13  1:15         ` Masahiro Yamada
  0 siblings, 0 replies; 42+ messages in thread
From: Masahiro Yamada @ 2019-08-13  1:15 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-07-31  5:58   ` Masahiro Yamada
  2019-08-12 15:56     ` Joe Lawrence
@ 2019-08-13 10:26     ` Miroslav Benes
  1 sibling, 0 replies; 42+ messages in thread
From: Miroslav Benes @ 2019-08-13 10:26 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Joe Lawrence, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-12 15:56     ` Joe Lawrence
@ 2019-08-15 15:05       ` Masahiro Yamada
  2019-08-16  8:19         ` Miroslav Benes
  0 siblings, 1 reply; 42+ messages in thread
From: Masahiro Yamada @ 2019-08-15 15:05 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-15 15:05       ` Masahiro Yamada
@ 2019-08-16  8:19         ` Miroslav Benes
  2019-08-16 12:43           ` Joe Lawrence
  0 siblings, 1 reply; 42+ messages in thread
From: Miroslav Benes @ 2019-08-16  8:19 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Joe Lawrence, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 07/10] livepatch: Add sample livepatch module
  2019-05-09 14:38 ` [PATCH v4 07/10] livepatch: Add sample livepatch module Joe Lawrence
@ 2019-08-16 11:35   ` Masahiro Yamada
  2019-08-16 12:47     ` Joe Lawrence
  0 siblings, 1 reply; 42+ messages in thread
From: Masahiro Yamada @ 2019-08-16 11:35 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-16  8:19         ` Miroslav Benes
@ 2019-08-16 12:43           ` Joe Lawrence
  2019-08-16 19:01             ` Joe Lawrence
  2019-08-19  3:49             ` Masahiro Yamada
  0 siblings, 2 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-08-16 12:43 UTC (permalink / raw)
  To: Miroslav Benes, Masahiro Yamada
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 07/10] livepatch: Add sample livepatch module
  2019-08-16 11:35   ` Masahiro Yamada
@ 2019-08-16 12:47     ` Joe Lawrence
  0 siblings, 0 replies; 42+ messages in thread
From: Joe Lawrence @ 2019-08-16 12:47 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-16 12:43           ` Joe Lawrence
@ 2019-08-16 19:01             ` Joe Lawrence
  2019-08-19  3:50               ` Masahiro Yamada
  2019-08-19  3:49             ` Masahiro Yamada
  1 sibling, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-08-16 19:01 UTC (permalink / raw)
  To: Miroslav Benes, Masahiro Yamada
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-16 12:43           ` Joe Lawrence
  2019-08-16 19:01             ` Joe Lawrence
@ 2019-08-19  3:49             ` Masahiro Yamada
  2019-08-19  7:31               ` Miroslav Benes
  1 sibling, 1 reply; 42+ messages in thread
From: Masahiro Yamada @ 2019-08-19  3:49 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Miroslav Benes, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-16 19:01             ` Joe Lawrence
@ 2019-08-19  3:50               ` Masahiro Yamada
  2019-08-19 15:55                 ` Joe Lawrence
  0 siblings, 1 reply; 42+ messages in thread
From: Masahiro Yamada @ 2019-08-19  3:50 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Miroslav Benes, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-19  3:49             ` Masahiro Yamada
@ 2019-08-19  7:31               ` Miroslav Benes
  2019-08-19 16:02                 ` Joe Lawrence
  0 siblings, 1 reply; 42+ messages in thread
From: Miroslav Benes @ 2019-08-19  7:31 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Joe Lawrence, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-19  3:50               ` Masahiro Yamada
@ 2019-08-19 15:55                 ` Joe Lawrence
  2019-08-20  7:54                   ` Miroslav Benes
  0 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-08-19 15:55 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Miroslav Benes, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-19  7:31               ` Miroslav Benes
@ 2019-08-19 16:02                 ` Joe Lawrence
  2019-08-22  3:35                   ` Masahiro Yamada
  0 siblings, 1 reply; 42+ messages in thread
From: Joe Lawrence @ 2019-08-19 16:02 UTC (permalink / raw)
  To: Miroslav Benes, Masahiro Yamada
  Cc: Linux Kernel Mailing List, live-patching, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-19 15:55                 ` Joe Lawrence
@ 2019-08-20  7:54                   ` Miroslav Benes
  0 siblings, 0 replies; 42+ messages in thread
From: Miroslav Benes @ 2019-08-20  7:54 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Masahiro Yamada, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

> > 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
  2019-08-19 16:02                 ` Joe Lawrence
@ 2019-08-22  3:35                   ` Masahiro Yamada
  0 siblings, 0 replies; 42+ messages in thread
From: Masahiro Yamada @ 2019-08-22  3:35 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Miroslav Benes, Linux Kernel Mailing List, live-patching,
	Linux Kbuild mailing list

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

^ permalink raw reply related	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2019-08-22  3:36 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 01/10] livepatch: Create and include UAPI headers Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 02/10] kbuild: Support for Symbols.list creation Joe Lawrence
2019-05-21 13:48   ` Masahiro Yamada
2019-05-09 14:38 ` [PATCH v4 03/10] livepatch: Add klp-convert tool Joe Lawrence
2019-07-31  2:50   ` Masahiro Yamada
2019-07-31  3:36     ` Masahiro Yamada
2019-08-09 18:42       ` Joe Lawrence
2019-08-13  1:15         ` Masahiro Yamada
2019-05-09 14:38 ` [PATCH v4 04/10] livepatch: Add klp-convert annotation helpers Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 05/10] modpost: Integrate klp-convert Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules Joe Lawrence
2019-07-31  5:58   ` Masahiro Yamada
2019-08-12 15:56     ` Joe Lawrence
2019-08-15 15:05       ` Masahiro Yamada
2019-08-16  8:19         ` Miroslav Benes
2019-08-16 12:43           ` Joe Lawrence
2019-08-16 19:01             ` Joe Lawrence
2019-08-19  3:50               ` Masahiro Yamada
2019-08-19 15:55                 ` Joe Lawrence
2019-08-20  7:54                   ` Miroslav Benes
2019-08-19  3:49             ` Masahiro Yamada
2019-08-19  7:31               ` Miroslav Benes
2019-08-19 16:02                 ` Joe Lawrence
2019-08-22  3:35                   ` Masahiro Yamada
2019-08-13 10:26     ` Miroslav Benes
2019-05-09 14:38 ` [PATCH v4 07/10] livepatch: Add sample livepatch module Joe Lawrence
2019-08-16 11:35   ` Masahiro Yamada
2019-08-16 12:47     ` Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 08/10] documentation: Update on livepatch elf format Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 09/10] livepatch/selftests: add klp-convert Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 10/10] livepatch/klp-convert: abort on special sections Joe Lawrence
2019-06-13 13:00 ` [PATCH v4 00/10] klp-convert livepatch build tooling Miroslav Benes
2019-06-13 13:15   ` Joe Lawrence
2019-06-13 20:48     ` Joe Lawrence
2019-06-14  8:34       ` Petr Mladek
2019-06-14 14:20         ` Joe Lawrence
2019-06-14 16:36           ` Libor Pechacek
2019-06-25 11:36         ` Miroslav Benes
2019-06-25 13:24           ` Joe Lawrence
2019-06-25 19:08           ` Joe Lawrence
2019-06-26 10:27             ` Miroslav Benes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).