Re: [PATCH v2 0/8] klp-convert

Re: [PATCH v2 0/8] klp-convert

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:05AM -0300, Joao Moreira wrote:
> Livepatches may use symbols which are not contained in its own scope,
> and, because of that, may end up compiled with relocations that will
> only be resolved during module load. Yet, when the referenced symbols are
> not exported, solving this relocation requires information on the object
> that holds the symbol (either vmlinux or modules) and its position inside
> the object, as an object may contain multiple symbols with the same name.
> Providing such information must be done accordingly to what is specified
> in Documentation/livepatch/module-elf-format.txt.
> 
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem in two different forms: (i) by relying on a symbol map, which is
> built during kernel compilation, to automatically infers the relocation
> targeted symbol, and, when such inference is not possible (ii) by using
> annotations in the elf object to convert the relocation accordingly to
> the specification, enabling it to be handled by the livepatch loader.
> 
> Given the above, add support for symbol mapping in the form of
> Symbols.list file; add klp-convert tool; integrate klp-convert tool into
> kbuild; make livepatch modules discernible during kernel compilation
> pipeline; add data-structure and macros to enable users to annotate
> livepatch source code; make modpost stage compatible with livepatches;
> update livepatch-sample and update documentation.
> 
> The patch was tested under three use-cases:
> 
> use-case 1: There is a relocation in the lp that can be automatically
> resolved by klp-convert (tested by removing the annotations from
> samples/livepatch/livepatch-annotated-sample.c)
> 
> use-case 2: There is a relocation in the lp that cannot be automatically
> resolved, as the name of the respective symbol appears in multiple
> objects. The livepatch contains an annotation to enable a correct
> relocation - reproducible with this livepatch sample:
> www.livewire.com.br/suse/klp/livepatch-sample.1.c
> 
> use-case 3: There is a relocation in the lp that cannot be automatically
> resolved similarly as 2, but no annotation was provided in the livepatch,
> triggering an error during compilation - reproducible with this livepatch
> sample: www.livewire.com.br/suse/klp/livepatch-sample.2.c
> 
> In comparison with v1, this version of the patch-set:
> - was rebased to kernel 4.19
> - adds a Symbols.list versioning information
> - brings bug fixes and code improvements to klp-convert sources
> 
> This is a patch-set repost, given that a typo in a mail address prevented
> the original submission from being posted to lkml.
> 
> [ ... snip ... ]

Hi Joao,

Apologies for taking so long to get to this patchset, but I finally
spent last week reviewing and testing.  My goal was to write a klp
self-test based on the implementation and your sample module.  Along the
way I spotted a few minor bugs and other small suggestions.  Instead of
dumping a bunch of code or patch content in my replies, I posted my
rebase and modified branch here:

  https://github.com/joe-lawrence/linux/tree/klp-convert-v2-rebase-review

I added subject line [squash] tags to individual commits that should be
considered fixups for patches in this set.  Those commit logs also
contain [joe: description] tags and my sign-offs for that purpose as
well.

Hopefully this form of feedback will be easy to digest.  I'll reply to
the individual patchs here with high-level comments and a pointer to the
corresponding github patch.  Let me know if there are any questions.  If
it is easier to simply repost as a v3 with those changes, I can do that
as well -- whichever method is easier for you.

I'll also take comments on my work-in-progress self-test for
klp-convert:

  [new] livepatch/selftests: add klp-convert 
  https://github.com/torvalds/linux/commit/b0d858b9356d3c909096509a0f18e092b739b44f

At the moment, it consists of two livepatch modules (I'd prefer to
consolidate, but ran into an issue with klp-convert and multiple object
files) and verifies that livepatch can correctly resolve sympos of 0/1,
2 for unique/non-uniquely named strings and functions.

-- Joe

Re: [PATCH v2 0/8] klp-convert

[Joao Moreira]

On 3/18/19 4:18 PM, Joe Lawrence wrote:
> On Fri, Mar 01, 2019 at 11:13:05AM -0300, Joao Moreira wrote:
>> Livepatches may use symbols which are not contained in its own scope,
>> and, because of that, may end up compiled with relocations that will
>> only be resolved during module load. Yet, when the referenced symbols are
>> not exported, solving this relocation requires information on the object
>> that holds the symbol (either vmlinux or modules) and its position inside
>> the object, as an object may contain multiple symbols with the same name.
>> Providing such information must be done accordingly to what is specified
>> in Documentation/livepatch/module-elf-format.txt.
>>
>> Currently, there is no trivial way to embed the required information as
>> requested in the final livepatch elf object. klp-convert solves this
>> problem in two different forms: (i) by relying on a symbol map, which is
>> built during kernel compilation, to automatically infers the relocation
>> targeted symbol, and, when such inference is not possible (ii) by using
>> annotations in the elf object to convert the relocation accordingly to
>> the specification, enabling it to be handled by the livepatch loader.
>>
>> Given the above, add support for symbol mapping in the form of
>> Symbols.list file; add klp-convert tool; integrate klp-convert tool into
>> kbuild; make livepatch modules discernible during kernel compilation
>> pipeline; add data-structure and macros to enable users to annotate
>> livepatch source code; make modpost stage compatible with livepatches;
>> update livepatch-sample and update documentation.
>>
>> The patch was tested under three use-cases:
>>
>> use-case 1: There is a relocation in the lp that can be automatically
>> resolved by klp-convert (tested by removing the annotations from
>> samples/livepatch/livepatch-annotated-sample.c)
>>
>> use-case 2: There is a relocation in the lp that cannot be automatically
>> resolved, as the name of the respective symbol appears in multiple
>> objects. The livepatch contains an annotation to enable a correct
>> relocation - reproducible with this livepatch sample:
>> www.livewire.com.br/suse/klp/livepatch-sample.1.c
>>
>> use-case 3: There is a relocation in the lp that cannot be automatically
>> resolved similarly as 2, but no annotation was provided in the livepatch,
>> triggering an error during compilation - reproducible with this livepatch
>> sample: www.livewire.com.br/suse/klp/livepatch-sample.2.c
>>
>> In comparison with v1, this version of the patch-set:
>> - was rebased to kernel 4.19
>> - adds a Symbols.list versioning information
>> - brings bug fixes and code improvements to klp-convert sources
>>
>> This is a patch-set repost, given that a typo in a mail address prevented
>> the original submission from being posted to lkml.
>>
>> [ ... snip ... ]
> 
> Hi Joao,
> 
> Apologies for taking so long to get to this patchset, but I finally
> spent last week reviewing and testing.  My goal was to write a klp
> self-test based on the implementation and your sample module.  Along the
> way I spotted a few minor bugs and other small suggestions.  Instead of
> dumping a bunch of code or patch content in my replies, I posted my
> rebase and modified branch here:
> 
>    https://github.com/joe-lawrence/linux/tree/klp-convert-v2-rebase-review
> 
> I added subject line [squash] tags to individual commits that should be
> considered fixups for patches in this set.  Those commit logs also
> contain [joe: description] tags and my sign-offs for that purpose as
> well.
> 
> Hopefully this form of feedback will be easy to digest.  I'll reply to
> the individual patchs here with high-level comments and a pointer to the
> corresponding github patch.  Let me know if there are any questions.  If
> it is easier to simply repost as a v3 with those changes, I can do that
> as well -- whichever method is easier for you.
> 

Hi Joe, again thanks a lot for the review. To the things which I had 
something to say, I already replied to the respective messages. If none 
has issues with the fix for the ppc64le .TOC. symbols issue (on patch 
5/8) and with the fix for the multi-used-m Makefile (on patch 2/8), I 
guess we are good for moving forward to a v3.

