Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Fri, 3 May 2019, Joe Lawrence wrote:
> On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote:
> >
> > [ ... snip ... ]
> >
> > Quick look, but it seems quite similar to the problem we had with
> > apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which
> > introduced it.
>
> That was an interesting diversion :) I think I grok the idea as:
>
> The kernel supports a few different code-patching methods:
>
> - SMP locks
> - alternatives
> - paravirt
> - jump labels
>
> and we need to ensure that they do not prematurely operate on unresolved
> klp-relocations. The solution that led to arch/x86/kernel/livepatch.c
> introduces "klp.arch" sections that rename such klp-relocations *and*
> their associated special section data structures. Processing is then
> deferred until after a relevant klp_object is loaded.
Correct.
> > I think, we should do the same for jump labels. Add
> > jump_label_apply_nops() from module_finalize() to
> > arch_klp_init_object_loaded() and convert jump_table ELF section so its
> > processing is delayed.
>
> Nod. Tthat sounds about right. There may be some more work yet in the
> static keys API as well, but I'm not 100%.
>
> > Which leads me another TODO... klp-convert does not convert even
> > .altinstructions and .parainstructions sections, so it has that problem as
> > well. If I remember, it was on Josh's TODO list when he first introduced
> > klp-convert. See cover.1477578530.git.jpoim...@redhat.com.
>
> In the RFC, Josh highlights a somewhat difficult problem regarding these
> special sections -- how to associate these special section data
> structures and their relocations to a specific klp_object.
>
> If I understand his suggestion, he proposed annotating livepatch module
> replacement functions as to stuff them into specially named ELF sections
> (which would include the klp_object name) and then bypass the existing
> livepatch registration API. No minor change.
>
> With that in mind, I'm starting to think of a game plan for klp-convert
> like:
>
> - phase 1: detect /abort unsupported sections
>
> - phase 2: manual annotations in livepatch modules (like
> KLP_MODULE_RELOC / SYMPOS, but for special sections) so
> that klp-convert can start building "klp.arch" sections
>
> - phase 3: livepatch API change above to support somewhat more
> automatic generation of phase 2 annotations
Looks good to me. First, I'd focus on something which covers (hopefully) a
vast majority cases and then we can do the rest. So yes, this seems to be
a good plan.
> > And of course we should look at the other supported architectures and
> > their module_finalize() functions. I have it on my TODO list somewhere,
> > but you know how it works with those :/. I am sure there are more hidden
> > surprises there.
>
> Hmm, well powerpc and s390 do appear to have processing for special
> sections as well ... but for the moment, I'm going to focus on x86 as
> that seems like enough work for now :)
Yes, please :).
> > > Detection
> > > -
> > >
> > > I can post ("livepatch/klp-convert: abort on static key conversion")
> > > here as a follow commit if it looks reasonable and folks wish to review
> > > it... or we can try and tackle static keys before merging klp-convert.
> >
> > Good idea. I'd rather fix it, but I think it could be a lot of work, so
> > something like this patch seems to be a good idea.
>
> I'm thinking of adding this in a commit so klp-convert can intercept
> these sections:
>
> static bool is_section_supported(char *sname)
> {
> if (strcmp(sname, ".rela.altinstructions") == 0)
> return false;
> if (strcmp(sname, ".rela.parainstructions") == 0)
> return false;
> if (strcmp(sname, ".rela__jump_table") == 0)
> return false;
> return true;
> }
>
> Right now my v4 collection has a bunch of small fixups and nitpick
> corrections. It feels like a good resting place for now before
> embarking on special section support, what do you think?
Yes.
Thanks
Miroslav
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote:
>
> [ ... snip ... ]
>
> Quick look, but it seems quite similar to the problem we had with
> apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which
> introduced it.
That was an interesting diversion :) I think I grok the idea as:
The kernel supports a few different code-patching methods:
- SMP locks
- alternatives
- paravirt
- jump labels
and we need to ensure that they do not prematurely operate on unresolved
klp-relocations. The solution that led to arch/x86/kernel/livepatch.c
introduces "klp.arch" sections that rename such klp-relocations *and*
their associated special section data structures. Processing is then
deferred until after a relevant klp_object is loaded.
> I think, we should do the same for jump labels. Add
> jump_label_apply_nops() from module_finalize() to
> arch_klp_init_object_loaded() and convert jump_table ELF section so its
> processing is delayed.
Nod. Tthat sounds about right. There may be some more work yet in the
static keys API as well, but I'm not 100%.
> Which leads me another TODO... klp-convert does not convert even
> .altinstructions and .parainstructions sections, so it has that problem as
> well. If I remember, it was on Josh's TODO list when he first introduced
> klp-convert. See cover.1477578530.git.jpoim...@redhat.com.
In the RFC, Josh highlights a somewhat difficult problem regarding these
special sections -- how to associate these special section data
structures and their relocations to a specific klp_object.
If I understand his suggestion, he proposed annotating livepatch module
replacement functions as to stuff them into specially named ELF sections
(which would include the klp_object name) and then bypass the existing
livepatch registration API. No minor change.
With that in mind, I'm starting to think of a game plan for klp-convert
like:
- phase 1: detect /abort unsupported sections
- phase 2: manual annotations in livepatch modules (like
KLP_MODULE_RELOC / SYMPOS, but for special sections) so
that klp-convert can start building "klp.arch" sections
- phase 3: livepatch API change above to support somewhat more
automatic generation of phase 2 annotations
> The selftest for the alternatives would be appreciated too. One day.
In the course of understanding the background behind
arch/x86/kernel/livepatch.c, I wrote a bunch of livepatch selftests that
try out simple examples of those special sections.
For alternatives, I did something like:
/* TODO: find reliably true/false features */
#define TRUE_FEATURE (X86_FEATURE_FPU)
#define FALSE_FEATURE (X86_FEATURE_VME)
...
klp_function1()
klp_function2()
klp_new_function()
asm (ALTERNATIVE("call klp_function1", "call klp_function2",
TRUE_FEATURE));
asm (ALTERNATIVE("call klp_function1", "call klp_function2",
FALSE_FEATURE));
asm (ALTERNATIVE("call mod_function1", "call mod_function2",
TRUE_FEATURE));
asm (ALTERNATIVE("call mod_function1", "call mod_function2",
FALSE_FEATURE));
asm (ALTERNATIVE("call mod_function2", "call mod_function1",
TRUE_FEATURE));
asm (ALTERNATIVE("call mod_function2", "call mod_function1",
FALSE_FEATURE));
so that I could see what kind of relocations were generated for default
and non-default instructions as well as module-local and then
unexported-extern functions.
Once we have klp-convert supporting these conversions, I think something
like that would suffice. In the meantime, I'm not sure how to create
"klp.arch" sectioned ELFs without something like kpatch-build.
> And of course we should look at the other supported architectures and
> their module_finalize() functions. I have it on my TODO list somewhere,
> but you know how it works with those :/. I am sure there are more hidden
> surprises there.
Hmm, well powerpc and s390 do appear to have processing for special
sections as well ... but for the moment, I'm going to focus on x86 as
that seems like enough work for now :)
> > Detection
> > -
> >
> > I can post ("livepatch/klp-convert: abort on static key conversion")
> > here as a follow commit if it looks reasonable and folks wish to review
> > it... or we can try and tackle static keys before merging klp-convert.
>
> Good idea. I'd rather fix it, but I think it could be a lot of work, so
> something like this patch seems to be a good idea.
I'm thinking of adding this in a commit so klp-convert can intercept
these sections:
static bool is_section_supported(char *sname)
{
if (strcmp(sname, ".rela.altinstructions") == 0)
return false;
if (strcmp(sname, ".rela.parainstructions") == 0)
return false;
if (strcmp(sname, ".rela__jump_table") == 0)
return false;
return true;
}
Right now my v4 collection has a bunch of small fixups and nitpick
corrections. It
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/24/19 3:13 PM, Joao Moreira wrote: Future Work --- I don't see an easy way to support multiple homonym symbols with unique values in the current livepatch module Elf format. The only solutions that come to mind right now include renaming homonym symbols somehow to retain the relocation->symbol relationship when separate object files are combined. Perhaps an intermediate linker step could make annotated symbols unique in some way to achieve this. /thinking out loud I'd set this aside for now and we can return to it later. I think it could be quite rare in practice. I agree, especially since we can detect this corner case and abort the translation. I was thinking about renaming the symbol too. We can extend the symbol naming convention we have now and deal with it in klp_resolve_symbols(), but maybe Josh will come up with something clever and cleaner. I think this could work well, but (sorry if I understood Joe's idea wrongly) not as a linker step. Instead of modifying the linker, I think we could create another tool and plug it into the kbuild pipeline prior to the livepatch module linking. This way, we would parse the .o elf files, check for homonyms and rename them based on a convention that is later understood by klp-convert, as suggested. My knowledge of the build tools is limited, so there was a bunch of hand-waving you couldn't see when I wrote that paragraph :) But yes, that is basically the idea: plugging into the kbuild pipeline to give these some kinda of .o-unique prefix that klp-convert would interpret and strip accordingly. If I am not missing something, this would fix the case where we have homonyms pointing to the same or different positions, without additional user intervention other then adding the SYMPOS annotations. If you consider this to be useful I can start experiencing. It's not the highest priority, but even a prototype of how to insert a script into the pipeline to achieve this would be massively time saving for myself. If renaming looks easy, we could try to work into the initial klp-convert patchset... if not, save it for a follow up enhancement. Thanks, -- Joe
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Wed, Apr 24, 2019 at 04:13:59PM -0300, Joao Moreira wrote: > > > Future Work > > > --- > > > > > > I don't see an easy way to support multiple homonym > > > symbols with unique values in the current livepatch module > > > Elf format. The only solutions that come to mind right now include > > > renaming homonym symbols somehow to retain the relocation->symbol > > > relationship when separate object files are combined. Perhaps an > > > intermediate linker step could make annotated symbols unique in some way > > > to achieve this. /thinking out loud > > > > I'd set this aside for now and we can return to it later. I think it could > > be quite rare in practice. > > > > I was thinking about renaming the symbol too. We can extend the symbol > > naming convention we have now and deal with it in klp_resolve_symbols(), > > but maybe Josh will come up with something clever and cleaner. > > I think this could work well, but (sorry if I understood Joe's idea wrongly) > not as a linker step. Instead of modifying the linker, I think we could > create another tool and plug it into the kbuild pipeline prior to the > livepatch module linking. This way, we would parse the .o elf files, check > for homonyms and rename them based on a convention that is later understood > by klp-convert, as suggested. > > If I am not missing something, this would fix the case where we have > homonyms pointing to the same or different positions, without additional > user intervention other then adding the SYMPOS annotations. > > If you consider this to be useful I can start experiencing. I tend to agree with Miroslav that it's probably ok to ignore this issue for now, as long as klp-convert can detect it and spit out an error. I assume the patch author could work around it with a kallsyms hack. If we encounter it much in the real world then we could figure out a solution at that point. -- Josh
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/24/19 3:19 PM, Miroslav Benes wrote:
[...]
Result: a small tweak to sympos_sanity_check() to relax its symbol
uniqueness verification: allow for duplicate
instances. Now it will only complain when a supplied symbol references
the same but a different .
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 82c27d219372..713835dfc9ec 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
}
}
-/* Checks if two or more elements in usr_symbols have the same name */
+/*
+ * Checks if two or more elements in usr_symbols have the same
+ * object and name, but different symbol position
+ */
static bool sympos_sanity_check(void)
{
bool sane = true;
@@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
list_for_each_entry(sp, _symbols, list) {
aux = list_next_entry(sp, list);
list_for_each_entry_from(aux, _symbols, list) {
- if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
+ if (sp->pos != aux->pos &&
+ strcmp(sp->object_name, aux->object_name) == 0 &&
+ strcmp(sp->symbol_name, aux->symbol_name) == 0)
WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d
vs. %s.%s,%d.",
sp->object_name, sp->symbol_name, sp->pos,
aux->object_name, aux->symbol_name, aux->pos);
Looks good and I'd definitely include it in v4.
Unique sympos
-
But even with the above workaround, specifying unique symbol positions
will (and should) result in a klp-convert complaint.
When modifying the test module with unique symbol position annotation
values (1 and 2 respectively):
test_klp_convert_multi_objs_a.c:
extern void *state_show;
...
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
KLP_SYMPOS(state_show, 1)
};
test_klp_convert_multi_objs_b.c:
extern void *state_show;
...
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
KLP_SYMPOS(state_show, 2)
};
Each object file's symbol table contains a "state_show" instance:
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep
'\'
30: 0 NOTYPE GLOBAL DEFAULTUNDEF state_show
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep
'\'
20: 0 NOTYPE GLOBAL DEFAULTUNDEF state_show
and the intermediate test_klp_convert_multi_objs.klp.o file contains a
combined .klp.module_relocs.vmlinux relocation section with two
KLP_SYMPOS structures, each with a unique value:
% objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
000 0001
010 0002
but the symbol tables were merged, sorted and non-unique symbols
removed, leaving a *single* unresolved "state_show" instance:
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o | grep
'\'
53: 0 NOTYPE GLOBAL DEFAULTUNDEF state_show
which means that even though we have separate relocations for each
"state_show" instance:
Relocation section [ 6] '.rela.text.unlikely' for section [ 5]
'.text.unlikely' at offset 0x44510 contains 11 entries:
Offset TypeValue Addend Name
0x0003 X86_64_32S 00 +0 state_show
...
0x0034 X86_64_32S 00 +0 state_show
...
they share the same rela->sym and there is no way to distinguish which
one correlates to which KLP_SYMPOS structure.
Future Work
---
I don't see an easy way to support multiple homonym
symbols with unique values in the current livepatch module
Elf format. The only solutions that come to mind right now include
renaming homonym symbols somehow to retain the relocation->symbol
relationship when separate object files are combined. Perhaps an
intermediate linker step could make annotated symbols unique in some way
to achieve this. /thinking out loud
I'd set this aside for now and we can return to it later. I think it could
be quite rare in practice.
I was thinking about renaming the symbol too. We can extend the symbol
naming convention we have now and deal with it in klp_resolve_symbols(),
but maybe Josh will come up with something clever and cleaner.
I think this could work well, but (sorry if I understood Joe's idea
wrongly) not as a linker step. Instead of modifying the linker, I think
we could create another tool and plug it into the kbuild pipeline prior
to the livepatch module linking. This way, we would parse the .o elf
files, check for homonyms and
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
[...]
> Result: a small tweak to sympos_sanity_check() to relax its symbol
> uniqueness verification: allow for duplicate
> instances. Now it will only complain when a supplied symbol references
> the same but a different .
>
> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
> index 82c27d219372..713835dfc9ec 100644
> --- a/scripts/livepatch/klp-convert.c
> +++ b/scripts/livepatch/klp-convert.c
> @@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf
> *klp_elf)
> }
> }
>
> -/* Checks if two or more elements in usr_symbols have the same name */
> +/*
> + * Checks if two or more elements in usr_symbols have the same
> + * object and name, but different symbol position
> + */
> static bool sympos_sanity_check(void)
> {
> bool sane = true;
> @@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
> list_for_each_entry(sp, _symbols, list) {
> aux = list_next_entry(sp, list);
> list_for_each_entry_from(aux, _symbols, list) {
> - if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
> + if (sp->pos != aux->pos &&
> + strcmp(sp->object_name, aux->object_name) == 0 &&
> + strcmp(sp->symbol_name, aux->symbol_name) == 0)
> WARN("Conflicting KLP_SYMPOS definition:
> %s.%s,%d vs. %s.%s,%d.",
> sp->object_name, sp->symbol_name, sp->pos,
> aux->object_name, aux->symbol_name, aux->pos);
Looks good and I'd definitely include it in v4.
> Unique sympos
> -
>
> But even with the above workaround, specifying unique symbol positions
> will (and should) result in a klp-convert complaint.
>
> When modifying the test module with unique symbol position annotation
> values (1 and 2 respectively):
>
> test_klp_convert_multi_objs_a.c:
>
> extern void *state_show;
> ...
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> KLP_SYMPOS(state_show, 1)
> };
>
> test_klp_convert_multi_objs_b.c:
>
> extern void *state_show;
> ...
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> KLP_SYMPOS(state_show, 2)
> };
>
>
> Each object file's symbol table contains a "state_show" instance:
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep
> '\'
> 30: 0 NOTYPE GLOBAL DEFAULTUNDEF state_show
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep
> '\'
> 20: 0 NOTYPE GLOBAL DEFAULTUNDEF state_show
>
> and the intermediate test_klp_convert_multi_objs.klp.o file contains a
> combined .klp.module_relocs.vmlinux relocation section with two
> KLP_SYMPOS structures, each with a unique value:
>
> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
> lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
>
> 000 0001
> 010 0002
>
> but the symbol tables were merged, sorted and non-unique symbols
> removed, leaving a *single* unresolved "state_show" instance:
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o |
> grep '\'
> 53: 0 NOTYPE GLOBAL DEFAULTUNDEF state_show
>
> which means that even though we have separate relocations for each
> "state_show" instance:
>
> Relocation section [ 6] '.rela.text.unlikely' for section [ 5]
> '.text.unlikely' at offset 0x44510 contains 11 entries:
> Offset TypeValue Addend Name
> 0x0003 X86_64_32S 00 +0 state_show
> ...
> 0x0034 X86_64_32S 00 +0 state_show
> ...
>
> they share the same rela->sym and there is no way to distinguish which
> one correlates to which KLP_SYMPOS structure.
>
>
> Future Work
> ---
>
> I don't see an easy way to support multiple homonym
> symbols with unique values in the current livepatch module
> Elf format. The only solutions that come to mind right now include
> renaming homonym symbols somehow to retain the relocation->symbol
> relationship when separate object files are combined. Perhaps an
> intermediate linker step could make annotated symbols unique in some way
> to achieve this. /thinking out loud
I'd set this aside for now and we can return to it later. I think it could
be quite rare in practice.
I was thinking about renaming the symbol too. We can extend the symbol
naming convention we have now and deal with it in klp_resolve_symbols(),
but maybe Josh will come up with something clever and cleaner.
Miroslav
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> TODO
>
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
Multiple object files
=
For v3, we tweaked the build scripts so that we could successfully build
a klp-converted livepatch module from multiple object files.
One interesting use-case is supporting two livepatch symbols of the same
name, but different object/position values.
An example target kernel module might be layed out like this:
test_klp_convert_mod.ko << target module is comprised of
two object files, each defining
test_klp_convert_mod_a.o their own local get_homonym_string()
get_homonym_string() function and homonym_string[]
homonym_string[] character arrays.
test_klp_convert_mod_b.o
get_homonym_string()
homonym_string[]
A look at interesting parts of the the module's symbol table:
% eu-readelf --symbols lib/livepatch/test_klp_convert_mod.ko | \
grep -e 'homonym' -e test_klp_convert_mod_
29: 0 FILELOCAL DEFAULT ABS
test_klp_convert_mod_a.c
32: 0010 8 FUNCLOCAL DEFAULT3
get_homonym_string
33: 17 OBJECT LOCAL DEFAULT5 homonym_string
37: 0 FILELOCAL DEFAULT ABS
test_klp_convert_mod_b.c
38: 0020 8 FUNCLOCAL DEFAULT3
get_homonym_string
39: 17 OBJECT LOCAL DEFAULT 11 homonym_string
suggests that any livepatch module that wishes to resolve to
test_klp_convert_mod_a.c :: get_homonym_string() should include an
annotation like:
file_a.c:
KLP_MODULE_RELOC(test_klp_convert_mod) relocs_a[] = {
KLP_SYMPOS(get_homonym_string, 1),
};
and to resolve test_klp_convert_mod_b.c :: get_homonym_string():
file_b.c:
KLP_MODULE_RELOC(test_klp_convert_mod) relocs_b[] = {
KLP_SYMPOS(get_homonym_string, 2),
};
Unfortunately klp-convert v3 will fail to build such livepatch module,
regardless of sympos values:
klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,2 vs.
vmlinux.state_show,1.
klp-convert: Unable to load user-provided sympos
make[1]: *** [scripts/Makefile.modpost:148:
lib/livepatch/test_klp_convert_multi_objs.ko] Error 255
This abort message can be traced to sympos_sanity_check() as it verifies
that there no duplicate symbol names found in its list of user specified
symbols (ie, those needing to be converted).
Common sympos
-
New test case and related fixup can be found here:
https://github.com/joe-lawrence/linux/commits/klp-convert-v4-multiple-objs
To better debug this issue, I added another selftest that demonstrates
this configuration in isolation. "show_state" is a popular kernel
function name (my VM reported 5 instances in kallsyms) so I chose that
symbol name.
Initially I specified the *same* symbol position (1) in both .c file
KLP_SYMPOS annotations. At the very least, we can trivially patch
klp-convert v3 to support annotations for the the same symbol value across object files.
Result: a small tweak to sympos_sanity_check() to relax its symbol
uniqueness verification: allow for duplicate
instances. Now it will only complain when a supplied symbol references
the same but a different .
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 82c27d219372..713835dfc9ec 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
}
}
-/* Checks if two or more elements in usr_symbols have the same name */
+/*
+ * Checks if two or more elements in usr_symbols have the same
+ * object and name, but different symbol position
+ */
static bool sympos_sanity_check(void)
{
bool sane = true;
@@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
list_for_each_entry(sp, _symbols, list) {
aux = list_next_entry(sp, list);
list_for_each_entry_from(aux, _symbols, list) {
- if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
+ if (sp->pos != aux->pos &&
+ strcmp(sp->object_name, aux->object_name) == 0 &&
+ strcmp(sp->symbol_name, aux->symbol_name) == 0)
WARN("Conflicting KLP_SYMPOS definition:
%s.%s,%d vs. %s.%s,%d.",
sp->object_name, sp->symbol_name, sp->pos,
aux->object_name, aux->symbol_name, aux->pos);
Unique sympos
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Tue, 16 Apr 2019, Joe Lawrence wrote: > On 4/10/19 11:50 AM, Joe Lawrence wrote: > > Hi folks, > > > > This is the third installment of the klp-convert tool for generating and > > processing livepatch symbols for livepatch module builds. For those > > following along at home, archive links to previous versions: > > > > RFC: > >https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/ > > v2: > > > > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/ > > > > (Note that I don't see v2 archived at lore, but that is a link to the > > most recent subthread that lore did catch.) > > > > > > 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. > > Hi Miroslav, > > I noticed that some binutils programs like gdb, objdump, etc. don't like the > .ko kernel objects that we're generating from this patchset, specifically > those with the additional '.klp.rela...text' livepatch symbol relocation > sections. > > For reference, I opened a new bugzilla with more details here: > https://sourceware.org/bugzilla/show_bug.cgi?id=24456 Another great catch. > And was about to ping the binutils mailing list about the assertion that is > tripping in bfd/elf.c. The thought occurred to me that you guys might already > be carrying a patch to workaround this issue? No, unfortunately we don't. At least I don't know about anything and a quick test on openSUSE Leap 15.0 confirms it (objdump -D gives "bad value"). Thanks Miroslav
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/10/19 11:50 AM, Joe Lawrence wrote: Hi folks, This is the third installment of the klp-convert tool for generating and processing livepatch symbols for livepatch module builds. For those following along at home, archive links to previous versions: RFC: https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/ v2: https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/ (Note that I don't see v2 archived at lore, but that is a link to the most recent subthread that lore did catch.) 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. Hi Miroslav, I noticed that some binutils programs like gdb, objdump, etc. don't like the .ko kernel objects that we're generating from this patchset, specifically those with the additional '.klp.rela...text' livepatch symbol relocation sections. For reference, I opened a new bugzilla with more details here: https://sourceware.org/bugzilla/show_bug.cgi?id=24456 And was about to ping the binutils mailing list about the assertion that is tripping in bfd/elf.c. The thought occurred to me that you guys might already be carrying a patch to workaround this issue? -- Joe
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/16/19 1:24 AM, Balbir Singh wrote: Could we get some details with examples or a sample, sorry I might be dense and missing simple information. The problem being solved is not clear to me from the changelog. Hi Balbir, I see that Miroslav has already pointed to documentation, samples and self-tests for the patchset, but here is a summary in my own words: kpatch-build constructs livepatch kernel modules by comparing a reference build with a patched build, combing through ELF object sections and extracting new and changed sections that include patched code. An alternative approach is "source-based", in which a livepatch kernel module is built (mostly entirely) using the kernel's build infrastructure. Sources for a livepatch are gathered ahead of time and then built like an ordinary kernel module. In either approach, there lies the problem of symbol visibility: how can a livepatch resolve symbols that a kernel module ordinarily can't. For example, file local or simply unexported symbols in across the kernel and other modules. Enter the concept of "livepatch symbols" described in module-elf-format.txt. kpatch-build already creates such "livepatch symbols" (see its create_klp_relasecs_and_syms()) and the livepatching core kernel code already knows how resolve such symbols at klp_object patch time (see klp_write_object_relocations()). The klp-convert tool and this supporting patchset would empower source-based-constructed livepatch modules to do the same. -- Joe
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
[...] > Current behavior > > > Not good. The livepatch successfully builds but crashes on load: > > % insmod lib/livepatch/test_klp_static_keys_mod.ko > % insmod lib/livepatch/test_klp_static_keys.ko > > BUG: unable to handle kernel NULL pointer dereference at 0010 > #PF error: [normal kernel read fault] > PGD 0 P4D 0 > Oops: [#1] SMP PTI > CPU: 3 PID: 9367 Comm: insmod Tainted: GE K 5.1.0-rc4+ #4 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014 > RIP: 0010:jump_label_apply_nops+0x3b/0x60 > Code: 02 00 00 48 c1 e5 04 48 01 dd 48 39 eb 74 3a 72 0b eb 36 48 83 c3 10 > 48 39 dd 76 2d 48 8b 43 08 48 89 c2 83 e0 01 48 83 e2 fc <48> 8b 54 13 10 83 > e2 01 38 c2 75 dd 48 89 df 31 f6 48 83 c3 10 e8 > RSP: 0018:a8874068fcf8 EFLAGS: 00010206 > RAX: RBX: c07fd000 RCX: 000d > RDX: 3f803000 RSI: a5077be0 RDI: c07fe540 > RBP: c07fd0a0 R08: a88740f43878 R09: a88740eed000 > R10: 00055a4b R11: a88740f43878 R12: a88740f430b8 > R13: R14: a88740f42df8 R15: 00042b01 > FS: 7f4f1dafb740() GS:9a81fbb8() knlGS: > CS: 0010 DS: ES: CR0: 80050033 > CR2: 0010 CR3: b5d8a000 CR4: 06e0 > Call Trace: >module_finalize+0x184/0x1c0 >load_module+0x1400/0x1910 >? kernel_read_file+0x18d/0x1c0 >? __do_sys_finit_module+0xa8/0x110 >__do_sys_finit_module+0xa8/0x110 >do_syscall_64+0x55/0x1a0 >entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x7f4f1cae82bd > > > Future work > --- > > At the very least, I think this call-chain ordering is wrong for > livepatch static key symbols: > > load_module > > apply_relocations > > post_relocation > module_finalize > jump_label_apply_nops << > > ... > > prepare_coming_module > blocking_notifier_call_chain(_notify_list, MODULE_STATE_COMING, > mod); > jump_label_module_notify > case MODULE_STATE_COMING > jump_label_add_module << > > do_init_module > > do_one_initcall(mod->init) > __init patch_init [kpatch-patch] > klp_register_patch > klp_init_patch > klp_for_each_object(patch, obj) > klp_init_object > klp_init_object_loaded > klp_write_object_relocations << > > blocking_notifier_call_chain(_notify_list, MODULE_STATE_LIVE, > mod); > jump_label_module_notify > case MODULE_STATE_LIVE > jump_label_invalidate_module_init > > where klp_write_object_relocations() is called way *after* > jump_label_apply_nops() and jump_label_add_module(). Quick look, but it seems quite similar to the problem we had with apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which introduced it. I think, we should do the same for jump labels. Add jump_label_apply_nops() from module_finalize() to arch_klp_init_object_loaded() and convert jump_table ELF section so its processing is delayed. Which leads me another TODO... klp-convert does not convert even .altinstructions and .parainstructions sections, so it has that problem as well. If I remember, it was on Josh's TODO list when he first introduced klp-convert. See cover.1477578530.git.jpoim...@redhat.com. The selftest for the alternatives would be appreciated too. One day. And of course we should look at the other supported architectures and their module_finalize() functions. I have it on my TODO list somewhere, but you know how it works with those :/. I am sure there are more hidden surprises there. > Detection > - > > I have been tinkering with some prototype code to defer > jump_label_apply_nops() and jump_label_add_module(), but it has been > slow going. I think the jist of it is that we're going to need to call > these dynamically when individual klp_objects are patched, not when the > livepatch module itself loads. If anyone with static key expertise > wants to jump in here, let me know. > > In the meantime, I cooked up a potential followup commit to detect > conversion of static key symbols and klp-convert failure. It basically > runs through the output .ko's ELF symbols and verifies that none of the > converted ones can be found as a .rela__jump_table relocated symbol. It > accurately catches the problematic references in test_klp_static_keys.ko > thus far. > > This was based on a similar issue reported as a bug against > kpatch-build, in which Josh wrote code to detect this scenario: > > https://github.com/dynup/kpatch/issues/946 > >
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Tue, 16 Apr 2019, Balbir Singh wrote:
> On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
> > Hi folks,
> >
> > This is the third installment of the klp-convert tool for generating and
> > processing livepatch symbols for livepatch module builds. For those
> > following along at home, archive links to previous versions:
> >
> > RFC:
> > https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/
> > v2:
> >
> > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/
> >
> > (Note that I don't see v2 archived at lore, but that is a link to the
> > most recent subthread that lore did catch.)
> >
> >
> > 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.
> >
>
>
> Could we get some details with examples or a sample, sorry I might be dense
> and missing simple information. The problem being solved is not clear to
> me from the changelog.
The patch set tries to solve the problem described in
Documentation/livepatch/module-elf-format.txt. Currently, there is no tool
in upstream to automatically generate the relocation records. kpatch-build
can do it and we'd like to have it as well. klp-convert should thus do the
job.
The sample module is introduced in patch 7 and you can look at the
selftests in patch 9 too.
Maybe we can describe the problem better in the cover letter, but on the
other hand patch changelogs are more important and they are good (well, in
my opinion of course).
> > 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 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, 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. For example. see the saved_command_line
> > variable in lib/livepatch/test_klp_convert2.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. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
> > in lib/livepatch/test_klp_convert{1,2}.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 by
> > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
> > lib/livepatch/test_klp_convert{1,2}.c.
> >
>
>
> Are these a part of the series?
Yes, see 9/9.
Regards,
Miroslav
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
> Hi folks,
>
> This is the third installment of the klp-convert tool for generating and
> processing livepatch symbols for livepatch module builds. For those
> following along at home, archive links to previous versions:
>
> RFC:
> https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/
> v2:
>
> https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/
>
> (Note that I don't see v2 archived at lore, but that is a link to the
> most recent subthread that lore did catch.)
>
>
> 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.
>
Could we get some details with examples or a sample, sorry I might be dense
and missing simple information. The problem being solved is not clear to
me from the changelog.
> 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 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, 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. For example. see the saved_command_line
> variable in lib/livepatch/test_klp_convert2.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. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
> in lib/livepatch/test_klp_convert{1,2}.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 by
> removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
> lib/livepatch/test_klp_convert{1,2}.c.
>
Are these a part of the series?
Balbir Singh
>
> Branches
>
>
> Since I spent some time tinkering with v2 and accumulated a bunch of
> fixes, I collected them up and am posting this new version. For review
> purposes, I posted two branches up to github:
>
> 1 - an expanded branch that with changes separate from the original
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded
>
> 2 - a squashed branch of (1) that comprises v3:
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3
>
> Non-trivial commits in the expanded branch have some extra commentary
> and details for debugging in the commit message that were dropped when
> squashing into their respective parent commits.
>
>
> TODO
>
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
>
>
> Changelogs
> --
>
> The commit changelogs were getting long, so I've moved them here for
> posterity and review purposes:
>
> livepatch: Create and include UAPI headers
> v2:
> - jmoreira: split up and changelog
> v3:
> - joe: convert to SPDX license tags
>
> kbuild: Support for Symbols.list creation
> v3:
> - jmoreira: adjust for multiobject livepatch
> - joe: add klpclean to PHONY
> - joe: align KLP prefix
>
> livepatch: Add klp-convert tool
> v2:
> - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
> at the end
> - jmoreira: add support to automatic relocation conversion in
> klp-convert.c, changelog
> v3:
> - joe: convert to SPDX license tags
> - jmoreira: add rela symbol name to WARNs
> - jmoreira: ignore relocations to .TOC and
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
> Hi folks,
>
> This is the third installment of the klp-convert tool for generating and
> processing livepatch symbols for livepatch module builds. For those
> following along at home, archive links to previous versions:
>
> RFC:
> https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/
> v2:
>
> https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/
>
> (Note that I don't see v2 archived at lore, but that is a link to the
> most recent subthread that lore did catch.)
>
>
> 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 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, 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. For example. see the saved_command_line
> variable in lib/livepatch/test_klp_convert2.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. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
> in lib/livepatch/test_klp_convert{1,2}.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 by
> removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
> lib/livepatch/test_klp_convert{1,2}.c.
>
>
> Branches
>
>
> Since I spent some time tinkering with v2 and accumulated a bunch of
> fixes, I collected them up and am posting this new version. For review
> purposes, I posted two branches up to github:
>
> 1 - an expanded branch that with changes separate from the original
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded
>
> 2 - a squashed branch of (1) that comprises v3:
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3
>
> Non-trivial commits in the expanded branch have some extra commentary
> and details for debugging in the commit message that were dropped when
> squashing into their respective parent commits.
>
>
> TODO
>
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
>
>
> Changelogs
> --
>
> The commit changelogs were getting long, so I've moved them here for
> posterity and review purposes:
>
> livepatch: Create and include UAPI headers
> v2:
> - jmoreira: split up and changelog
> v3:
> - joe: convert to SPDX license tags
>
> kbuild: Support for Symbols.list creation
> v3:
> - jmoreira: adjust for multiobject livepatch
> - joe: add klpclean to PHONY
> - joe: align KLP prefix
>
> livepatch: Add klp-convert tool
> v2:
> - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
> at the end
> - jmoreira: add support to automatic relocation conversion in
> klp-convert.c, changelog
> v3:
> - joe: convert to SPDX license tags
> - jmoreira: add rela symbol name to WARNs
> - jmoreira: ignore relocations to .TOC and symbols with index 0
> - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks
> - joe: fix symbol use-after-frees
> - joe: fix remaining valgrind leak complaints (non-error paths only)
> - joe:
