Re: [PATCH v2 1/8] livepatch: Create and include UAPI headers

Re: [PATCH v2 1/8] livepatch: Create and include UAPI headers

[Miroslav Benes]

> diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
> new file mode 100644
> index 000000000000..bc35f85fd859
> --- /dev/null
> +++ b/include/uapi/linux/livepatch.h
> @@ -0,0 +1,28 @@
> +/*
> + * livepatch.h - Kernel Live Patching Core
> + *
> + * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@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/>.
> + */

People will ask to replace GPL boilerplate with SPDX tag.

> +#ifndef _UAPI_LIVEPATCH_H
> +#define _UAPI_LIVEPATCH_H
> +
> +#include <linux/types.h>

#include is not necessary here, I think.

> +#define KLP_RELA_PREFIX		".klp.rela."
> +#define KLP_SYM_PREFIX		".klp.sym."
> +
> +#endif /* _UAPI_LIVEPATCH_H */

Miroslav

Re: [PATCH v2 2/8] kbuild: Support for Symbols.list creation

[Miroslav Benes]

More CCs added. I'd give you lore.kernel.org link for the whole patch set 
(and cover letter), but the patch set is not archived for some strange 
reason.

On Wed, 30 Jan 2019, Joao Moreira wrote:

> 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 statementes. This approach was originally
> proposed by Miroslav Benes as a workaround for identifying livepathces
> 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.

If I remember correctly, the approach was originally proposed by Michal 
Marek. I only wanted to avoid modinfo.

> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  .gitignore                 |  1 +
>  Makefile                   | 28 +++++++++++++++++++++++++---
>  samples/livepatch/Makefile |  1 +
>  scripts/Makefile.build     |  6 ++++++
>  4 files changed, 33 insertions(+), 3 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 f5b1d0d168e0..8903d72793b4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -560,10 +560,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
> @@ -1255,9 +1258,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
>  
> @@ -1352,7 +1371,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 $(objtree)/Symbols.list
> +
> +clean: archclean vmlinuxclean klpclean
>  
>  # mrproper - Delete all generated files, including .config
>  #
> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> index 2472ce39a18d..9705df7f9a86 100644
> --- a/samples/livepatch/Makefile
> +++ b/samples/livepatch/Makefile
> @@ -1,3 +1,4 @@
> +LIVEPATCH_livepatch-sample.o := y
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o

We should eventually add the same flag even for the rest of samples.

> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index fd03d60f6c5a..1e28ad21314c 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -247,6 +247,11 @@ cmd_gen_ksymdeps = \
>  	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
>  endif
>  
> +ifdef CONFIG_LIVEPATCH
> +cmd_livepatch = $(if $(LIVEPATCH_$(basetarget).o),			\
> +	$(shell touch $(MODVERDIR)/$(basetarget).livepatch))
> +endif
> +
>  define rule_cc_o_c
>  	$(call cmd,checksrc)
>  	$(call cmd_and_fixdep,cc_o_c)
> @@ -283,6 +288,7 @@ $(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) F
>  	$(call if_changed_rule,cc_o_c)
>  	@{ echo $(@:.o=.ko); echo $@; \
>  	   $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
> +	$(call cmd_livepatch)
>  
>  quiet_cmd_cc_lst_c = MKLST   $@
>        cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
> -- 
> 2.16.4
> 

Miroslav

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Miroslav Benes]

Adding CCs...

On Wed, 30 Jan 2019, Joao Moreira wrote:

> 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.
> 
> [khlebnikov:
> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> * 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
> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> ]
> 
> [mbenes:
> * remove modinfo call. LIVEPATCH_ in Makefiled
> ]
> 
> [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.
> ]
> 
> 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>

I'm definitely not an expert on this topic (and we talked about it in the 
past off-list), but I'd simplify the SoB chain. At least you don't need my 
SoB there. I'm happy that you took the code and transformed it.

> ---
>  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 30816037036e..c30a8573c816 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -239,6 +239,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 $^),$^)
> @@ -246,7 +248,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 7d4af0d0accb..da779a185218 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>                   -o $@ $(filter-out FORCE,$^) ;                         \
>  	$(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
> +	$(call 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 26bf886bd168..1dfc34d8b668 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1979,6 +1979,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.modinfo, info.modinfo_len, "livepatch"))
> +		mod->livepatch = 1;
> +
>  	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
>  		symname = remove_dot(info.strtab + sym->st_name);
>  
> @@ -2106,7 +2110,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.16.4
> 