If you don't mind, this is fine by me that you squash the changes and 
post the newer version. In fact, I can't figure out why my patch 
submissions did not appear in lkml (if someone knows what could be the 
reason, please, let me know), so I guess it would be nice to have it 
reachable this time.

Tks,
Joao

> I'll also take comments on my work-in-progress self-test for
> klp-convert:
> 
>    [new] livepatch/selftests: add klp-convert
>    https://github.com/torvalds/linux/commit/b0d858b9356d3c909096509a0f18e092b739b44f
> 
> At the moment, it consists of two livepatch modules (I'd prefer to
> consolidate, but ran into an issue with klp-convert and multiple object
> files) and verifies that livepatch can correctly resolve sympos of 0/1,
> 2 for unique/non-uniquely named strings and functions.
> 
> -- Joe
> 

Re: [PATCH v2 0/8] klp-convert

[Joe Lawrence]

On 3/26/19 4:18 PM, Joao Moreira wrote:
> 
> 
> On 3/18/19 4:18 PM, Joe Lawrence wrote:
>> On Fri, Mar 01, 2019 at 11:13:05AM -0300, Joao Moreira wrote:
>>> Livepatches may use symbols which are not contained in its own scope,
>>> and, because of that, may end up compiled with relocations that will
>>> only be resolved during module load. Yet, when the referenced symbols are
>>> not exported, solving this relocation requires information on the object
>>> that holds the symbol (either vmlinux or modules) and its position inside
>>> the object, as an object may contain multiple symbols with the same name.
>>> Providing such information must be done accordingly to what is specified
>>> in Documentation/livepatch/module-elf-format.txt.
>>>
>>> Currently, there is no trivial way to embed the required information as
>>> requested in the final livepatch elf object. klp-convert solves this
>>> problem in two different forms: (i) by relying on a symbol map, which is
>>> built during kernel compilation, to automatically infers the relocation
>>> targeted symbol, and, when such inference is not possible (ii) by using
>>> annotations in the elf object to convert the relocation accordingly to
>>> the specification, enabling it to be handled by the livepatch loader.
>>>
>>> Given the above, add support for symbol mapping in the form of
>>> Symbols.list file; add klp-convert tool; integrate klp-convert tool into
>>> kbuild; make livepatch modules discernible during kernel compilation
>>> pipeline; add data-structure and macros to enable users to annotate
>>> livepatch source code; make modpost stage compatible with livepatches;
>>> update livepatch-sample and update documentation.
>>>
>>> The patch was tested under three use-cases:
>>>
>>> use-case 1: There is a relocation in the lp that can be automatically
>>> resolved by klp-convert (tested by removing the annotations from
>>> samples/livepatch/livepatch-annotated-sample.c)
>>>
>>> use-case 2: There is a relocation in the lp that cannot be automatically
>>> resolved, as the name of the respective symbol appears in multiple
>>> objects. The livepatch contains an annotation to enable a correct
>>> relocation - reproducible with this livepatch sample:
>>> www.livewire.com.br/suse/klp/livepatch-sample.1.c
>>>
>>> use-case 3: There is a relocation in the lp that cannot be automatically
>>> resolved similarly as 2, but no annotation was provided in the livepatch,
>>> triggering an error during compilation - reproducible with this livepatch
>>> sample: www.livewire.com.br/suse/klp/livepatch-sample.2.c
>>>
>>> In comparison with v1, this version of the patch-set:
>>> - was rebased to kernel 4.19
>>> - adds a Symbols.list versioning information
>>> - brings bug fixes and code improvements to klp-convert sources
>>>
>>> This is a patch-set repost, given that a typo in a mail address prevented
>>> the original submission from being posted to lkml.
>>>
>>> [ ... snip ... ]
>>
>> Hi Joao,
>>
>> Apologies for taking so long to get to this patchset, but I finally
>> spent last week reviewing and testing.  My goal was to write a klp
>> self-test based on the implementation and your sample module.  Along the
>> way I spotted a few minor bugs and other small suggestions.  Instead of
>> dumping a bunch of code or patch content in my replies, I posted my
>> rebase and modified branch here:
>>
>>     https://github.com/joe-lawrence/linux/tree/klp-convert-v2-rebase-review
>>
>> I added subject line [squash] tags to individual commits that should be
>> considered fixups for patches in this set.  Those commit logs also
>> contain [joe: description] tags and my sign-offs for that purpose as
>> well.
>>
>> Hopefully this form of feedback will be easy to digest.  I'll reply to
>> the individual patchs here with high-level comments and a pointer to the
>> corresponding github patch.  Let me know if there are any questions.  If
>> it is easier to simply repost as a v3 with those changes, I can do that
>> as well -- whichever method is easier for you.
>>
> 
> Hi Joe, again thanks a lot for the review. To the things which I had
> something to say, I already replied to the respective messages. If none
> has issues with the fix for the ppc64le .TOC. symbols issue (on patch
> 5/8) and with the fix for the multi-used-m Makefile (on patch 2/8), I
> guess we are good for moving forward to a v3.
 >> If you don't mind, this is fine by me that you squash the changes and
> post the newer version. In fact, I can't figure out why my patch
> submissions did not appear in lkml (if someone knows what could be the
> reason, please, let me know), so I guess it would be nice to have it
> reachable this time.

I can do that... what I will do is collate all the comments, squash 
revisions, and update logs accordingly.  I'll post it up on github to 
let the 0-day lkp bot run through it and you can take a look to double 
check the series, attributions, etc before I post it here.

BTW, something I *just* noticed when putting together that toy 
out-of-tree module to test out multi-object livepatch modules is that we 
aren't considering out-of-tree symbols in Symbols.list.

Perhaps we can save that for v4 or beyond, but maybe we want to 
re-arrange the klp-convert arguments to "klp-convert <input.ko> <out.ko> 
<Symbols.list> [Symbols.list ...]" where we treat everything after 
<out.ko> as a symbol list file?  (This would assume we would generate a 
separate out-of-tree module Symbols.list file.)    /thinking-out-loud

-- Joe

Re: [PATCH v2 0/8] klp-convert

[Miroslav Benes]

 
> BTW, something I *just* noticed when putting together that toy out-of-tree
> module to test out multi-object livepatch modules is that we aren't
> considering out-of-tree symbols in Symbols.list.
> 
> Perhaps we can save that for v4 or beyond, but maybe we want to re-arrange the
> klp-convert arguments to "klp-convert <input.ko> <out.ko> <Symbols.list>
> [Symbols.list ...]" where we treat everything after <out.ko> as a symbol list
> file?  (This would assume we would generate a separate out-of-tree module
> Symbols.list file.)    /thinking-out-loud

I understand it could help the testing quite a bit right now, but do we 
care about out-of-tree modules in general?

Miroslav

Re: [PATCH v2 0/8] klp-convert

[Joe Lawrence]

On 4/4/19 7:49 AM, Miroslav Benes wrote:
>   
>> BTW, something I *just* noticed when putting together that toy out-of-tree
>> module to test out multi-object livepatch modules is that we aren't
>> considering out-of-tree symbols in Symbols.list.
>>
>> Perhaps we can save that for v4 or beyond, but maybe we want to re-arrange the
>> klp-convert arguments to "klp-convert <input.ko> <out.ko> <Symbols.list>
>> [Symbols.list ...]" where we treat everything after <out.ko> as a symbol list
>> file?  (This would assume we would generate a separate out-of-tree module
>> Symbols.list file.)    /thinking-out-loud
> 
> I understand it could help the testing quite a bit right now, but do we
> care about out-of-tree modules in general?
> 

Yeah, this was only something I hit because I found it easier to 
construct OOT modules to test and share with Joao.

