From: Joe Lawrence <joe.lawrence@redhat.com>
To: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
linux-kbuild@vger.kernel.org
Cc: Jessica Yu <jeyu@kernel.org>, Jiri Kosina <jikos@kernel.org>,
Joao Moreira <jmoreira@suse.de>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Konstantin Khlebnikov <khlebnikov@yandex-team.ru>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
Michael Matz <matz@suse.de>, Miroslav Benes <mbenes@suse.cz>,
Nicolai Stange <nstange@suse.de>, Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH v3 0/9] klp-convert livepatch build tooling
Date: Wed, 17 Apr 2019 16:13:16 -0400 [thread overview]
Message-ID: <20190417201316.GA690@redhat.com> (raw)
In-Reply-To: <20190410155058.9437-1-joe.lawrence@redhat.com>
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: 0000000000000000 0 FILE LOCAL DEFAULT ABS test_klp_convert_mod_a.c
32: 0000000000000010 8 FUNC LOCAL DEFAULT 3 get_homonym_string
33: 0000000000000000 17 OBJECT LOCAL DEFAULT 5 homonym_string
37: 0000000000000000 0 FILE LOCAL DEFAULT ABS test_klp_convert_mod_b.c
38: 0000000000000020 8 FUNC LOCAL DEFAULT 3 get_homonym_string
39: 0000000000000000 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 <object,
name, position> value across object files.
Result: a small tweak to sympos_sanity_check() to relax its symbol
uniqueness verification: allow for duplicate <object, name, position>
instances. Now it will only complain when a supplied symbol references
the same <object, name> but a different <position>.
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, &usr_symbols, list) {
aux = list_next_entry(sp, list);
list_for_each_entry_from(aux, &usr_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
-------------
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 '\<state_show\>'
30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\<state_show\>'
20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF 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 <sympos> value:
% objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
0000000 0000 0000 0000 0000 0001 0000 0000 0000
0000010 0000 0000 0002 0000
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 '\<state_show\>'
53: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF 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 Type Value Addend Name
0x0000000000000003 X86_64_32S 000000000000000000 +0 state_show
...
0x0000000000000034 X86_64_32S 000000000000000000 +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 <object, name>
symbols with unique <position> 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
In the meantime, the unique symbol <position> case is already detected
and with a little tweaking we could support multiple common symbol
<position> values.
-- Joe
next prev parent reply other threads:[~2019-04-17 20:13 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-10 15:50 [PATCH v3 0/9] klp-convert livepatch build tooling Joe Lawrence
2019-04-10 15:50 ` [PATCH v3 1/9] livepatch: Create and include UAPI headers Joe Lawrence
2019-04-11 0:32 ` Masahiro Yamada
2019-04-11 14:30 ` Joe Lawrence
2019-04-11 15:48 ` Josh Poimboeuf
2019-04-11 18:41 ` Miroslav Benes
2019-04-10 15:50 ` [PATCH v3 2/9] kbuild: Support for Symbols.list creation Joe Lawrence
2019-04-11 9:18 ` Artem Savkov
2019-04-11 15:18 ` Joe Lawrence
2019-04-11 19:04 ` Miroslav Benes
2019-04-16 14:13 ` Joe Lawrence
2019-04-16 19:02 ` Miroslav Benes
2019-04-10 15:50 ` [PATCH v3 3/9] livepatch: Add klp-convert tool Joe Lawrence
2019-04-10 18:22 ` Joe Lawrence
2019-04-12 9:02 ` Miroslav Benes
2019-04-23 20:35 ` Joe Lawrence
2019-04-24 17:47 ` Miroslav Benes
2019-04-24 21:00 ` Joe Lawrence
2019-04-10 15:50 ` [PATCH v3 4/9] livepatch: Add klp-convert annotation helpers Joe Lawrence
2019-04-10 18:18 ` Joe Lawrence
2019-04-12 9:14 ` Miroslav Benes
2019-04-10 15:50 ` [PATCH v3 5/9] modpost: Integrate klp-convert Joe Lawrence
2019-04-11 15:54 ` Joe Lawrence
2019-04-10 15:50 ` [PATCH v3 6/9] modpost: Add modinfo flag to livepatch modules Joe Lawrence
2019-04-12 10:43 ` Miroslav Benes
2019-04-10 15:50 ` [PATCH v3 7/9] livepatch: Add sample livepatch module Joe Lawrence
2019-04-10 15:50 ` [PATCH v3 8/9] documentation: Update on livepatch elf format Joe Lawrence
2019-04-10 15:50 ` [PATCH v3 9/9] livepatch/selftests: add klp-convert Joe Lawrence
2019-04-12 21:26 ` [PATCH v3 0/9] klp-convert livepatch build tooling Joe Lawrence
2019-04-16 11:37 ` Miroslav Benes
2019-05-03 14:29 ` Joe Lawrence
2019-05-06 14:39 ` Miroslav Benes
2019-04-16 5:24 ` Balbir Singh
2019-04-16 8:29 ` Miroslav Benes
2019-04-16 13:37 ` Joe Lawrence
2019-04-16 13:55 ` Joe Lawrence
2019-04-16 19:09 ` Miroslav Benes
2019-04-17 20:13 ` Joe Lawrence [this message]
2019-04-24 18:19 ` Miroslav Benes
2019-04-24 19:13 ` Joao Moreira
2019-04-24 19:23 ` Josh Poimboeuf
2019-04-24 19:31 ` Joe Lawrence
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190417201316.GA690@redhat.com \
--to=joe.lawrence@redhat.com \
--cc=jeyu@kernel.org \
--cc=jikos@kernel.org \
--cc=jmoreira@suse.de \
--cc=jpoimboe@redhat.com \
--cc=khlebnikov@yandex-team.ru \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=matz@suse.de \
--cc=mbenes@suse.cz \
--cc=nstange@suse.de \
--cc=pmladek@suse.com \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).