Miroslav

Re: [PATCH v2 6/8] modpost: Add modinfo flag to livepatch modules

[Miroslav Benes]

Adding CCs...

On Wed, 30 Jan 2019, Joao Moreira 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.
> 
> [jmoreira:
> * fix modpost.c (add_livepatch_flag) to update module structure with
> livepatch flag and prevent modpost from breaking due to unresolved
> symbols]
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  samples/livepatch/livepatch-sample.c |  1 -
>  scripts/Makefile.modpost             |  8 +++-
>  scripts/mod/modpost.c                | 84 +++++++++++++++++++++++++++++++++---
>  3 files changed, 85 insertions(+), 8 deletions(-)
> 
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> index 2d554dd930e2..20a5891ebf7b 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -90,4 +90,3 @@ static void livepatch_exit(void)
>  module_init(livepatch_init);
>  module_exit(livepatch_exit);
>  MODULE_LICENSE("GPL");
> -MODULE_INFO(livepatch, "Y");

Again, we shoud do the same for the other samples we have in 
samples/livepatch/ directory.

> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index da779a185218..0149a72ea7ae 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))
>  
> @@ -79,7 +84,8 @@ modpost = scripts/mod/modpost                    \
>   $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
>   $(if $(CONFIG_DEBUG_SECTION_MISMATCH),,-S)      \
>   $(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 1dfc34d8b668..2fd1d409ca5a 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1979,10 +1979,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.modinfo, info.modinfo_len, "livepatch"))
> -		mod->livepatch = 1;
> -
>  	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
>  		symname = remove_dot(info.strtab + sym->st_name);
>  
> @@ -2421,6 +2417,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;
> @@ -2436,8 +2502,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:mnsST:o:awE")) != -1) {
> +	while ((opt = getopt(argc, argv, "i:I:e:l:mnsST:o:awM:K:E")) != -1) {
>  		switch (opt) {
>  		case 'i':
>  			kernel_read = optarg;
> @@ -2454,6 +2521,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;
> @@ -2514,15 +2584,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);
> @@ -2541,6 +2612,7 @@ int main(int argc, char **argv)
>  			      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
>  		}
>  	}
> +	free_livepatch_mods(livepatch_mods);
>  	free(buf.p);
>  
>  	return err;
> -- 
> 2.16.4
> 

Miroslav

Re: [PATCH v2 6/8] modpost: Add modinfo flag to livepatch modules

[Josh Poimboeuf]

On Wed, Feb 20, 2019 at 04:50:08PM +0100, Miroslav Benes wrote:
> Adding CCs...

Where was this posted?  I don't see it on lkml.  It would be good to be
able to see the whole series for review.