I mentioned it because kpatch-build supports OOT and apparently some 
folks are using it:

   https://github.com/dynup/kpatch/pull/923

But like I said, we can push that off for another day for now.

-- Joe

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

[Joe Lawrence]

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 <jmoreira@suse.de>
> ---
>  .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

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

[Miroslav Benes]

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

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

[Joao Moreira]

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 <jmoreira@suse.de> 

--- 

  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
> 

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

[Joe Lawrence]

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 <jmoreira@suse.de>
> 
> ---
> 
>    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

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

[Joao Moreira]

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 <jmoreira@suse.de>
>>
>> ---
>>
>>    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 <jmoreira@suse.de> 

--- 

  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

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

[Joe Lawrence]

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

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

[Joe Lawrence]

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 <joe.lawrence@redhat.com>");
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)

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

[Joe Lawrence]

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 <joe.lawrence@redhat.com>");
> 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

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

[Miroslav Benes]

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

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

[Joe Lawrence]

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

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

[Miroslav Benes]

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

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

[Miroslav Benes]

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

Re: [PATCH v2 3/8] livepatch: Add klp-convert tool

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:08AM -0300, Joao Moreira wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Livepatches may use symbols which are not contained in its own scope,
> and, because of that, may end up compiled with relocations that will
> only be resolved during module load. Yet, when the referenced symbols
> are not exported, solving this relocation requires information on the
> object that holds the symbol (either vmlinux or modules) and its
> position inside the object, as an object may contain multiple symbols
> with the same name. Providing such information must be done
> accordingly to what is specified in
> Documentation/livepatch/module-elf-format.txt.
> 
> Currently, there is no trivial way to embed the required information
> as requested in the final livepatch elf object. klp-convert solves
> this problem in two different forms: (i) by relying on Symbols.list,
> which is built during kernel compilation, to automatically infer the
> relocation targeted symbol, and, when such inference is not possible
> (ii) by using annotations in the elf object to convert the relocation
> accordingly to the specification, enabling it to be handled by the
> livepatch loader.
> 
> Given the above, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
> 
> The core file of klp-convert is scripts/livepatch/klp-convert.c, which
> implements the heuristics used to solve the relocations and the
> conversion of unresolved symbols into the expected format, as defined
> in [1].
> 
> klp-convert receives as arguments the Symbols.list file, an input
> livepatch module to be converted and the output name for the converted
> livepatch. When it starts running, klp-convert parses Symbols.list and
> builds two internal lists of symbols, one containing the exported and
> another containing the non-exported symbols. Then, by parsing the rela
> sections in the elf object, klp-convert identifies which symbols must
> be converted, which are those unresolved and that do not have a
> corresponding exported symbol, and attempts to convert them
> accordingly to the specification.
> 
> By using Symbols.list, klp-convert identifies which symbols have names
> that only appear in a single kernel object, thus being capable of
> resolving these cases without the intervention of the developer. When
> various homonymous symbols exist through kernel objects, it is not
> possible to infer the right one, thus klp-convert falls back into
> using developer annotations. If these were not provided, then the tool
> will print a list with all acceptable targets for the symbol being
> processed.
> 
> Annotations in the context of klp-convert are accessible as struct
> klp_module_reloc entries in sections named
> .klp.module_relocs.<objname>. These entries are pairs of symbol
> references and positions which are to be resolved against definitions
> in <objname>.
> 
> Define the structure klp_module_reloc in
> include/linux/uapi/livepatch.h allowing developers to annotate the
> livepatch source code with it.
> 
> klp-convert relies on libelf and on a list implementation. Add files
> scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a
> libelf interfacing layer and scripts/livepatch/list.h, which is a
> list implementation.
> 
> Update Makefiles to correctly support the compilation of the new tool,
> update MAINTAINERS file and add a .gitignore file.
> 
> [1] - Documentation/livepatch/module-elf-format.txt
> 
> [khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
> at the end]
> [jmoreira:
> * add support to automatic relocation conversion in klp-convert.c
> * changelog]
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  MAINTAINERS                     |   1 +
>  include/uapi/linux/livepatch.h  |   5 +
>  scripts/Makefile                |   1 +
>  scripts/livepatch/.gitignore    |   1 +
>  scripts/livepatch/Makefile      |   7 +
>  scripts/livepatch/elf.c         | 696 ++++++++++++++++++++++++++++++++++++++++
>  scripts/livepatch/elf.h         |  84 +++++
>  scripts/livepatch/klp-convert.c | 610 +++++++++++++++++++++++++++++++++++
>  scripts/livepatch/klp-convert.h |  50 +++
>  scripts/livepatch/list.h        | 389 ++++++++++++++++++++++
>  10 files changed, 1844 insertions(+)
>  create mode 100644 scripts/livepatch/.gitignore
>  create mode 100644 scripts/livepatch/Makefile
>  create mode 100644 scripts/livepatch/elf.c
>  create mode 100644 scripts/livepatch/elf.h
>  create mode 100644 scripts/livepatch/klp-convert.c
>  create mode 100644 scripts/livepatch/klp-convert.h
>  create mode 100644 scripts/livepatch/list.h
> 
> [ ... snip ... ]
> diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
> new file mode 100644
> index 000000000000..f279dd3be1b7
> --- /dev/null
> +++ b/scripts/livepatch/elf.c
> @@ -0,0 +1,696 @@
> +/*
> + * elf.c - ELF access library
> + *
> + * Adapted from kpatch (https://github.com/dynup/kpatch):
> + * Copyright (C) 2013-2016 Josh Poimboeuf <jpoimboe@redhat.com>
> + * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> [ ... snip ... ]

We could use the newer, more succinct SPDX version:
/* SPDX-License-Identifier: GPL-2.0 */

> diff --git a/scripts/livepatch/elf.h b/scripts/livepatch/elf.h
> new file mode 100644
> index 000000000000..e8aa8f5fb3bc
> --- /dev/null
> +++ b/scripts/livepatch/elf.h
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (C) 2015-2016 Josh Poimboeuf <jpoimboe@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> [ ... snip ... ]

/* SPDX-License-Identifier: GPL-2.0 */

> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
> new file mode 100644
> index 000000000000..2a39d656c8d6
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.c
> @@ -0,0 +1,610 @@
> +/*
> + * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@redhat.com>
> + * Copyright (C) 2017 Joao Moreira   <jmoreira@suse.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */

/* SPDX-License-Identifier: GPL-2.0 */

> [ ... snip ... ]
> +
> +/* Removes symbols used for sympos annotation from livepatch elf object */
> +static void clear_sympos_symbols(struct section *sec, struct elf *klp_elf)
> +{
> +	struct symbol *sym, *aux;
> +
> +	list_for_each_entry_safe(sym, aux, &klp_elf->symbols, list) {
> +		if (sym->sec == sec) {
> +			list_del(&sym->list);
> +			free(sym);
> +		}
> +	}
> +}

Running klp-convert through 'valgrind --leak-check=full
--track-origins=yes', it found use-after-frees involving symbols that
were run through clear_sympos_symbols... later when must_convert() is
called against all of the rela->syms, the symbols are gone but the
corresponding relas are still allocated and live on their section
rela-list.

A similar use-after-free is present in update_relas() when we want to
elf_write_file() ... again, iterating through sections relas in which
their sym may have been freed.

Suggested fix here:

  [squash] livepatch/klp-convert: fix use-after-frees 
  https://github.com/torvalds/linux/commit/4e5f39e069ec39cac53ae4d7f3d15f05d17f47ff

