From: Miroslav Benes <[email protected]>
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 <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
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 <[email protected]>");
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 <[email protected]>");
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 <[email protected]>");
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 <[email protected]>");
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
Hi Joe,
On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <[email protected]> wrote:
>
> From: Miroslav Benes <[email protected]>
>
> 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
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 <[email protected]> wrote:
> >
> > From: Miroslav Benes <[email protected]>
> >
> > 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
On Wed, 31 Jul 2019, Masahiro Yamada wrote:
> Hi Joe,
>
>
> On Thu, May 9, 2019 at 11:39 PM Joe Lawrence <[email protected]> wrote:
> >
> > From: Miroslav Benes <[email protected]>
> >
> > 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
Hi Joe,
On Tue, Aug 13, 2019 at 12:56 AM Joe Lawrence <[email protected]> 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 <[email protected]> wrote:
> > >
> > > From: Miroslav Benes <[email protected]>
> > >
> > > 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
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
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
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
On Fri, Aug 16, 2019 at 9:43 PM Joe Lawrence <[email protected]> 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
Hi Joe,
On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence <[email protected]> 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
On Mon, 19 Aug 2019, Masahiro Yamada wrote:
> On Fri, Aug 16, 2019 at 9:43 PM Joe Lawrence <[email protected]> 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
On 8/18/19 11:50 PM, Masahiro Yamada wrote:
> Hi Joe,
>
> On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence <[email protected]> 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
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
> > 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
Hi Joe,
On Tue, Aug 20, 2019 at 1:02 AM Joe Lawrence <[email protected]> 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