> 
> On Wed, 30 Jan 2019, Joao Moreira 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.
> > 
> > [jmoreira:
> > * fix modpost.c (add_livepatch_flag) to update module structure with
> > livepatch flag and prevent modpost from breaking due to unresolved
> > symbols]
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > Signed-off-by: Joao Moreira <jmoreira@suse.de>
> > ---
> >  samples/livepatch/livepatch-sample.c |  1 -
> >  scripts/Makefile.modpost             |  8 +++-
> >  scripts/mod/modpost.c                | 84 +++++++++++++++++++++++++++++++++---
> >  3 files changed, 85 insertions(+), 8 deletions(-)
> > 
> > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> > index 2d554dd930e2..20a5891ebf7b 100644
> > --- a/samples/livepatch/livepatch-sample.c
> > +++ b/samples/livepatch/livepatch-sample.c
> > @@ -90,4 +90,3 @@ static void livepatch_exit(void)
> >  module_init(livepatch_init);
> >  module_exit(livepatch_exit);
> >  MODULE_LICENSE("GPL");
> > -MODULE_INFO(livepatch, "Y");
> 
> Again, we shoud do the same for the other samples we have in 
> samples/livepatch/ directory.
> 
> > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > index da779a185218..0149a72ea7ae 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))
> >  
> > @@ -79,7 +84,8 @@ modpost = scripts/mod/modpost                    \
> >   $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
> >   $(if $(CONFIG_DEBUG_SECTION_MISMATCH),,-S)      \
> >   $(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 1dfc34d8b668..2fd1d409ca5a 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1979,10 +1979,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.modinfo, info.modinfo_len, "livepatch"))
> > -		mod->livepatch = 1;
> > -
> >  	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
> >  		symname = remove_dot(info.strtab + sym->st_name);
> >  
> > @@ -2421,6 +2417,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;
> > @@ -2436,8 +2502,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:mnsST:o:awE")) != -1) {
> > +	while ((opt = getopt(argc, argv, "i:I:e:l:mnsST:o:awM:K:E")) != -1) {
> >  		switch (opt) {
> >  		case 'i':
> >  			kernel_read = optarg;
> > @@ -2454,6 +2521,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;
> > @@ -2514,15 +2584,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);
> > @@ -2541,6 +2612,7 @@ int main(int argc, char **argv)
> >  			      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
> >  		}
> >  	}
> > +	free_livepatch_mods(livepatch_mods);
> >  	free(buf.p);
> >  
> >  	return err;
> > -- 
> > 2.16.4
> > 
> 
> Miroslav

-- 
Josh

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Joao Moreira]