There are also several other memory leak complaints from valgrind that I
cleaned up.  I didn't look at very error path, as that has a tendency to
clutter up the code, but for successful klp-convert invocations, this
should clean up the rest of the valgrind whining:

  [squash] livepatch/klp-convert: fix remaining memory leaks
  https://github.com/torvalds/linux/commit/ad7681937946bea430449b83f77622dbbe6300de 

> [ ... snip ... ]
> +
> +/* Checks if sympos is valid, otherwise prints valid sympos list */
> +static bool valid_sympos(struct sympos *sp)
> +{
> +	struct symbol_entry *e;
> +	int counter = 0;
> +
> +	list_for_each_entry(e, &symbols, list) {
> +		if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
> +		    (strcmp(e->object_name, sp->object_name) == 0)) {
> +			if (counter == sp->pos)
> +				return true;
> +			counter++;
> +		}
> +	}
> +
> +	WARN("Provided KLP_SYMPOS does not match a symbol: %s.%s,%d",
> +			sp->object_name, sp->symbol_name, sp->pos);
> +	print_valid_module_relocs(sp->symbol_name);
> +
> +	return false;
> +}

I believe there is an off-by-one error condition either here, or in
livepatch kernel core sympos disambiguator code (in which case, external
tools like kpatch-build would also need to be adjusted).

The scenarios that I've observed using klp-convert and kpatch-build:

  sympos == 0, uniquely named symbol
  sympos == 1, non-unique symbol name, first instance
  sympos == 2, non-unique symbol name, second instance
  ...

When I built a klp-convert selftest, I made sure that the target module
contained multiple symbols of the same name across two .o files.
However, when I tried to use a KLP_MODULE_RELOC annotation in the
livepatch to resolve to the second (ie, last) symbol, using a sympos=2,
klp-convert kept telling me that the only valid KLP_SYMPOS were 0 and 1.

The following code adjusts klp-convert to match the sympos logic, as I
understand it in livepatch/core.c and kpatch-build;

  [squash] livepatch/klp-convert: fix klp-convert off-by-one sympos 
  https://github.com/torvalds/linux/commit/1ed8e5baa98f7920bbbaa562278b3ed44552e01f

This change also adds a check that sympos == 0 is in fact a uniquely
named symbol.

> [ ... snip ... ]
> +
> +int main(int argc, const char **argv)
> +{
> +	const char *klp_in_module, *klp_out_module, *symbols_list;
> +	struct rela *rela, *tmprela;
> +	struct section *sec, *aux;
> +	struct sympos sp;
> +	struct elf *klp_elf;
> +
> +	if (argc != 4) {
> +		WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]);
> +		return -1;
> +	}
> +
> +	symbols_list = argv[1];
> +	klp_in_module = argv[2];
> +	klp_out_module = argv[3];
> +
> +	klp_elf = elf_open(klp_in_module);
> +	if (!klp_elf) {
> +		WARN("Unable to read elf file %s\n", klp_in_module);
> +		return -1;
> +	}
> +
> +	if (!load_syms_lists(symbols_list))
> +		return -1;
> +
> +	if (!load_usr_symbols(klp_elf)) {
> +		WARN("Unable to load user-provided sympos");
> +		return -1;
> +	}
> +
> +	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
> +		if (!is_rela_section(sec))
> +			continue;
> +
> +		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> +			if (!must_convert(rela->sym))
> +				continue;
> +
> +			if (!find_missing_position(rela->sym, &sp)) {
> +				WARN("Unable to find missing symbol");

A really minor suggestion, but I found it useful to tell the user
exactly which symbol was missing:

  [squash] livepatch/klp-convert: add missing symbol name to warning 
  https://github.com/torvalds/linux/commit/7a41a8f3d9b9e11f0c75914cb925af1486c1ecc6

> [ ... snip ... ]
> diff --git a/scripts/livepatch/klp-convert.h b/scripts/livepatch/klp-convert.h
> new file mode 100644
> index 000000000000..1f3da2d9430d
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.h
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@redhat.com>
> + * Copyright (C) 2017 Joao Moreira   <jmoreira@suse.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> [ ... snip ... ]

/* SPDX-License-Identifier: GPL-2.0 */

> diff --git a/scripts/livepatch/list.h b/scripts/livepatch/list.h
> new file mode 100644
> index 000000000000..b324eb29c3b6
> --- /dev/null
> +++ b/scripts/livepatch/list.h
> @@ -0,0 +1,389 @@
> [ ... snip ... ]

This file has no license at all, suggested:
/* SPDX-License-Identifier: GPL-2.0 */

-- Joe

Re: [PATCH v2 3/8] livepatch: Add klp-convert tool

[Miroslav Benes]

> > [ ... snip ... ]
> > +
> > +/* Checks if sympos is valid, otherwise prints valid sympos list */
> > +static bool valid_sympos(struct sympos *sp)
> > +{
> > +	struct symbol_entry *e;
> > +	int counter = 0;
> > +
> > +	list_for_each_entry(e, &symbols, list) {
> > +		if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
> > +		    (strcmp(e->object_name, sp->object_name) == 0)) {
> > +			if (counter == sp->pos)
> > +				return true;
> > +			counter++;
> > +		}
> > +	}
> > +
> > +	WARN("Provided KLP_SYMPOS does not match a symbol: %s.%s,%d",
> > +			sp->object_name, sp->symbol_name, sp->pos);
> > +	print_valid_module_relocs(sp->symbol_name);
> > +
> > +	return false;
> > +}
> 
> I believe there is an off-by-one error condition either here, or in
> livepatch kernel core sympos disambiguator code (in which case, external
> tools like kpatch-build would also need to be adjusted).
> 
> The scenarios that I've observed using klp-convert and kpatch-build:
> 
>   sympos == 0, uniquely named symbol
>   sympos == 1, non-unique symbol name, first instance
>   sympos == 2, non-unique symbol name, second instance
>   ...

Yes, that is exactly how we defined it back then.
 
> When I built a klp-convert selftest, I made sure that the target module
> contained multiple symbols of the same name across two .o files.
> However, when I tried to use a KLP_MODULE_RELOC annotation in the
> livepatch to resolve to the second (ie, last) symbol, using a sympos=2,
> klp-convert kept telling me that the only valid KLP_SYMPOS were 0 and 1.
> 
> The following code adjusts klp-convert to match the sympos logic, as I
> understand it in livepatch/core.c and kpatch-build;
> 
>   [squash] livepatch/klp-convert: fix klp-convert off-by-one sympos 
>   https://github.com/torvalds/linux/commit/1ed8e5baa98f7920bbbaa562278b3ed44552e01f
> 
> This change also adds a check that sympos == 0 is in fact a uniquely
> named symbol.

Looks good to me. Maybe we can even make it simpler and use similar 
approach as in klp_find_callback() and klp_find_object_symbol() from 
kernel/livepatch/core.c. On the other hand, given that we want to print 
useful output in all cases, the code might happen to be more complicated 
and definitely ugly. 
 
