On Fri, Mar 01, 2019 at 11:13:07AM -0300, 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
^^^^^^^^^^^
nit: s/statementes/statements
> proposed by Miroslav Benes as a workaround for identifying livepathces
^^^^^^^^^^^
nit: s/livepathces/livepatches
> without depending on modinfo during the modpost stage. It was moved to
> this patch as the approach also shown to be useful while building
> Symbols.list.
>
> Signed-off-by: Joao Moreira <[email protected]>
> ---
> .gitignore | 1 +
> Makefile | 28 +++++++++++++++++++++++++---
> samples/livepatch/Makefile | 1 +
> scripts/Makefile.build | 6 ++++++
> 4 files changed, 33 insertions(+), 3 deletions(-)
>
> [ ... snip ... ]
> diff --git a/Makefile b/Makefile
> index 9ef547fc7ffe..b28eba2cad01 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -561,10 +561,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
> @@ -1244,9 +1247,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
Nit: the tab between "KLP" and "Symbols.list" doesn't match the same
alignment as other build commands:
[squash] kbuild: align KLP prefix
https://github.com/torvalds/linux/commit/6419a05560731eb1306e441aadcaa2dfd9f9f935
> [ ... snip ... ]
> @@ -1341,7 +1360,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
Another nit, but the klpclean target doesn't create a klpclean file, so
we should add it to PHONY:
[squash] kbuild: add klpclean to PHONY
https://github.com/torvalds/linux/commit/7a8b0bea2177c8150ead4848c4498a6081b4a5ae
> [ ... snip ... ]
> 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
Would this change be better located in [PATCH 6/8] modpost: Add modinfo
flag to livepatch modules? Or was it needed to test-drive the rest of
this patch?
> 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 $< && \
Since cmd_livepatch is only called for single-used-m, does this mean
that we can only klp-convert single object file livepatch modules?
I stumbled upon this when trying to create a self-test module that
incorporated two object files. I tried adding a $(call cmd_livepatch)
in the recipe for $(obj)/%.o, but that didn't help. My kbuild foo
wasn't good enough to figure this one out.
-- Joe
> > 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 $< && \
>
> Since cmd_livepatch is only called for single-used-m, does this mean
> that we can only klp-convert single object file livepatch modules?
>
> I stumbled upon this when trying to create a self-test module that
> incorporated two object files. I tried adding a $(call cmd_livepatch)
> in the recipe for $(obj)/%.o, but that didn't help. My kbuild foo
> wasn't good enough to figure this one out.
I looked at my original code and it is a bit different there. I placed it
under rule_cc_o_c right after objtool command. If I remember correctly
this is the correct recipe for .c->.o. Unfortunately I forgot the details
and there is of course nothing about it in my notes.
Does it help?
Joao, is there a reason you moved it elsewhere?
Miroslav
On 3/20/19 4:08 PM, Miroslav Benes wrote:
>>> 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 $< && \
>>
>> Since cmd_livepatch is only called for single-used-m, does this mean
>> that we can only klp-convert single object file livepatch modules?
>>
>> I stumbled upon this when trying to create a self-test module that
>> incorporated two object files. I tried adding a $(call cmd_livepatch)
>> in the recipe for $(obj)/%.o, but that didn't help. My kbuild foo
>> wasn't good enough to figure this one out.
>
> I looked at my original code and it is a bit different there. I placed it
> under rule_cc_o_c right after objtool command. If I remember correctly
> this is the correct recipe for .c->.o. Unfortunately I forgot the details
> and there is of course nothing about it in my notes.
>
> Does it help?
>
> Joao, is there a reason you moved it elsewhere?
Hi,
Unfortunately I can't remember why the chunk was moved to where it is in
this version of the patch, sorry. Yet, I did try to move this into the
rule cc_o_c and it seemed to work with not damage.
Joe, would you kindly verify and squash properly the patch below, which
places cmd_livepatch in rule_cc_o_c?
Thank you.
Subject: [PATCH] Move cmd_klp_convert to the right place
Signed-off-by: Joao Moreira <[email protected]>
---
scripts/Makefile.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 1e28ad21314c..5f66106a47d6 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -260,6 +260,7 @@ define rule_cc_o_c
$(call cmd,objtool)
$(call cmd,modversions_c)
$(call cmd,record_mcount)
+ $(call cmd,livepatch)
endef
define rule_as_o_S
@@ -288,7 +289,6 @@ $(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
>
On 3/26/19 10:40 AM, Joao Moreira wrote:
>
>
> On 3/20/19 4:08 PM, Miroslav Benes wrote:
>>>> 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 $< && \
>>>
>>> Since cmd_livepatch is only called for single-used-m, does this mean
>>> that we can only klp-convert single object file livepatch modules?
>>>
>>> I stumbled upon this when trying to create a self-test module that
>>> incorporated two object files. I tried adding a $(call cmd_livepatch)
>>> in the recipe for $(obj)/%.o, but that didn't help. My kbuild foo
>>> wasn't good enough to figure this one out.
>>
>> I looked at my original code and it is a bit different there. I placed it
>> under rule_cc_o_c right after objtool command. If I remember correctly
>> this is the correct recipe for .c->.o. Unfortunately I forgot the details
>> and there is of course nothing about it in my notes.
>>
>> Does it help?
>>
>> Joao, is there a reason you moved it elsewhere?
>
> Hi,
>
> Unfortunately I can't remember why the chunk was moved to where it is in
> this version of the patch, sorry. Yet, I did try to move this into the
> rule cc_o_c and it seemed to work with not damage.
>
> Joe, would you kindly verify and squash properly the patch below, which
> places cmd_livepatch in rule_cc_o_c?
>
> Thank you.
>
> Subject: [PATCH] Move cmd_klp_convert to the right place
>
>
>
> Signed-off-by: Joao Moreira <[email protected]>
>
> ---
>
> scripts/Makefile.build | 2 +-
>
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 1e28ad21314c..5f66106a47d6 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -260,6 +260,7 @@ define rule_cc_o_c
> $(call cmd,objtool)
> $(call cmd,modversions_c)
> $(call cmd,record_mcount)
> + $(call cmd,livepatch)
> endef
>
> define rule_as_o_S
> @@ -288,7 +289,6 @@ $(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 $< && \
Hi Joao,
This change seems to work okay for (again) single object modules, but
I'm having issues with multi-object modules.
Here are my sources:
% head -n100 *
==> Makefile <==
KDIR := /lib/modules/$(shell uname -r)/build
PWD := $(shell pwd)
LIVEPATCH_test_mod_a.o := y
LIVEPATCH_test_mod_b.o := y
obj-m += test_mod.o
test_mod-y := \
test_mod_a.o \
test_mod_b.o
default:
$(MAKE) -C $(KDIR) M=$(PWD)
clean:
@rm -rf .tmp_versions/
@rm -f .*.cmd *.o *.mod.* *.ko modules.order Module.symvers
==> test_mod_a.c <==
#include <linux/module.h>
__used static void function(void) { }
MODULE_LICENSE("GPL");
==> test_mod_b.c <==
__used static void function(void) { }
But when I build, I don't see klp-convert invoked for any of the object
files:
% make
make -C /lib/modules/5.0.0+/build M=/home/cloud-user/klp-convert-modtest
make[1]: Entering directory '/home/cloud-user/disk/linux'
CC [M] /home/cloud-user/klp-convert-modtest/test_mod_a.o
CC [M] /home/cloud-user/klp-convert-modtest/test_mod_b.o
LD [M] /home/cloud-user/klp-convert-modtest/test_mod.o
Building modules, stage 2.
MODPOST 1 modules
CC /home/cloud-user/klp-convert-modtest/test_mod.mod.o
LD [M] /home/cloud-user/klp-convert-modtest/test_mod.ko
make[1]: Leaving directory '/home/cloud-user/disk/linux'
However, if I modify the Makefile to build test_mod_a.o into its own
module, I see "KLP
/home/cloud-user/klp-convert-modtest/test_mod_a.ko" in the build output.
-- Joe
On 3/26/19 1:15 PM, Joe Lawrence wrote:
> On 3/26/19 10:40 AM, Joao Moreira wrote:
>>
>>
>> On 3/20/19 4:08 PM, Miroslav Benes wrote:
>>>>> 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 $< && \
>>>>
>>>> Since cmd_livepatch is only called for single-used-m, does this mean
>>>> that we can only klp-convert single object file livepatch modules?
>>>>
>>>> I stumbled upon this when trying to create a self-test module that
>>>> incorporated two object files. I tried adding a $(call cmd_livepatch)
>>>> in the recipe for $(obj)/%.o, but that didn't help. My kbuild foo
>>>> wasn't good enough to figure this one out.
>>>
>>> I looked at my original code and it is a bit different there. I
>>> placed it
>>> under rule_cc_o_c right after objtool command. If I remember correctly
>>> this is the correct recipe for .c->.o. Unfortunately I forgot the
>>> details
>>> and there is of course nothing about it in my notes.
>>>
>>> Does it help?
>>>
>>> Joao, is there a reason you moved it elsewhere?
>>
>> Hi,
>>
>> Unfortunately I can't remember why the chunk was moved to where it is in
>> this version of the patch, sorry. Yet, I did try to move this into the
>> rule cc_o_c and it seemed to work with not damage.
>>
>> Joe, would you kindly verify and squash properly the patch below, which
>> places cmd_livepatch in rule_cc_o_c?
>>
>> Thank you.
>>
>> Subject: [PATCH] Move cmd_klp_convert to the right place
>>
>>
>> Signed-off-by: Joao Moreira <[email protected]>
>>
>> ---
>>
>> scripts/Makefile.build | 2 +-
>>
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 1e28ad21314c..5f66106a47d6 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -260,6 +260,7 @@ define rule_cc_o_c
>> $(call cmd,objtool)
>> $(call cmd,modversions_c)
>> $(call cmd,record_mcount)
>> + $(call cmd,livepatch)
>> endef
>> define rule_as_o_S
>> @@ -288,7 +289,6 @@ $(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 $< && \
>
> Hi Joao,
>
> This change seems to work okay for (again) single object modules, but
> I'm having issues with multi-object modules.
>
Hi Joe, thanks for the sources, this made everything much easier in my
side :)
In the patch below I change a little bit the interface used to inform
kbuild that a module is a livepatch. Instead of defining the flag
LIVEPATCH_ per .o file, we define it per module (what actually makes
much more sense). We later use $(basetarget) in the Makefile for
checking the flags. By doing so, and invoking cmd_livepatch both from
the $(single-used-m) and $(multi-used-m) we ensure that the .livepatch
file is created for each module, what later in the pipeline flags the
invocation of klp-convert.
I tested the following patch with the sources you provided (with little
modifications, removing the .o from the LIVEPATCH_ definitions and using
the module name instead of the object names), achieving successful
compilation and conversion. I also tested against the sample
livepatches, thus I think it might be ok now.
Do you think we can go this way to solve the problem in v3?
-- PATCH --
Subject: [PATCH] Fix cmd_livepatch for multi-object modules
Signed-off-by: Joao Moreira <[email protected]>
---
samples/livepatch/Makefile | 10 +++++-----
scripts/Makefile.build | 5 +++--
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 93725c434f4f..dea530840725 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1,8 +1,8 @@
-LIVEPATCH_livepatch-sample.o := y
-LIVEPATCH_livepatch-shadow-fix1.o := y
-LIVEPATCH_livepatch-shadow-fix2.o := y
-LIVEPATCH_livepatch-callbacks-demo.o := y
-LIVEPATCH_livepatch-annotated-sample.o := y
+LIVEPATCH_livepatch-sample := y
+LIVEPATCH_livepatch-shadow-fix1 := y
+LIVEPATCH_livepatch-shadow-fix2 := y
+LIVEPATCH_livepatch-callbacks-demo := y
+LIVEPATCH_livepatch-annotated-sample := y
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index dba89e605eab..52e69b4084d5 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -250,7 +250,7 @@ cmd_gen_ksymdeps = \
endif
ifdef CONFIG_LIVEPATCH
-cmd_livepatch = $(if $(LIVEPATCH_$(basetarget).o),
\
+cmd_livepatch = $(if $(LIVEPATCH_$(basetarget)),
\
$(shell touch $(MODVERDIR)/$(basetarget).livepatch))
endif
@@ -288,9 +288,9 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source)
$(objtool_dep) FORCE
$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source)
$(objtool_dep) FORCE
$(call cmd,force_checksrc)
$(call if_changed_rule,cc_o_c)
+ $(call cmd,livepatch)
@{ echo $(@:.o=.ko); echo $@; \
$(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
- $(call cmd_livepatch)
quiet_cmd_cc_lst_c = MKLST $@
cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
@@ -469,6 +469,7 @@ cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@
$(filter %.o,$^) $(cmd_secanalysis
$(multi-used-m): FORCE
$(call if_changed,link_multi-m)
+ $(call cmd,livepatch)
@{ echo $(@:.o=.ko); echo $(filter %.o,$^); \
$(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
$(call multi_depend, $(multi-used-m), .o, -objs -y -m)
--
2.16.4
On 3/26/19 2:13 PM, Joao Moreira wrote:
>
>
> On 3/26/19 1:15 PM, Joe Lawrence wrote:
>>
>> Hi Joao,
>>
>> This change seems to work okay for (again) single object modules, but
>> I'm having issues with multi-object modules.
>>
>
> Hi Joe, thanks for the sources, this made everything much easier in my
> side :)
>
> In the patch below I change a little bit the interface used to inform
> kbuild that a module is a livepatch. Instead of defining the flag
> LIVEPATCH_ per .o file, we define it per module (what actually makes
> much more sense). We later use $(basetarget) in the Makefile for
> checking the flags. By doing so, and invoking cmd_livepatch both from
> the $(single-used-m) and $(multi-used-m) we ensure that the .livepatch
> file is created for each module, what later in the pipeline flags the
> invocation of klp-convert.
>
> I tested the following patch with the sources you provided (with little
> modifications, removing the .o from the LIVEPATCH_ definitions and using
> the module name instead of the object names), achieving successful
> compilation and conversion. I also tested against the sample
> livepatches, thus I think it might be ok now.
Cool thanks for taking a look -- I can confirm that the toy code I sent
over builds with those modifications and so does the sample and selftest
I was working on. I'll set about refactoring that klp-convert selftest
to combine .o files into a more compact module.
> Do you think we can go this way to solve the problem in v3?
Yeah, I'll add it to the list of patches to squash for v3.
Thanks,
-- Joe
On Tue, Mar 26, 2019 at 04:53:12PM -0400, Joe Lawrence wrote:
> On 3/26/19 2:13 PM, Joao Moreira wrote:
> >
> >
> > On 3/26/19 1:15 PM, Joe Lawrence wrote:
> >>
> > > Hi Joao,
> > >
> > > This change seems to work okay for (again) single object modules, but
> > > I'm having issues with multi-object modules.
> > >
> >
> > Hi Joe, thanks for the sources, this made everything much easier in my
> > side :)
> >
> > In the patch below I change a little bit the interface used to inform
> > kbuild that a module is a livepatch. Instead of defining the flag
> > LIVEPATCH_ per .o file, we define it per module (what actually makes
> > much more sense). We later use $(basetarget) in the Makefile for
> > checking the flags. By doing so, and invoking cmd_livepatch both from
> > the $(single-used-m) and $(multi-used-m) we ensure that the .livepatch
> > file is created for each module, what later in the pipeline flags the
> > invocation of klp-convert.
> >
> > I tested the following patch with the sources you provided (with little
> > modifications, removing the .o from the LIVEPATCH_ definitions and using
> > the module name instead of the object names), achieving successful
> > compilation and conversion. I also tested against the sample
> > livepatches, thus I think it might be ok now.
>
> Cool thanks for taking a look -- I can confirm that the toy code I sent over
> builds with those modifications and so does the sample and selftest I was
> working on. I'll set about refactoring that klp-convert selftest to combine
> .o files into a more compact module.
Hmm, maybe I spoke too soon.
I am having issues if I have a two-object livepatch module in which each
object file needs to specify its own KLP_MODULE_RELOC for the same
symbol name.
For example: I have test_klp_convert.ko which is comprised of
test_klp_convert_a.o. which needs to refer to state_show,1 and
test_klp_convert_b.o. which needs to refer to state_show,2.
% make
...
KLP lib/livepatch/test_klp_convert.ko
klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,0 vs. vmlinux.state_show,1.
klp-convert: Unable to load user-provided sympos
make[2]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert.ko] Error 255
make[1]: *** [/home/cloud-user/disk/linux/Makefile:1282: modules] Error 2
make: *** [Makefile:170: sub-make] Error 2
I take a closer look next week, but in the meantime, see the source
files below.
-- Joe
==> lib/livepatch/test_klp_convert_a.c <==
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
/* klp-convert symbols - vmlinux */
extern void *state_show;
__used void print_state_show(void)
{
pr_info("%s: state_show: %p\n", __func__, state_show);
}
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_a[] = {
KLP_SYMPOS(state_show, 1)
};
static struct klp_func funcs[] = {
{
}, { }
};
static struct klp_object objs[] = {
{
/* name being NULL means vmlinux */
.funcs = funcs,
}, { }
};
static struct klp_patch patch = {
.mod = THIS_MODULE,
.objs = objs,
};
static int test_klp_convert_init(void)
{
int ret;
ret = klp_enable_patch(&patch);
if (ret)
return ret;
return 0;
}
static void test_klp_convert_exit(void)
{
}
module_init(test_klp_convert_init);
module_exit(test_klp_convert_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Joe Lawrence <[email protected]>");
MODULE_DESCRIPTION("Livepatch test: klp-convert");
==> lib/livepatch/test_klp_convert_b.c <==
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
/* klp-convert symbols - vmlinux */
extern void *state_show;
__used void print_state_show_b(void)
{
pr_info("%s: state_show: %p\n", __func__, state_show);
}
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
KLP_SYMPOS(state_show, 2)
};
==> lib/livepatch/Makefile <==
# SPDX-License-Identifier: GPL-2.0
#
# Makefile for livepatch test code.
LIVEPATCH_test_klp_atomic_replace := y
LIVEPATCH_test_klp_callbacks_demo := y
LIVEPATCH_test_klp_callbacks_demo2 := y
LIVEPATCH_test_klp_convert := y
LIVEPATCH_test_klp_livepatch := y
obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
test_klp_callbacks_demo.o \
test_klp_callbacks_demo2.o \
test_klp_callbacks_busy.o \
test_klp_callbacks_mod.o \
test_klp_convert.o \
test_klp_livepatch.o \
test_klp_shadow_vars.o
test_klp_convert-y := \
test_klp_convert_a.o \
test_klp_convert_b.o
# Target modules to be livepatched require CC_FLAGS_FTRACE
CFLAGS_test_klp_callbacks_busy.o += $(CC_FLAGS_FTRACE)
CFLAGS_test_klp_callbacks_mod.o += $(CC_FLAGS_FTRACE)
CFLAGS_test_klp_convert_mod.o += $(CC_FLAGS_FTRACE)
On Thu, Mar 28, 2019 at 04:17:03PM -0400, Joe Lawrence wrote:
> On Tue, Mar 26, 2019 at 04:53:12PM -0400, Joe Lawrence wrote:
> > On 3/26/19 2:13 PM, Joao Moreira wrote:
> > >
> > >
> > > On 3/26/19 1:15 PM, Joe Lawrence wrote:
> > >>
> > > > Hi Joao,
> > > >
> > > > This change seems to work okay for (again) single object modules, but
> > > > I'm having issues with multi-object modules.
> > > >
> > >
> > > Hi Joe, thanks for the sources, this made everything much easier in my
> > > side :)
> > >
> > > In the patch below I change a little bit the interface used to inform
> > > kbuild that a module is a livepatch. Instead of defining the flag
> > > LIVEPATCH_ per .o file, we define it per module (what actually makes
> > > much more sense). We later use $(basetarget) in the Makefile for
> > > checking the flags. By doing so, and invoking cmd_livepatch both from
> > > the $(single-used-m) and $(multi-used-m) we ensure that the .livepatch
> > > file is created for each module, what later in the pipeline flags the
> > > invocation of klp-convert.
> > >
> > > I tested the following patch with the sources you provided (with little
> > > modifications, removing the .o from the LIVEPATCH_ definitions and using
> > > the module name instead of the object names), achieving successful
> > > compilation and conversion. I also tested against the sample
> > > livepatches, thus I think it might be ok now.
> >
> > Cool thanks for taking a look -- I can confirm that the toy code I sent over
> > builds with those modifications and so does the sample and selftest I was
> > working on. I'll set about refactoring that klp-convert selftest to combine
> > .o files into a more compact module.
>
> Hmm, maybe I spoke too soon.
>
> I am having issues if I have a two-object livepatch module in which each
> object file needs to specify its own KLP_MODULE_RELOC for the same
> symbol name.
>
> For example: I have test_klp_convert.ko which is comprised of
> test_klp_convert_a.o. which needs to refer to state_show,1 and
> test_klp_convert_b.o. which needs to refer to state_show,2.
>
> % make
> ...
> KLP lib/livepatch/test_klp_convert.ko
> klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,0 vs. vmlinux.state_show,1.
> klp-convert: Unable to load user-provided sympos
> make[2]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert.ko] Error 255
> make[1]: *** [/home/cloud-user/disk/linux/Makefile:1282: modules] Error 2
> make: *** [Makefile:170: sub-make] Error 2
>
> I take a closer look next week, but in the meantime, see the source
> files below.
Okay, with a fresh set of eyes today, I think the issue can be
summarized as:
* "Special" livepatch symbols, as processed by klp-convert, require
external linkage, otherwise a new local storage instance will be
created and miss klp-convert altogether
* When linking together two object files, their combined symbol table
will include a de-duped list of uniquly named global symbols
So if we are to run klp-convert on the combined module object file (as
per v2 plus suggested changes in this thread), we are going to run into
problems if ...
Example
=======
(quoted in previous reply), test_klp_convert_a and test_klp_convert_b
have their own "state_show" variable in which each wishes to resolve to
unique symbol positions (2 and 3 accordingly).
test_klp_convert_a
------------------
We get one symbol table entry and one relocation as expected.
% eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
Symbol table [27] '.symtab' contains 38 entries:
29 local symbols String table: [28] '.strtab'
Num: Value Size Type Bind Vis Ndx Name
...
30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
...
% eu-readelf --relocs lib/livepatch/test_klp_convert_a.o
Relocation section [12] '.rela.klp.module_relocs.vmlinux' for section [11] '.klp.module_relocs.vmlinux' at offset 0x4b8 contains 1 entry:
Offset Type Value Addend Name
000000000000000000 X86_64_64 000000000000000000 +0 state_show
test_klp_convert_b
------------------
Just like the other object file, one symbol table entry and one
relocation:
% eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
Symbol table [24] '.symtab' contains 23 entries:
19 local symbols String table: [25] '.strtab'
Num: Value Size Type Bind Vis Ndx Name
...
20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
...
% eu-readelf --relocs lib/livepatch/test_klp_convert_b.o
Relocation section [ 9] '.rela.klp.module_relocs.vmlinux' for section [ 8] '.klp.module_relocs.vmlinux' at offset 0x118 contains 1 entry:
Offset Type Value Addend Name
000000000000000000 X86_64_64 000000000000000000 +0 state_show
test_klp_convert
----------------
But the combined test_klp_convert.klp.o file has only a single
unresolved "state_show" symbol in its symbol table:
% eu-readelf --symbols lib/livepatch/test_klp_convert.klp.o
Symbol table [35] '.symtab' contains 57 entries:
47 local symbols String table: [36] '.strtab'
Num: Value Size Type Bind Vis Ndx Name
...
52: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
...
though the .rela.klp.module_relocs.vmlinux relocation section has two
entries:
% eu-readelf --relocs lib/livepatch/test_klp_convert.klp.o
Relocation section [17] '.rela.klp.module_relocs.vmlinux' for section [16] '.klp.module_relocs.vmlinux' at offset 0x446c8 contains 2 entries:
Offset Type Value Addend Name
000000000000000000 X86_64_64 000000000000000000 +0 state_show
0x0000000000000010 X86_64_64 000000000000000000 +0 state_show
and it looks like the combined KLP_MODULE_RELOC still contains the two
unique symbol position values (2 and 3):
% objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 |................|
00000010 00 00 00 00 00 00 00 00 03 00 00 00 |............|
0000001c
Maybe we can work around this by modifying the annotation macros and/or
klp-convert, or live with this for now.
Note: I'm inclined to defer work on resolving this limitation to v4. I
would like v3 to focus on collecting and squashing the feedback up to
now on v2. There are a few other outstanding issues that I have run
across while testing this patchset, so I feel that it would be clearer
for folks to base comments on those off a clean v3 slate.
-- Joe
> ==> lib/livepatch/test_klp_convert_a.c <==
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/livepatch.h>
>
> /* klp-convert symbols - vmlinux */
> extern void *state_show;
> __used void print_state_show(void)
> {
> pr_info("%s: state_show: %p\n", __func__, state_show);
> }
>
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_a[] = {
> KLP_SYMPOS(state_show, 1)
> };
>
> static struct klp_func funcs[] = {
> {
> }, { }
> };
>
> static struct klp_object objs[] = {
> {
> /* name being NULL means vmlinux */
> .funcs = funcs,
> }, { }
> };
>
> static struct klp_patch patch = {
> .mod = THIS_MODULE,
> .objs = objs,
> };
>
> static int test_klp_convert_init(void)
> {
> int ret;
>
> ret = klp_enable_patch(&patch);
> if (ret)
> return ret;
>
> return 0;
> }
>
> static void test_klp_convert_exit(void)
> {
> }
>
> module_init(test_klp_convert_init);
> module_exit(test_klp_convert_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Joe Lawrence <[email protected]>");
> MODULE_DESCRIPTION("Livepatch test: klp-convert");
>
> ==> lib/livepatch/test_klp_convert_b.c <==
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/livepatch.h>
>
> /* klp-convert symbols - vmlinux */
> extern void *state_show;
>
> __used void print_state_show_b(void)
> {
> pr_info("%s: state_show: %p\n", __func__, state_show);
> }
>
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> KLP_SYMPOS(state_show, 2)
> };
>
>
> ==> lib/livepatch/Makefile <==
> # SPDX-License-Identifier: GPL-2.0
> #
> # Makefile for livepatch test code.
>
> LIVEPATCH_test_klp_atomic_replace := y
> LIVEPATCH_test_klp_callbacks_demo := y
> LIVEPATCH_test_klp_callbacks_demo2 := y
> LIVEPATCH_test_klp_convert := y
> LIVEPATCH_test_klp_livepatch := y
>
> obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
> test_klp_callbacks_demo.o \
> test_klp_callbacks_demo2.o \
> test_klp_callbacks_busy.o \
> test_klp_callbacks_mod.o \
> test_klp_convert.o \
> test_klp_livepatch.o \
> test_klp_shadow_vars.o
>
> test_klp_convert-y := \
> test_klp_convert_a.o \
> test_klp_convert_b.o
>
> # Target modules to be livepatched require CC_FLAGS_FTRACE
> CFLAGS_test_klp_callbacks_busy.o += $(CC_FLAGS_FTRACE)
> CFLAGS_test_klp_callbacks_mod.o += $(CC_FLAGS_FTRACE)
> CFLAGS_test_klp_convert_mod.o += $(CC_FLAGS_FTRACE)
-- Joe
> > Hmm, maybe I spoke too soon.
> >
> > I am having issues if I have a two-object livepatch module in which each
> > object file needs to specify its own KLP_MODULE_RELOC for the same
> > symbol name.
> >
> > For example: I have test_klp_convert.ko which is comprised of
> > test_klp_convert_a.o. which needs to refer to state_show,1 and
> > test_klp_convert_b.o. which needs to refer to state_show,2.
> >
> > % make
> > ...
> > KLP lib/livepatch/test_klp_convert.ko
> > klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,0 vs. vmlinux.state_show,1.
> > klp-convert: Unable to load user-provided sympos
> > make[2]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert.ko] Error 255
> > make[1]: *** [/home/cloud-user/disk/linux/Makefile:1282: modules] Error 2
> > make: *** [Makefile:170: sub-make] Error 2
> >
> > I take a closer look next week, but in the meantime, see the source
> > files below.
>
> Okay, with a fresh set of eyes today, I think the issue can be
> summarized as:
>
> * "Special" livepatch symbols, as processed by klp-convert, require
> external linkage, otherwise a new local storage instance will be
> created and miss klp-convert altogether
>
> * When linking together two object files, their combined symbol table
> will include a de-duped list of uniquly named global symbols
>
> So if we are to run klp-convert on the combined module object file (as
> per v2 plus suggested changes in this thread), we are going to run into
> problems if ...
>
>
> Example
> =======
>
> (quoted in previous reply), test_klp_convert_a and test_klp_convert_b
> have their own "state_show" variable in which each wishes to resolve to
> unique symbol positions (2 and 3 accordingly).
>
>
> test_klp_convert_a
> ------------------
>
> We get one symbol table entry and one relocation as expected.
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
>
> Symbol table [27] '.symtab' contains 38 entries:
> 29 local symbols String table: [28] '.strtab'
> Num: Value Size Type Bind Vis Ndx Name
> ...
> 30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
> ...
>
> % eu-readelf --relocs lib/livepatch/test_klp_convert_a.o
>
> Relocation section [12] '.rela.klp.module_relocs.vmlinux' for section [11] '.klp.module_relocs.vmlinux' at offset 0x4b8 contains 1 entry:
> Offset Type Value Addend Name
> 000000000000000000 X86_64_64 000000000000000000 +0 state_show
>
>
> test_klp_convert_b
> ------------------
>
> Just like the other object file, one symbol table entry and one
> relocation:
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
> Symbol table [24] '.symtab' contains 23 entries:
> 19 local symbols String table: [25] '.strtab'
> Num: Value Size Type Bind Vis Ndx Name
> ...
> 20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
> ...
>
> % eu-readelf --relocs lib/livepatch/test_klp_convert_b.o
>
> Relocation section [ 9] '.rela.klp.module_relocs.vmlinux' for section [ 8] '.klp.module_relocs.vmlinux' at offset 0x118 contains 1 entry:
> Offset Type Value Addend Name
> 000000000000000000 X86_64_64 000000000000000000 +0 state_show
>
>
> test_klp_convert
> ----------------
>
> But the combined test_klp_convert.klp.o file has only a single
> unresolved "state_show" symbol in its symbol table:
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert.klp.o
>
> Symbol table [35] '.symtab' contains 57 entries:
> 47 local symbols String table: [36] '.strtab'
> Num: Value Size Type Bind Vis Ndx Name
> ...
> 52: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
> ...
>
> though the .rela.klp.module_relocs.vmlinux relocation section has two
> entries:
>
> % eu-readelf --relocs lib/livepatch/test_klp_convert.klp.o
>
> Relocation section [17] '.rela.klp.module_relocs.vmlinux' for section [16] '.klp.module_relocs.vmlinux' at offset 0x446c8 contains 2 entries:
> Offset Type Value Addend Name
> 000000000000000000 X86_64_64 000000000000000000 +0 state_show
> 0x0000000000000010 X86_64_64 000000000000000000 +0 state_show
>
> and it looks like the combined KLP_MODULE_RELOC still contains the two
> unique symbol position values (2 and 3):
>
> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
> 00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 |................|
> 00000010 00 00 00 00 00 00 00 00 03 00 00 00 |............|
> 0000001c
Nice :/
> Maybe we can work around this by modifying the annotation macros and/or
> klp-convert, or live with this for now.
The question is (and I'll check later. I cannot wrap my head around it
now) if it at least works if there are two references of the same symbol
in two different .o. It would be same state_show in this case and not two
different ones. If it works then I think we can live with it for a while,
because after all duplicate symbols are quite rare in the kernel.
I think it should not be a big problem to fix it though (famous last
words). The information is there, so klp-convert with a changed annotation
macro should deal with it.
> Note: I'm inclined to defer work on resolving this limitation to v4. I
> would like v3 to focus on collecting and squashing the feedback up to
> now on v2. There are a few other outstanding issues that I have run
> across while testing this patchset, so I feel that it would be clearer
> for folks to base comments on those off a clean v3 slate.
Makes sense to me.
Miroslav
On 4/3/19 8:48 AM, Miroslav Benes wrote:
>
>> and it looks like the combined KLP_MODULE_RELOC still contains the two
>> unique symbol position values (2 and 3):
>>
>> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
>> 00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 |................|
>> 00000010 00 00 00 00 00 00 00 00 03 00 00 00 |............|
>> 0000001c
>
> Nice :/
>
>> Maybe we can work around this by modifying the annotation macros and/or
>> klp-convert, or live with this for now.
>
> The question is (and I'll check later. I cannot wrap my head around it
> now) if it at least works if there are two references of the same symbol
> in two different .o. It would be same state_show in this case and not two
> different ones. If it works then I think we can live with it for a while,
> because after all duplicate symbols are quite rare in the kernel.
Possibly, but in testing that scenario I found another issue. Check out what
happens to the combined .klp.module_relocs.vmlinux section for:
test_klp_convert_a.c
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_a[] = {
KLP_SYMPOS(state_show, 2)
KLP_SYMPOS(joe, 10)
KLP_SYMPOS(joe2, 11)
};
test_klp_convert_b.c
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
KLP_SYMPOS(state_show, 2)
};
The second file's klp_module_reloc are not aligned with the first,
so I think there is additional padding to push the second set to a
word boundary:
% objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00
|-*sym----------------| |--sympos-| |-*sym-----
00000010 00 00 00 00 0a 00 00 00 00 00 00 00 00 00 00 00
----------| |--sympos-| |-*sym----------------|
00000020 0b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
|-sympos--| |-*sym----------------|
^^^^^^^^^^^
padding
00000030 02 00 00 00
|-sympos--|
in this case, klp-convert thought the last symbol's sympos was
incorrectly 0 and not 2.
If the packed attribute is merely a space optimization, can we
simply pull that (or can we specify slightly looser alignment to
account for the padding)?
I'll continue working on putting together v3 and add this new item
to the TODO list.
Thanks,
-- Joe
On Wed, 3 Apr 2019, Joe Lawrence wrote:
> On 4/3/19 8:48 AM, Miroslav Benes wrote:
> >
> >> and it looks like the combined KLP_MODULE_RELOC still contains the two
> >> unique symbol position values (2 and 3):
> >>
> >> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
> >> 00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 |................|
> >> 00000010 00 00 00 00 00 00 00 00 03 00 00 00 |............|
> >> 0000001c
> >
> > Nice :/
> >
> >> Maybe we can work around this by modifying the annotation macros and/or
> >> klp-convert, or live with this for now.
> >
> > The question is (and I'll check later. I cannot wrap my head around it
> > now) if it at least works if there are two references of the same symbol
> > in two different .o. It would be same state_show in this case and not two
> > different ones. If it works then I think we can live with it for a while,
> > because after all duplicate symbols are quite rare in the kernel.
>
> Possibly, but in testing that scenario I found another issue. Check out what
> happens to the combined .klp.module_relocs.vmlinux section for:
>
> test_klp_convert_a.c
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_a[] = {
> KLP_SYMPOS(state_show, 2)
> KLP_SYMPOS(joe, 10)
> KLP_SYMPOS(joe2, 11)
> };
>
> test_klp_convert_b.c
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> KLP_SYMPOS(state_show, 2)
> };
>
> The second file's klp_module_reloc are not aligned with the first,
> so I think there is additional padding to push the second set to a
> word boundary:
>
> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
> 00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00
> |-*sym----------------| |--sympos-| |-*sym-----
>
> 00000010 00 00 00 00 0a 00 00 00 00 00 00 00 00 00 00 00
> ----------| |--sympos-| |-*sym----------------|
>
> 00000020 0b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> |-sympos--| |-*sym----------------|
>
> ^^^^^^^^^^^
> padding
> 00000030 02 00 00 00
> |-sympos--|
>
>
> in this case, klp-convert thought the last symbol's sympos was
> incorrectly 0 and not 2.
Ugh. It's getting better and better.
> If the packed attribute is merely a space optimization, can we
> simply pull that (or can we specify slightly looser alignment to
> account for the padding)?
I think so.
> I'll continue working on putting together v3 and add this new item
> to the TODO list.
Great job btw. Thanks.
Miroslav
On Wed, 3 Apr 2019, Miroslav Benes wrote:
>
> > > Hmm, maybe I spoke too soon.
> > >
> > > I am having issues if I have a two-object livepatch module in which each
> > > object file needs to specify its own KLP_MODULE_RELOC for the same
> > > symbol name.
> > >
> > > For example: I have test_klp_convert.ko which is comprised of
> > > test_klp_convert_a.o. which needs to refer to state_show,1 and
> > > test_klp_convert_b.o. which needs to refer to state_show,2.
> > >
> > > % make
> > > ...
> > > KLP lib/livepatch/test_klp_convert.ko
> > > klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,0 vs. vmlinux.state_show,1.
> > > klp-convert: Unable to load user-provided sympos
> > > make[2]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert.ko] Error 255
> > > make[1]: *** [/home/cloud-user/disk/linux/Makefile:1282: modules] Error 2
> > > make: *** [Makefile:170: sub-make] Error 2
> > >
> > > I take a closer look next week, but in the meantime, see the source
> > > files below.
> >
> > Okay, with a fresh set of eyes today, I think the issue can be
> > summarized as:
> >
> > * "Special" livepatch symbols, as processed by klp-convert, require
> > external linkage, otherwise a new local storage instance will be
> > created and miss klp-convert altogether
> >
> > * When linking together two object files, their combined symbol table
> > will include a de-duped list of uniquly named global symbols
> >
> > So if we are to run klp-convert on the combined module object file (as
> > per v2 plus suggested changes in this thread), we are going to run into
> > problems if ...
> >
> >
> > Example
> > =======
> >
> > (quoted in previous reply), test_klp_convert_a and test_klp_convert_b
> > have their own "state_show" variable in which each wishes to resolve to
> > unique symbol positions (2 and 3 accordingly).
> >
> >
> > test_klp_convert_a
> > ------------------
> >
> > We get one symbol table entry and one relocation as expected.
> >
> > % eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
> >
> > Symbol table [27] '.symtab' contains 38 entries:
> > 29 local symbols String table: [28] '.strtab'
> > Num: Value Size Type Bind Vis Ndx Name
> > ...
> > 30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
> > ...
> >
> > % eu-readelf --relocs lib/livepatch/test_klp_convert_a.o
> >
> > Relocation section [12] '.rela.klp.module_relocs.vmlinux' for section [11] '.klp.module_relocs.vmlinux' at offset 0x4b8 contains 1 entry:
> > Offset Type Value Addend Name
> > 000000000000000000 X86_64_64 000000000000000000 +0 state_show
> >
> >
> > test_klp_convert_b
> > ------------------
> >
> > Just like the other object file, one symbol table entry and one
> > relocation:
> >
> > % eu-readelf --symbols lib/livepatch/test_klp_convert_a.o
> > Symbol table [24] '.symtab' contains 23 entries:
> > 19 local symbols String table: [25] '.strtab'
> > Num: Value Size Type Bind Vis Ndx Name
> > ...
> > 20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
> > ...
> >
> > % eu-readelf --relocs lib/livepatch/test_klp_convert_b.o
> >
> > Relocation section [ 9] '.rela.klp.module_relocs.vmlinux' for section [ 8] '.klp.module_relocs.vmlinux' at offset 0x118 contains 1 entry:
> > Offset Type Value Addend Name
> > 000000000000000000 X86_64_64 000000000000000000 +0 state_show
> >
> >
> > test_klp_convert
> > ----------------
> >
> > But the combined test_klp_convert.klp.o file has only a single
> > unresolved "state_show" symbol in its symbol table:
> >
> > % eu-readelf --symbols lib/livepatch/test_klp_convert.klp.o
> >
> > Symbol table [35] '.symtab' contains 57 entries:
> > 47 local symbols String table: [36] '.strtab'
> > Num: Value Size Type Bind Vis Ndx Name
> > ...
> > 52: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
> > ...
> >
> > though the .rela.klp.module_relocs.vmlinux relocation section has two
> > entries:
> >
> > % eu-readelf --relocs lib/livepatch/test_klp_convert.klp.o
> >
> > Relocation section [17] '.rela.klp.module_relocs.vmlinux' for section [16] '.klp.module_relocs.vmlinux' at offset 0x446c8 contains 2 entries:
> > Offset Type Value Addend Name
> > 000000000000000000 X86_64_64 000000000000000000 +0 state_show
> > 0x0000000000000010 X86_64_64 000000000000000000 +0 state_show
> >
> > and it looks like the combined KLP_MODULE_RELOC still contains the two
> > unique symbol position values (2 and 3):
> >
> > % objcopy -O binary --only-section=.klp.module_relocs.vmlinux lib/livepatch/test_klp_convert.klp.o >(hexdump -C)
> > 00000000 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 |................|
> > 00000010 00 00 00 00 00 00 00 00 03 00 00 00 |............|
> > 0000001c
>
> Nice :/
>
> > Maybe we can work around this by modifying the annotation macros and/or
> > klp-convert, or live with this for now.
>
> The question is (and I'll check later. I cannot wrap my head around it
> now) if it at least works if there are two references of the same symbol
> in two different .o. It would be same state_show in this case and not two
> different ones. If it works then I think we can live with it for a while,
> because after all duplicate symbols are quite rare in the kernel.
I think it should work. There is only one case which causes problems.
There exists an ambiguous local symbol in one parent object and a
livepatch needs to reference two (or more) different versions of the
symbol. In that case there have to be more sympos records provided by a
user to help klp-convert distinguish between the cases. klp-convert is of
course confused because it currently cannot cope with that.
All other cases should be fine (theoretically).
So we need to come up with a solution which would help klp-convert
recognize the cases (symbol usage sites). Some sort of user annotation,
but I cannot think of anything right now.
Miroslav