On 4/4/19 8:31 AM, Miroslav Benes wrote:
> On Fri, 22 Mar 2019, Joe Lawrence wrote:
> 
>> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
>>> 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.
>>>
>>> [khlebnikov:
>>> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
>>> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
>>> * 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
>>> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
>>> ]
>>>
>>> [mbenes:
>>> * remove modinfo call. LIVEPATCH_ in Makefiled
>>> ]
>>>
>>> [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.
>>> ]
>>>
>>> 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>
>>> ---
>>>   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(-)
>>>
>>>
>>> [ ... snip ... ]
>>>
>>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>>> index 7d4af0d0accb..da779a185218 100644
>>> --- a/scripts/Makefile.modpost
>>> +++ b/scripts/Makefile.modpost
>>> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>>>                    -o $@ $(filter-out FORCE,$^) ;                         \
>>>   	$(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
>>> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>>                             ^
>> Should there be a ';' semicolon here (and maybe a line-break) between
>> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?
> 
> Probably yes.
> 
> Originally there was
> 	$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ;
> 
> but "echo-" disappeared somewhere. Joao?

Hi,

I just verified my sources and the echo-cmd got lost in between the v2 
rebases. I believe it must have happened while I was manually fixing a 
conflict, or something. Sorry.

Here is a piece of the original snippet, as it was supposed to be:

define rule_ld_ko_o 

        $(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ; 
\
        $(call save-cmd,ld_ko_o) ; 
\
        $(if $(CONFIG_LIVEPATCH),                                       \

Joe, do you mind fixing it for v3?

Thanks for spotting it.

> 
> So maybe even
> 	$(call cmd,ld_ko_o)
> could be enough, because cmd contains echo-cmd, but I'm not good at
> reading scripts/Kbuild.include.
>   
>> I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
>> really strange build errors that I traced to this line.  Without a
>> semicolon, the build was trying to run a make command with a linker
>> command smashed onto the end of it:
>>
>>    make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m elf64lppc -T ./scripts/module-common.lds -T ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o crypto/xts.ko crypto/xts.o crypto/xts.mod.o
> 
> Miroslav
> 

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Miroslav Benes]

On Fri, 22 Mar 2019, Joe Lawrence wrote:

> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
> > 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.
> > 
> > [khlebnikov:
> > * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> > * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> > * 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
> > * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> > ]
> > 
> > [mbenes:
> > * remove modinfo call. LIVEPATCH_ in Makefiled
> > ]
> > 
> > [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.
> > ]
> > 
> > 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>
> > ---
> >  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(-)
> > 
> >
> > [ ... snip ... ]
> >
> > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > index 7d4af0d0accb..da779a185218 100644
> > --- a/scripts/Makefile.modpost
> > +++ b/scripts/Makefile.modpost
> > @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
> >                   -o $@ $(filter-out FORCE,$^) ;                         \
> >  	$(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
> > +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>                            ^
> Should there be a ';' semicolon here (and maybe a line-break) between
> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?

Probably yes.

Originally there was
	$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ;

but "echo-" disappeared somewhere. Joao?

So maybe even
	$(call cmd,ld_ko_o)
could be enough, because cmd contains echo-cmd, but I'm not good at 
reading scripts/Kbuild.include.
 
> I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
> really strange build errors that I traced to this line.  Without a
> semicolon, the build was trying to run a make command with a linker
> command smashed onto the end of it:
> 
>   make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m elf64lppc -T ./scripts/module-common.lds -T ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o crypto/xts.ko crypto/xts.o crypto/xts.mod.o

Miroslav

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Joe Lawrence]

On 3/22/19 12:37 PM, Joao Moreira wrote:
> On 2019-03-22 11:54, Joe Lawrence wrote:
>> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
>>> 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.
>>>
>>> [khlebnikov:
>>> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
>>> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
>>> * 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
>>> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
>>> ]
>>>
>>> [mbenes:
>>> * remove modinfo call. LIVEPATCH_ in Makefiled
>>> ]
>>>
>>> [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.
>>> ]
>>>
>>> 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>
>>> ---
>>>   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(-)
>>>
>>>
>>> [ ... snip ... ]
>>>
>>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>>> index 7d4af0d0accb..da779a185218 100644
>>> --- a/scripts/Makefile.modpost
>>> +++ b/scripts/Makefile.modpost
>>> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>>>                    -o $@ $(filter-out FORCE,$^) ;
>>>    \
>>>   	$(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
>>> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>>                             ^
>> Should there be a ';' semicolon here (and maybe a line-break) between
>> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?
>>
>> I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
>> really strange build errors that I traced to this line.  Without a
>> semicolon, the build was trying to run a make command with a linker
>> command smashed onto the end of it:
>>
>>    make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m
>> elf64lppc -T ./scripts/module-common.lds -T
>> ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o
>> crypto/xts.ko crypto/xts.o crypto/xts.mod.o
>>
>>
>> Now, klp-convert looks like will need some ppc64le work as well, as
>> it's
>> confused about .TOC. symbols:
>>
>>      CC      samples/livepatch/livepatch-annotated-sample.mod.o
>>      CC      samples/livepatch/livepatch-callbacks-busymod.mod.o
>>      CC      samples/livepatch/livepatch-callbacks-demo.mod.o
>>      CC      samples/livepatch/livepatch-callbacks-mod.mod.o
>>      CC      samples/livepatch/livepatch-sample.mod.o
>>      CC      samples/livepatch/livepatch-shadow-fix1.mod.o
>>      CC      samples/livepatch/livepatch-shadow-fix2.mod.o
>>      CC      samples/livepatch/livepatch-shadow-mod.mod.o
>>      LD [M]  samples/livepatch/livepatch-annotated-sample.ko
>>      LD [M]  samples/livepatch/livepatch-callbacks-demo.ko
>>      KLP     samples/livepatch/livepatch-annotated-sample.ko
>>      LD [M]  samples/livepatch/livepatch-callbacks-mod.ko
>>      KLP     samples/livepatch/livepatch-callbacks-demo.ko
>>      LD [M]  samples/livepatch/livepatch-sample.ko
>>      LD [M]  samples/livepatch/livepatch-shadow-fix1.ko
>>    klp-convert: Define KLP_SYMPOS for the symbol: .TOC.
>>    Valid KLP_SYMPOS for symbol .TOC.:
>>
>>    [ ... snip listing of all .TOC's across the kernel ... ]
>>
>> but we can save that for another day.
>>
> 
> Hi Joe, first of all, thank you for you in-depth review. I did not have
> the time to go through everything yet, and I'll reply properly to the
> other comments soon. Yet, I would like to add a quick note here, since
> you are testing ppc64le. ppc64le is exactly one of the things I have
> been dealing with in the last two days, specifically the .TOC. symbols
> (there is also another bug related to converting relocations of symbols
> with index 0, but this one is architecture agnostic).
> 
> If you would like to take a look, the fix is in the latest commit here:
> - https://github.com/SUSE/klp-convert/tree/fixppc64le
> 
> I would be very glad if you have any comment or input on that, and I
> guess that if the fix is that simple, I can manage to squash it into v3.

Hi João,

I added https://github.com/SUSE/klp-convert/commit/69b89ef5c366 on top 
of my 
https://github.com/joe-lawrence/linux/tree/klp-convert-v2-rebase-review 
branch with positive results:

(No build klp-convert build complaints)

   % uname -r
   Linux ibm-p9z-21-lp26.mpc.lab.eng.bos.redhat.com 5.1.0-rc1+ #6 SMP 
Thu Mar 21 11:40:24 EDT 2019 ppc64le ppc64le ppc64le GNU/Linux

   % insmod samples/livepatch/livepatch-annotated-sample.ko
   % grep -o 'livepatch.*' /proc/cmdline
   livepatch=1

   % ./tools/testing/selftests/livepatch/test-livepatch.sh
   TEST: basic function patching ... ok
   TEST: multiple livepatches ... ok
   TEST: atomic replace livepatch ... ok
   TEST: klp-convert symbols ... ok

There may be more ppc64le specifics lurking, but if we could get samples 
and self-tests working, I think we'd be in good shape for v3.

-- Joe

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Joao Moreira]

On 2019-03-22 11:54, Joe Lawrence wrote:
> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
>> 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.
>> 
>> [khlebnikov:
>> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
>> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
>> * 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
>> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
>> ]
>> 
>> [mbenes:
>> * remove modinfo call. LIVEPATCH_ in Makefiled
>> ]
>> 
>> [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.
>> ]
>> 
>> 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>
>> ---
>>  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(-)
>> 
>> 
>> [ ... snip ... ]
>> 
>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> index 7d4af0d0accb..da779a185218 100644
>> --- a/scripts/Makefile.modpost
>> +++ b/scripts/Makefile.modpost
>> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>>                   -o $@ $(filter-out FORCE,$^) ;                       
>>   \
>>  	$(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
>> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>                            ^
> Should there be a ';' semicolon here (and maybe a line-break) between
> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?
> 
> I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
> really strange build errors that I traced to this line.  Without a
> semicolon, the build was trying to run a make command with a linker
> command smashed onto the end of it:
> 
>   make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m
> elf64lppc -T ./scripts/module-common.lds -T
> ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o
> crypto/xts.ko crypto/xts.o crypto/xts.mod.o
> 
> 
> Now, klp-convert looks like will need some ppc64le work as well, as 
> it's
> confused about .TOC. symbols:
> 
>     CC      samples/livepatch/livepatch-annotated-sample.mod.o
>     CC      samples/livepatch/livepatch-callbacks-busymod.mod.o
>     CC      samples/livepatch/livepatch-callbacks-demo.mod.o
>     CC      samples/livepatch/livepatch-callbacks-mod.mod.o
>     CC      samples/livepatch/livepatch-sample.mod.o
>     CC      samples/livepatch/livepatch-shadow-fix1.mod.o
>     CC      samples/livepatch/livepatch-shadow-fix2.mod.o
>     CC      samples/livepatch/livepatch-shadow-mod.mod.o
>     LD [M]  samples/livepatch/livepatch-annotated-sample.ko
>     LD [M]  samples/livepatch/livepatch-callbacks-demo.ko
>     KLP     samples/livepatch/livepatch-annotated-sample.ko
>     LD [M]  samples/livepatch/livepatch-callbacks-mod.ko
>     KLP     samples/livepatch/livepatch-callbacks-demo.ko
>     LD [M]  samples/livepatch/livepatch-sample.ko
>     LD [M]  samples/livepatch/livepatch-shadow-fix1.ko
>   klp-convert: Define KLP_SYMPOS for the symbol: .TOC.
>   Valid KLP_SYMPOS for symbol .TOC.:
> 
>   [ ... snip listing of all .TOC's across the kernel ... ]
> 
> but we can save that for another day.
> 

Hi Joe, first of all, thank you for you in-depth review. I did not have 
the time to go through everything yet, and I'll reply properly to the 
other comments soon. Yet, I would like to add a quick note here, since 
you are testing ppc64le. ppc64le is exactly one of the things I have 
been dealing with in the last two days, specifically the .TOC. symbols 
(there is also another bug related to converting relocations of symbols 
with index 0, but this one is architecture agnostic).

If you would like to take a look, the fix is in the latest commit here:
- https://github.com/SUSE/klp-convert/tree/fixppc64le

I would be very glad if you have any comment or input on that, and I 
guess that if the fix is that simple, I can manage to squash it into v3.

Thanks,
João

> 
> -- Joe

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
> 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.
> 
> [khlebnikov:
> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> * 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
> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> ]
> 
> [mbenes:
> * remove modinfo call. LIVEPATCH_ in Makefiled
> ]
> 
> [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.
> ]
> 
> 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>
> ---
>  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(-)
> 
>
> [ ... snip ... ]
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 7d4af0d0accb..da779a185218 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>                   -o $@ $(filter-out FORCE,$^) ;                         \
>  	$(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
> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
                           ^
Should there be a ';' semicolon here (and maybe a line-break) between
$(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?

I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
really strange build errors that I traced to this line.  Without a
semicolon, the build was trying to run a make command with a linker
command smashed onto the end of it:

  make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m elf64lppc -T ./scripts/module-common.lds -T ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o crypto/xts.ko crypto/xts.o crypto/xts.mod.o


Now, klp-convert looks like will need some ppc64le work as well, as it's
confused about .TOC. symbols:

    CC      samples/livepatch/livepatch-annotated-sample.mod.o
    CC      samples/livepatch/livepatch-callbacks-busymod.mod.o
    CC      samples/livepatch/livepatch-callbacks-demo.mod.o
    CC      samples/livepatch/livepatch-callbacks-mod.mod.o
    CC      samples/livepatch/livepatch-sample.mod.o
    CC      samples/livepatch/livepatch-shadow-fix1.mod.o
    CC      samples/livepatch/livepatch-shadow-fix2.mod.o
    CC      samples/livepatch/livepatch-shadow-mod.mod.o
    LD [M]  samples/livepatch/livepatch-annotated-sample.ko
    LD [M]  samples/livepatch/livepatch-callbacks-demo.ko
    KLP     samples/livepatch/livepatch-annotated-sample.ko
    LD [M]  samples/livepatch/livepatch-callbacks-mod.ko
    KLP     samples/livepatch/livepatch-callbacks-demo.ko
    LD [M]  samples/livepatch/livepatch-sample.ko
    LD [M]  samples/livepatch/livepatch-shadow-fix1.ko
  klp-convert: Define KLP_SYMPOS for the symbol: .TOC.
  Valid KLP_SYMPOS for symbol .TOC.:

  [ ... snip listing of all .TOC's across the kernel ... ]

but we can save that for another day.


-- Joe

Re: [PATCH v2 5/8] modpost: Integrate klp-convert

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
> 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.
> 
> [khlebnikov:
> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> * 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
> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> ]
> 
> [mbenes:
> * remove modinfo call. LIVEPATCH_ in Makefiled
> ]
> 
> [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.
> ]
> 
> 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>
> ---
>  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(-)
> 
> [ ... snip ... ]
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 7d4af0d0accb..da779a185218 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>                   -o $@ $(filter-out FORCE,$^) ;                         \
>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>  
> +SLIST = $(objtree)/Symbols.list
> +KLP_CONVERT = scripts/livepatch/klp-convert
> +quiet_cmd_klp_convert = KLP $@

Minor nit, but a little more whitespace after "KLP" will more neatly
align the build output:

  [squash] modpost: align KLP prefix 
  https://github.com/torvalds/linux/commit/543aa6bc390b778a2e5a6706960c1414404d409e

> [ ... snip ... ]
>  
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 26bf886bd168..1dfc34d8b668 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1979,6 +1979,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.modinfo, info.modinfo_len, "livepatch"))
> +		mod->livepatch = 1;
> +
> [ ... snip ... ]

To bisect/build post v5.0 merge, I needed to update the get_modinfo()
call.  h/t to the 0-day bot for flagging this:
  
  [squash] modpost: rebase for v5.0 
  https://github.com/torvalds/linux/commit/ab82f725dbc8d6366ca4912a4d05372e24e92e8b

-- Joe