> > [ ... snip ... ]
> > +
> > +int main(int argc, const char **argv)
> > +{
> > +	const char *klp_in_module, *klp_out_module, *symbols_list;
> > +	struct rela *rela, *tmprela;
> > +	struct section *sec, *aux;
> > +	struct sympos sp;
> > +	struct elf *klp_elf;
> > +
> > +	if (argc != 4) {
> > +		WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]);
> > +		return -1;
> > +	}
> > +
> > +	symbols_list = argv[1];
> > +	klp_in_module = argv[2];
> > +	klp_out_module = argv[3];
> > +
> > +	klp_elf = elf_open(klp_in_module);
> > +	if (!klp_elf) {
> > +		WARN("Unable to read elf file %s\n", klp_in_module);
> > +		return -1;
> > +	}
> > +
> > +	if (!load_syms_lists(symbols_list))
> > +		return -1;
> > +
> > +	if (!load_usr_symbols(klp_elf)) {
> > +		WARN("Unable to load user-provided sympos");
> > +		return -1;
> > +	}
> > +
> > +	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
> > +		if (!is_rela_section(sec))
> > +			continue;
> > +
> > +		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> > +			if (!must_convert(rela->sym))
> > +				continue;
> > +
> > +			if (!find_missing_position(rela->sym, &sp)) {
> > +				WARN("Unable to find missing symbol");
> 
> A really minor suggestion, but I found it useful to tell the user
> exactly which symbol was missing:
> 
>   [squash] livepatch/klp-convert: add missing symbol name to warning 
>   https://github.com/torvalds/linux/commit/7a41a8f3d9b9e11f0c75914cb925af1486c1ecc6

I think that Joao has exactly the same hunk staged somewhere 
already. You are right. The message is not much informative.

Miroslav

Re: [PATCH v2 3/8] livepatch: Add klp-convert tool

[Joao Moreira]

On 3/20/19 4:36 PM, Miroslav Benes wrote:
>>> [ ... snip ... ]
>>> +
>>> +/* Checks if sympos is valid, otherwise prints valid sympos list */
>>> +static bool valid_sympos(struct sympos *sp)
>>> +{
>>> +	struct symbol_entry *e;
>>> +	int counter = 0;
>>> +
>>> +	list_for_each_entry(e, &symbols, list) {
>>> +		if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
>>> +		    (strcmp(e->object_name, sp->object_name) == 0)) {
>>> +			if (counter == sp->pos)
>>> +				return true;
>>> +			counter++;
>>> +		}
>>> +	}
>>> +
>>> +	WARN("Provided KLP_SYMPOS does not match a symbol: %s.%s,%d",
>>> +			sp->object_name, sp->symbol_name, sp->pos);
>>> +	print_valid_module_relocs(sp->symbol_name);
>>> +
>>> +	return false;
>>> +}
>>
>> I believe there is an off-by-one error condition either here, or in
>> livepatch kernel core sympos disambiguator code (in which case, external
>> tools like kpatch-build would also need to be adjusted).
>>
>> The scenarios that I've observed using klp-convert and kpatch-build:
>>
>>    sympos == 0, uniquely named symbol
>>    sympos == 1, non-unique symbol name, first instance
>>    sympos == 2, non-unique symbol name, second instance
>>    ...
> 
> Yes, that is exactly how we defined it back then.
>   
>> When I built a klp-convert selftest, I made sure that the target module
>> contained multiple symbols of the same name across two .o files.
>> However, when I tried to use a KLP_MODULE_RELOC annotation in the
>> livepatch to resolve to the second (ie, last) symbol, using a sympos=2,
>> klp-convert kept telling me that the only valid KLP_SYMPOS were 0 and 1.
>>
>> The following code adjusts klp-convert to match the sympos logic, as I
>> understand it in livepatch/core.c and kpatch-build;
>>
>>    [squash] livepatch/klp-convert: fix klp-convert off-by-one sympos
>>    https://github.com/torvalds/linux/commit/1ed8e5baa98f7920bbbaa562278b3ed44552e01f
>>
>> This change also adds a check that sympos == 0 is in fact a uniquely
>> named symbol.
> 

Nice catch, thanks for it. All looks good to me.

> Looks good to me. Maybe we can even make it simpler and use similar
> approach as in klp_find_callback() and klp_find_object_symbol() from
> kernel/livepatch/core.c. On the other hand, given that we want to print
> useful output in all cases, the code might happen to be more complicated
> and definitely ugly.

I don't have an opinion here.

>   
>>> [ ... snip ... ]
>>> +
>>> +int main(int argc, const char **argv)
>>> +{
>>> +	const char *klp_in_module, *klp_out_module, *symbols_list;
>>> +	struct rela *rela, *tmprela;
>>> +	struct section *sec, *aux;
>>> +	struct sympos sp;
>>> +	struct elf *klp_elf;
>>> +
>>> +	if (argc != 4) {
>>> +		WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]);
>>> +		return -1;
>>> +	}
>>> +
>>> +	symbols_list = argv[1];
>>> +	klp_in_module = argv[2];
>>> +	klp_out_module = argv[3];
>>> +
>>> +	klp_elf = elf_open(klp_in_module);
>>> +	if (!klp_elf) {
>>> +		WARN("Unable to read elf file %s\n", klp_in_module);
>>> +		return -1;
>>> +	}
>>> +
>>> +	if (!load_syms_lists(symbols_list))
>>> +		return -1;
>>> +
>>> +	if (!load_usr_symbols(klp_elf)) {
>>> +		WARN("Unable to load user-provided sympos");
>>> +		return -1;
>>> +	}
>>> +
>>> +	list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
>>> +		if (!is_rela_section(sec))
>>> +			continue;
>>> +
>>> +		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
>>> +			if (!must_convert(rela->sym))
>>> +				continue;
>>> +
>>> +			if (!find_missing_position(rela->sym, &sp)) {
>>> +				WARN("Unable to find missing symbol");
>>
>> A really minor suggestion, but I found it useful to tell the user
>> exactly which symbol was missing:
>>
>>    [squash] livepatch/klp-convert: add missing symbol name to warning
>>    https://github.com/torvalds/linux/commit/7a41a8f3d9b9e11f0c75914cb925af1486c1ecc6
> 
> I think that Joao has exactly the same hunk staged somewhere
> already. You are right. The message is not much informative.
> 

Yes, someone had the same idea for the klp-convert which we use inside 
SUSE. But it did not make it to this patch-set.

Nice that Joe added it here :)

> Miroslav
> 

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

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:11AM -0300, Joao Moreira wrote:
> From: Miroslav Benes <mbenes@suse.cz>
> 
> Currently, livepatch infrastructure in the kernel relies on
> MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
> kernel module loader knows a module is indeed livepatch module and can
> behave accordingly.
> 
> klp-convert, on the other hand relies on LIVEPATCH_* statement in the
> module's Makefile for exactly the same reason.
> 
> Remove dependency on modinfo and generate MODULE_INFO flag
> automatically in modpost when LIVEPATCH_* is defined in the module's
> Makefile. Generate a list of all built livepatch modules based on
> the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
> this list as an argument for modpost which will use it to identify
> livepatch modules.
> 
> As MODULE_INFO is no longer needed, remove it.
> 
> [jmoreira:
> * fix modpost.c (add_livepatch_flag) to update module structure with
> livepatch flag and prevent modpost from breaking due to unresolved
> symbols]
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  samples/livepatch/livepatch-sample.c |  1 -
>  scripts/Makefile.modpost             |  8 +++-
>  scripts/mod/modpost.c                | 84 +++++++++++++++++++++++++++++++++---
>  3 files changed, 85 insertions(+), 8 deletions(-)
> 
> [ ... snip ... ]

While we're here, we can make the same changes to all the other in-tree
livepatch self-test and sample modules:

  [squash] modpost/livepatch: apply modinfo flag to all in-tree livepatch modules
  https://github.com/torvalds/linux/commit/22c59f05c9618903a946d5f8f79bfa345d31de3c

-- Joe

