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:
> http://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: http://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
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:
>> http://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: http://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
>
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:
>>> http://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: http://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
> 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
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