From: Marcos Paulo de Souza <mpdesouza@suse.de>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kbuild@vger.kernel.org,
Josh Poimboeuf <jpoimboe@kernel.org>,
Miroslav Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>,
Marcos Paulo de Souza <mpdesouza@suse.com>
Subject: Re: [PATCH v7 02/10] livepatch: Add klp-convert tool
Date: Tue, 14 Mar 2023 15:26:21 -0300 [thread overview]
Message-ID: <20230314182621.tsh55pjeo6onb6ix@daedalus> (raw)
In-Reply-To: <20230306140824.3858543-3-joe.lawrence@redhat.com>
On Mon, Mar 06, 2023 at 09:08:16AM -0500, Joe Lawrence 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 symbols.klp, 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.klp file, an input
> livepatch module to be converted and the output name for the converted
> livepatch. When it starts running, klp-convert parses symbols.klp 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.klp, 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
LGTM:
Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>
I only have two remarks:
>
> 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>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
...
> +#if 0
> + /*
> + * klp-relocations forbidden in sections that otherwise would
> + * match in allowed_prefixes[]
> + */
> + static const char * const not_allowed[] = {
> + ".rela.data.rel.ro",
> + ".rela.data.rel.ro.local",
> + ".rela.data..ro_after_init",
> + NULL
> + };
> +#endif
> +
> + /* klp-relocations allowed in sections only for vmlinux */
> + static const char * const allowed_vmlinux[] = {
> + ".rela__jump_table",
> + NULL
> + };
> +
> + /* klp-relocations allowed in sections with prefixes */
> + static const char * const allowed_prefixes[] = {
> + ".rela.data",
> + ".rela.rodata", // supported ???
> + ".rela.sdata",
> + ".rela.text",
> + ".rela.toc",
> + NULL
> + };
> +
> + const char * const *name;
> +
> +#if 0
> + for (name = not_allowed; *name; name++)
> + if (strcmp(sec->name, *name) == 0)
> + return false;
> +#endif
> +
Have you needed to enable the not_allowed checks when creating your livepatches?
Otherwise I believe that this can be removed and added again in the future is
needed.
> +int main(int argc, const char **argv)
> +{
> + const char *klp_in_module, *klp_out_module, *symbols_list;
...
> +
> +/* Functions kept commented since they might be useful for future debugging */
> +
> +/* Dumps sympos list (useful for debugging purposes)
> + * static void dump_sympos(void)
> + * {
> + * struct sympos *sp;
> + *
> + * fprintf(stderr, "BEGIN OF SYMPOS DUMP\n");
> + * list_for_each_entry(sp, &usr_symbols, list) {
> + * fprintf(stderr, "%s %s %d\n", sp->symbol_name, sp->object_name,
> + * sp->pos);
> + * }
> + * fprintf(stderr, "END OF SYMPOS DUMP\n");
> + * }
> + *
> + *
> + * / Dump symbols list for debugging purposes /
> + * static void dump_symbols(void)
> + * {
> + * struct symbol_entry *entry;
> + *
> + * fprintf(stderr, "BEGIN OF SYMBOLS DUMP\n");
> + * list_for_each_entry(entry, &symbols, list)
> + * printf("%s %s\n", entry->object_name, entry->symbol_name);
> + * fprintf(stderr, "END OF SYMBOLS DUMP\n");
> + * }
Same here. Have you used these functions recently when debugging klp-convert?
Othewise it can be removed as well.
next prev parent reply other threads:[~2023-03-14 18:26 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-06 14:08 [PATCH v7 00/10] livepatch: klp-convert tool Joe Lawrence
2023-03-06 14:08 ` [PATCH v7 01/10] livepatch: Create and include UAPI headers Joe Lawrence
2023-03-07 11:41 ` Marcos Paulo de Souza
2023-03-06 14:08 ` [PATCH v7 02/10] livepatch: Add klp-convert tool Joe Lawrence
2023-03-14 18:26 ` Marcos Paulo de Souza [this message]
2023-03-17 20:06 ` Joe Lawrence
2023-03-20 19:53 ` Marcos Paulo de Souza
2023-03-06 14:08 ` [PATCH v7 03/10] kbuild/modpost: create symbols.klp and integrate klp-convert Joe Lawrence
2023-03-14 18:48 ` Marcos Paulo de Souza
2023-03-06 14:08 ` [PATCH v7 04/10] livepatch: Add sample livepatch module Joe Lawrence
2023-03-14 18:17 ` Marcos Paulo de Souza
2023-03-06 14:08 ` [PATCH v7 05/10] documentation: Update on livepatch elf format Joe Lawrence
2023-03-07 11:48 ` Marcos Paulo de Souza
2023-03-06 14:08 ` [PATCH v7 06/10] livepatch/selftests: add klp-convert Joe Lawrence
2023-03-14 20:22 ` Marcos Paulo de Souza
2023-03-06 14:08 ` [PATCH v7 07/10] livepatch/selftests: test multiple sections Joe Lawrence
2023-03-06 14:08 ` [PATCH v7 08/10] livepatch/selftests: add __asm__ symbol renaming examples Joe Lawrence
2023-03-06 14:08 ` [PATCH v7 09/10] livepatch/selftests: add data relocations test Joe Lawrence
2023-03-06 14:08 ` [PATCH v7 10/10] livepatch/selftests: add static keys test Joe Lawrence
2023-03-14 20:23 ` [PATCH v7 00/10] livepatch: klp-convert tool Marcos Paulo de Souza
2023-03-17 20:29 ` Joe Lawrence
2023-03-17 23:20 ` Josh Poimboeuf
2023-03-20 19:23 ` Joe Lawrence
2023-04-11 10:06 ` Nicolai Stange
2023-05-02 23:38 ` Marcos Paulo de Souza
2023-05-03 19:54 ` Joe Lawrence
2023-05-09 20:34 ` Marcos Paulo de Souza
2023-03-20 20:15 ` Marcos Paulo de Souza
2023-04-19 20:27 ` Marcos Paulo de Souza
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=20230314182621.tsh55pjeo6onb6ix@daedalus \
--to=mpdesouza@suse.de \
--cc=joe.lawrence@redhat.com \
--cc=jpoimboe@kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=mpdesouza@suse.com \
--cc=pmladek@suse.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).