Re: [PATCH v2 7/8] livepatch: Add sample livepatch module

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:12AM -0300, Joao Moreira wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Add a new livepatch sample in samples/livepatch/ to make use of
> symbols that must be post-processed to enable load-time relocation
> resolution. As the new sample is to be used as an example, it is
> annotated with KLP_MODULE_RELOC and with KLP_SYMPOS macros.
> 
> The livepatch sample updates the function cmdline_proc_show to
> print the string referenced by the symbol saved_command_line
> appended by the string "livepatch=1".
> 
> Update livepatch-sample.c to remove livepatch MODULE_INFO
> statement.
> 
> [jmoreira:
> * update module to use KLP_SYMPOS
> * Comments on symbol resolution scheme
> * Update Makefile
> * Remove MODULE_INFO statement
> * Changelog
> ]
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  samples/livepatch/Makefile                     |   2 +
>  samples/livepatch/livepatch-annotated-sample.c | 113 +++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
>  create mode 100644 samples/livepatch/livepatch-annotated-sample.c
> 
> [ ... snip ... ]
>
> diff --git a/samples/livepatch/livepatch-annotated-sample.c b/samples/livepatch/livepatch-annotated-sample.c
> new file mode 100644
> index 000000000000..44d9e9542db1
> --- /dev/null
> +++ b/samples/livepatch/livepatch-annotated-sample.c

Super small nit, but I shuffled the annotated-sample code around so it
better matched the original sample.  This minimized the diff when I
compared the two versions to see what was added for sympos annotation:

  [squash] livepatch: adjust annotated sample formatting 
  https://github.com/torvalds/linux/commit/ce49c70e85cf87e513de3bcf64953de191c3c6d7

> [ ... snip ... ]
>
> +static int livepatch_init(void)
> +{
> +	int ret;
> +
> +	ret = klp_register_patch(&patch);
> +	if (ret)
> +		return ret;
> +	ret = klp_enable_patch(&patch);
> +	if (ret) {
> +		WARN_ON(klp_unregister_patch(&patch));
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void livepatch_exit(void)
> +{
> +	WARN_ON(klp_unregister_patch(&patch));
> +}

v5.0 deprecates klp_register_patch(), klp_unregister_patch(), and
klp_disable_patch(), so adjust the sample accordingly:

  [squash] livepatch: rebase the annotated sample 
  https://github.com/torvalds/linux/commit/fc04952376cfd8281eb663bd2ce18fec27eb42b2

-- Joe

Re: [PATCH v2 8/8] documentation: Update on livepatch elf format

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:13AM -0300, Joao Moreira wrote:
> Add a section to Documentation/livepatch/module-elf-format.txt
> describing how klp-convert works for fixing relocations.
> 
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  Documentation/livepatch/module-elf-format.txt | 47 ++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
> index f21a5289a09c..6b0259dfab49 100644
> --- a/Documentation/livepatch/module-elf-format.txt
> +++ b/Documentation/livepatch/module-elf-format.txt
> @@ -2,7 +2,8 @@
>  Livepatch module Elf format
>  ===========================
>  
> [ ... snip ... ]
>
> +--------------------------------------------------
> +4.  Automatic conversion of unresolved relocations
> +--------------------------------------------------
> +Sometimes livepatches may operate on symbols which are not self-contained nor
> +exported. When this happens, these symbols remain unresolved in the elf object
> +and will trigger an error during the livepatch instantiation.
> +
> +Whenever possible, the kernel building infrastructure solves this problem
> +automatically. First, a symbol database containing information on all compiled
> +objects is built. Second, this database - a file named Symbols.list, placed in
> +the kernel source root directory - is used to identify targets for unresolved
> +relocations, converting them in the livepatch elf accordingly to the
> +specifications above-described. While the first staged is fully handled by the
                                                   ^^^^^^
nit: s/staged/stage

> +building system, the second is done by a tool called klp-convert, which can be
> +found in "scripts/livepatch".
> +
> +When an unresolved relocation has as target a symbol whose name is also used by
> +different symbols throughout the kernel, the relocation cannot be resolved
> +automatically. In these cases, the livepatch developer must add annotations to
> +the livepatch, making it possible for the system to identify which is the
> +correct target amongst multiple homonymous symbols. Such annotations must be
> +done through a data structure as follows:
> +
> +struct KLP_MODULE_RELOC(object) data_structure_name[] = {
> +	KLP_SYMPOS(symbol, pos)
> +};
> +
> +In the above example, object refers to the object file which contains the
> +symbol, being vmlinux or a module; symbol refers to the symbol name that will
> +be relocated and pos is its position in the object.
> [ ... snip ... ]

Should we be explicit about how position is counted?  First = 1, second
= 2, etc?  See the off-by-one bug I pointed out in the "livepatch: Add
klp-convert tool" patch earlier.

-- Joe

Re: [PATCH v2 8/8] documentation: Update on livepatch elf format

[Miroslav Benes]

> > +building system, the second is done by a tool called klp-convert, which can be
> > +found in "scripts/livepatch".
> > +
> > +When an unresolved relocation has as target a symbol whose name is also used by
> > +different symbols throughout the kernel, the relocation cannot be resolved
> > +automatically. In these cases, the livepatch developer must add annotations to
> > +the livepatch, making it possible for the system to identify which is the
> > +correct target amongst multiple homonymous symbols. Such annotations must be
> > +done through a data structure as follows:
> > +
> > +struct KLP_MODULE_RELOC(object) data_structure_name[] = {
> > +	KLP_SYMPOS(symbol, pos)
> > +};
> > +
> > +In the above example, object refers to the object file which contains the
> > +symbol, being vmlinux or a module; symbol refers to the symbol name that will
> > +be relocated and pos is its position in the object.
> > [ ... snip ... ]
> 
> Should we be explicit about how position is counted?  First = 1, second
> = 2, etc?  See the off-by-one bug I pointed out in the "livepatch: Add
> klp-convert tool" patch earlier.

We could, but I would add it to a general section somewhere and just add a 
reference here.

Documentation/livepatch/livepatch.txt says
"As an optional parameter, the symbol position in the kallsyms database 
can be used to disambiguate functions of the same name. This is not the 
absolute position in the database, but rather the order it has been found 
only for a particular object ( vmlinux or a kernel module )."

We can improve it.

Documentation/livepatch/module-elf-format.txt says
"[D] The position of the symbol in the object (as according to kallsyms)
    This is used to differentiate duplicate symbols within the same
    object. The symbol position is expressed numerically (0, 1, 2...).
    The symbol position of a unique symbol is 0."

It may even confuse someone.

So yes, I'd be for a change here and there.

Miroslav

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

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
> cmd_klp_convert invokes klp-convert with the right arguments for the
> conversion of unresolved symbols inside a livepatch.
> 
> [khlebnikov:
> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> * rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside
>   if_changed_rule compares cmd_link_module and cmd_ld_ko_o
> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> ]
> 
> [mbenes:
> * remove modinfo call. LIVEPATCH_ in Makefiled
> ]
> 
> [jmoreira:
> * split up: move the .livepatch file-based scheme for identifying
> livepatches to a previous patch, as it was required for correctly
> building Symbols.list there.
> ]
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  scripts/Kbuild.include   |  4 +++-
>  scripts/Makefile.modpost | 16 +++++++++++++++-
>  scripts/mod/modpost.c    |  6 +++++-
>  scripts/mod/modpost.h    |  1 +
>  4 files changed, 24 insertions(+), 3 deletions(-)
> 
> [ ... snip ... ]
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 7d4af0d0accb..da779a185218 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>                   -o $@ $(filter-out FORCE,$^) ;                         \
>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>  
> +SLIST = $(objtree)/Symbols.list
> +KLP_CONVERT = scripts/livepatch/klp-convert
> +quiet_cmd_klp_convert = KLP $@

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

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

> [ ... snip ... ]
>  
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 26bf886bd168..1dfc34d8b668 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1979,6 +1979,10 @@ static void read_symbols(const char *modname)
>  		license = get_next_modinfo(&info, "license", license);
>  	}
>  
> +	/* Livepatch modules have unresolved symbols resolved by klp-convert */
> +	if (get_modinfo(info.modinfo, info.modinfo_len, "livepatch"))
> +		mod->livepatch = 1;
> +
> [ ... snip ... ]

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

-- Joe

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

[Joe Lawrence]

On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
> cmd_klp_convert invokes klp-convert with the right arguments for the
> conversion of unresolved symbols inside a livepatch.
> 
> [khlebnikov:
> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> * rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside
>   if_changed_rule compares cmd_link_module and cmd_ld_ko_o
> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> ]
> 
> [mbenes:
> * remove modinfo call. LIVEPATCH_ in Makefiled
> ]
> 
> [jmoreira:
> * split up: move the .livepatch file-based scheme for identifying
> livepatches to a previous patch, as it was required for correctly
> building Symbols.list there.
> ]
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joao Moreira <jmoreira@suse.de>
> ---
>  scripts/Kbuild.include   |  4 +++-
>  scripts/Makefile.modpost | 16 +++++++++++++++-
>  scripts/mod/modpost.c    |  6 +++++-
>  scripts/mod/modpost.h    |  1 +
>  4 files changed, 24 insertions(+), 3 deletions(-)
> 
>
> [ ... snip ... ]
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 7d4af0d0accb..da779a185218 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>                   -o $@ $(filter-out FORCE,$^) ;                         \
>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>  
> +SLIST = $(objtree)/Symbols.list
> +KLP_CONVERT = scripts/livepatch/klp-convert
> +quiet_cmd_klp_convert = KLP $@
> +      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);				\
> +			$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
> +
> +define rule_ld_ko_o
> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
                           ^
Should there be a ';' semicolon here (and maybe a line-break) between
$(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?

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

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


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

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

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

but we can save that for another day.


-- Joe

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

[Joao Moreira]

On 2019-03-22 11:54, Joe Lawrence wrote:
> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
>> From: Josh Poimboeuf <jpoimboe@redhat.com>
>> 
>> Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
>> cmd_klp_convert invokes klp-convert with the right arguments for the
>> conversion of unresolved symbols inside a livepatch.
>> 
>> [khlebnikov:
>> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
>> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
>> * rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside
>>   if_changed_rule compares cmd_link_module and cmd_ld_ko_o
>> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
>> ]
>> 
>> [mbenes:
>> * remove modinfo call. LIVEPATCH_ in Makefiled
>> ]
>> 
>> [jmoreira:
>> * split up: move the .livepatch file-based scheme for identifying
>> livepatches to a previous patch, as it was required for correctly
>> building Symbols.list there.
>> ]
>> 
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>> Signed-off-by: Joao Moreira <jmoreira@suse.de>
>> ---
>>  scripts/Kbuild.include   |  4 +++-
>>  scripts/Makefile.modpost | 16 +++++++++++++++-
>>  scripts/mod/modpost.c    |  6 +++++-
>>  scripts/mod/modpost.h    |  1 +
>>  4 files changed, 24 insertions(+), 3 deletions(-)
>> 
>> 
>> [ ... snip ... ]
>> 
>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> index 7d4af0d0accb..da779a185218 100644
>> --- a/scripts/Makefile.modpost
>> +++ b/scripts/Makefile.modpost
>> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>>                   -o $@ $(filter-out FORCE,$^) ;                       
>>   \
>>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>> 
>> +SLIST = $(objtree)/Symbols.list
>> +KLP_CONVERT = scripts/livepatch/klp-convert
>> +quiet_cmd_klp_convert = KLP $@
>> +      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);				\
>> +			$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
>> +
>> +define rule_ld_ko_o
>> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>                            ^
> Should there be a ';' semicolon here (and maybe a line-break) between
> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?
> 
> I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
> really strange build errors that I traced to this line.  Without a
> semicolon, the build was trying to run a make command with a linker
> command smashed onto the end of it:
> 
>   make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m
> elf64lppc -T ./scripts/module-common.lds -T
> ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o
> crypto/xts.ko crypto/xts.o crypto/xts.mod.o
> 
> 
> Now, klp-convert looks like will need some ppc64le work as well, as 
> it's
> confused about .TOC. symbols:
> 
>     CC      samples/livepatch/livepatch-annotated-sample.mod.o
>     CC      samples/livepatch/livepatch-callbacks-busymod.mod.o
>     CC      samples/livepatch/livepatch-callbacks-demo.mod.o
>     CC      samples/livepatch/livepatch-callbacks-mod.mod.o
>     CC      samples/livepatch/livepatch-sample.mod.o
>     CC      samples/livepatch/livepatch-shadow-fix1.mod.o
>     CC      samples/livepatch/livepatch-shadow-fix2.mod.o
>     CC      samples/livepatch/livepatch-shadow-mod.mod.o
>     LD [M]  samples/livepatch/livepatch-annotated-sample.ko
>     LD [M]  samples/livepatch/livepatch-callbacks-demo.ko
>     KLP     samples/livepatch/livepatch-annotated-sample.ko
>     LD [M]  samples/livepatch/livepatch-callbacks-mod.ko
>     KLP     samples/livepatch/livepatch-callbacks-demo.ko
>     LD [M]  samples/livepatch/livepatch-sample.ko
>     LD [M]  samples/livepatch/livepatch-shadow-fix1.ko
>   klp-convert: Define KLP_SYMPOS for the symbol: .TOC.
>   Valid KLP_SYMPOS for symbol .TOC.:
> 
>   [ ... snip listing of all .TOC's across the kernel ... ]
> 
> but we can save that for another day.
> 

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

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

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

Thanks,
João

> 
> -- Joe

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

[Joe Lawrence]

On 3/22/19 12:37 PM, Joao Moreira wrote:
> On 2019-03-22 11:54, Joe Lawrence wrote:
>> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
>>> From: Josh Poimboeuf <jpoimboe@redhat.com>
>>>
>>> Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
>>> cmd_klp_convert invokes klp-convert with the right arguments for the
>>> conversion of unresolved symbols inside a livepatch.
>>>
>>> [khlebnikov:
>>> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
>>> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
>>> * rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside
>>>    if_changed_rule compares cmd_link_module and cmd_ld_ko_o
>>> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
>>> ]
>>>
>>> [mbenes:
>>> * remove modinfo call. LIVEPATCH_ in Makefiled
>>> ]
>>>
>>> [jmoreira:
>>> * split up: move the .livepatch file-based scheme for identifying
>>> livepatches to a previous patch, as it was required for correctly
>>> building Symbols.list there.
>>> ]
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>>> Signed-off-by: Joao Moreira <jmoreira@suse.de>
>>> ---
>>>   scripts/Kbuild.include   |  4 +++-
>>>   scripts/Makefile.modpost | 16 +++++++++++++++-
>>>   scripts/mod/modpost.c    |  6 +++++-
>>>   scripts/mod/modpost.h    |  1 +
>>>   4 files changed, 24 insertions(+), 3 deletions(-)
>>>
>>>
>>> [ ... snip ... ]
>>>
>>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>>> index 7d4af0d0accb..da779a185218 100644
>>> --- a/scripts/Makefile.modpost
>>> +++ b/scripts/Makefile.modpost
>>> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>>>                    -o $@ $(filter-out FORCE,$^) ;
>>>    \
>>>   	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>>>
>>> +SLIST = $(objtree)/Symbols.list
>>> +KLP_CONVERT = scripts/livepatch/klp-convert
>>> +quiet_cmd_klp_convert = KLP $@
>>> +      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);				\
>>> +			$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
>>> +
>>> +define rule_ld_ko_o
>>> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>>                             ^
>> Should there be a ';' semicolon here (and maybe a line-break) between
>> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?
>>
>> I didn't see this in my x86_64 VM, but on a ppc64le box, I kept getting
>> really strange build errors that I traced to this line.  Without a
>> semicolon, the build was trying to run a make command with a linker
>> command smashed onto the end of it:
>>
>>    make -f ./arch/powerpc/Makefile.postlink crypto/xts.ko ld -r  -EL -m
>> elf64lppc -T ./scripts/module-common.lds -T
>> ./arch/powerpc/kernel/module.lds --save-restore-funcs  --build-id  -o
>> crypto/xts.ko crypto/xts.o crypto/xts.mod.o
>>
>>
>> Now, klp-convert looks like will need some ppc64le work as well, as
>> it's
>> confused about .TOC. symbols:
>>
>>      CC      samples/livepatch/livepatch-annotated-sample.mod.o
>>      CC      samples/livepatch/livepatch-callbacks-busymod.mod.o
>>      CC      samples/livepatch/livepatch-callbacks-demo.mod.o
>>      CC      samples/livepatch/livepatch-callbacks-mod.mod.o
>>      CC      samples/livepatch/livepatch-sample.mod.o
>>      CC      samples/livepatch/livepatch-shadow-fix1.mod.o
>>      CC      samples/livepatch/livepatch-shadow-fix2.mod.o
>>      CC      samples/livepatch/livepatch-shadow-mod.mod.o
>>      LD [M]  samples/livepatch/livepatch-annotated-sample.ko
>>      LD [M]  samples/livepatch/livepatch-callbacks-demo.ko
>>      KLP     samples/livepatch/livepatch-annotated-sample.ko
>>      LD [M]  samples/livepatch/livepatch-callbacks-mod.ko
>>      KLP     samples/livepatch/livepatch-callbacks-demo.ko
>>      LD [M]  samples/livepatch/livepatch-sample.ko
>>      LD [M]  samples/livepatch/livepatch-shadow-fix1.ko
>>    klp-convert: Define KLP_SYMPOS for the symbol: .TOC.
>>    Valid KLP_SYMPOS for symbol .TOC.:
>>
>>    [ ... snip listing of all .TOC's across the kernel ... ]
>>
>> but we can save that for another day.
>>
> 
> Hi Joe, first of all, thank you for you in-depth review. I did not have
> the time to go through everything yet, and I'll reply properly to the
> other comments soon. Yet, I would like to add a quick note here, since
> you are testing ppc64le. ppc64le is exactly one of the things I have
> been dealing with in the last two days, specifically the .TOC. symbols
> (there is also another bug related to converting relocations of symbols
> with index 0, but this one is architecture agnostic).
> 
> If you would like to take a look, the fix is in the latest commit here:
> - https://github.com/SUSE/klp-convert/tree/fixppc64le
> 
> I would be very glad if you have any comment or input on that, and I
> guess that if the fix is that simple, I can manage to squash it into v3.

Hi João,

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

(No build klp-convert build complaints)

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

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

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

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

-- Joe

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

[Miroslav Benes]

On Fri, 22 Mar 2019, Joe Lawrence wrote:

> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
> > cmd_klp_convert invokes klp-convert with the right arguments for the
> > conversion of unresolved symbols inside a livepatch.
> > 
> > [khlebnikov:
> > * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
> > * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
> > * rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside
> >   if_changed_rule compares cmd_link_module and cmd_ld_ko_o
> > * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
> > ]
> > 
> > [mbenes:
> > * remove modinfo call. LIVEPATCH_ in Makefiled
> > ]
> > 
> > [jmoreira:
> > * split up: move the .livepatch file-based scheme for identifying
> > livepatches to a previous patch, as it was required for correctly
> > building Symbols.list there.
> > ]
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > Signed-off-by: Joao Moreira <jmoreira@suse.de>
> > ---
> >  scripts/Kbuild.include   |  4 +++-
> >  scripts/Makefile.modpost | 16 +++++++++++++++-
> >  scripts/mod/modpost.c    |  6 +++++-
> >  scripts/mod/modpost.h    |  1 +
> >  4 files changed, 24 insertions(+), 3 deletions(-)
> > 
> >
> > [ ... snip ... ]
> >
> > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > index 7d4af0d0accb..da779a185218 100644
> > --- a/scripts/Makefile.modpost
> > +++ b/scripts/Makefile.modpost
> > @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
> >                   -o $@ $(filter-out FORCE,$^) ;                         \
> >  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
> >  
> > +SLIST = $(objtree)/Symbols.list
> > +KLP_CONVERT = scripts/livepatch/klp-convert
> > +quiet_cmd_klp_convert = KLP $@
> > +      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);				\
> > +			$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
> > +
> > +define rule_ld_ko_o
> > +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>                            ^
> Should there be a ';' semicolon here (and maybe a line-break) between
> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?

Probably yes.

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

but "echo-" disappeared somewhere. Joao?

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

Miroslav

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

[Joao Moreira]

On 4/4/19 8:31 AM, Miroslav Benes wrote:
> On Fri, 22 Mar 2019, Joe Lawrence wrote:
> 
>> On Fri, Mar 01, 2019 at 11:13:10AM -0300, Joao Moreira wrote:
>>> From: Josh Poimboeuf <jpoimboe@redhat.com>
>>>
>>> Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
>>> cmd_klp_convert invokes klp-convert with the right arguments for the
>>> conversion of unresolved symbols inside a livepatch.
>>>
>>> [khlebnikov:
>>> * save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that
>>> * fix bashisms for debian where /bin/sh is a symlink to /bin/dash
>>> * rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside
>>>    if_changed_rule compares cmd_link_module and cmd_ld_ko_o
>>> * check modinfo -F livepatch only if CONFIG_LIVEPATCH is true
>>> ]
>>>
>>> [mbenes:
>>> * remove modinfo call. LIVEPATCH_ in Makefiled
>>> ]
>>>
>>> [jmoreira:
>>> * split up: move the .livepatch file-based scheme for identifying
>>> livepatches to a previous patch, as it was required for correctly
>>> building Symbols.list there.
>>> ]
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>>> Signed-off-by: Joao Moreira <jmoreira@suse.de>
>>> ---
>>>   scripts/Kbuild.include   |  4 +++-
>>>   scripts/Makefile.modpost | 16 +++++++++++++++-
>>>   scripts/mod/modpost.c    |  6 +++++-
>>>   scripts/mod/modpost.h    |  1 +
>>>   4 files changed, 24 insertions(+), 3 deletions(-)
>>>
>>>
>>> [ ... snip ... ]
>>>
>>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>>> index 7d4af0d0accb..da779a185218 100644
>>> --- a/scripts/Makefile.modpost
>>> +++ b/scripts/Makefile.modpost
>>> @@ -125,8 +125,22 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>>>                    -o $@ $(filter-out FORCE,$^) ;                         \
>>>   	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>>>   
>>> +SLIST = $(objtree)/Symbols.list
>>> +KLP_CONVERT = scripts/livepatch/klp-convert
>>> +quiet_cmd_klp_convert = KLP $@
>>> +      cmd_klp_convert = mv $@ $(@:.ko=.klp.o);				\
>>> +			$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
>>> +
>>> +define rule_ld_ko_o
>>> +	$(call cmd,ld_ko_o) $(cmd_ld_ko_o) ;				\
>>                             ^
>> Should there be a ';' semicolon here (and maybe a line-break) between
>> $(call cmd,ld_ko_o) and $(cmd_ld_ko_o)?
> 
> Probably yes.
> 
> Originally there was
> 	$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ;
> 
> but "echo-" disappeared somewhere. Joao?

Hi,

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

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

define rule_ld_ko_o 

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

Joe, do you mind fixing it for v3?

Thanks for spotting it.

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