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
Subject: Re: [RFC PATCH v6 03/12] livepatch: Add klp-convert tool
Date: Mon, 6 Feb 2023 15:16:22 -0300 [thread overview]
Message-ID: <Y+FD9pCLJUuNKHo2@daedalus.suse.de> (raw)
In-Reply-To: <20220216163940.228309-4-joe.lawrence@redhat.com>
On Wed, Feb 16, 2022 at 11:39:31AM -0500, Joe Lawrence wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
All the comments bellow are suggestions. Besides them being addressed or not:
Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> +
> +/*
> + * User provided sympos annotation checks:
> + * - do two or more elements in usr_symbols have the same object and
> + * name, but different symbol position
> + * - are there any usr_symbols without a rela?
> + */
> +static bool sympos_sanity_check(struct elf *klp_elf)
> +{
> + bool sane = true;
> + struct sympos *sp, *aux;
> + struct section *sec;
> + struct rela *rela;
> +
> + list_for_each_entry(sp, &usr_symbols, list) {
> + bool found_rela = false;
> +
> + aux = list_next_entry(sp, list);
> + list_for_each_entry_from(aux, &usr_symbols, list) {
> + 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);
The WARN message could be simplified by mentioning the different position,
something like:
WARN("Conflicting KLP_SYMPOS definition: %s.%s %d vs. %d.",
sp->object_name, sp->symbol_name, sp->pos aux->pos);
> + sane = false;
> + }
> + }
> +
> + list_for_each_entry(sec, &klp_elf->sections, list) {
> + list_for_each_entry(rela, &sec->relas, list) {
> + if (!strcmp(sp->symbol_name, rela->sym->name)) {
> + found_rela = true;
> + break;
> + }
> + }
> + }
> + if (!found_rela) {
> + //sane = false;
At this point I believe that sane should be assigned to false to help the user
to know that the specified symbol isn't being used in the livepatch.
> + WARN("Couldn't find rela for annotated symbol: %s",
> + sp->symbol_name);
> + }
> +
> +
> + }
> + return sane;
> +}
<snip>
> +/*
> + * Searches for symbol in symbols list and returns its sympos if it is unique,
> + * otherwise prints a list with all considered valid sympos
> + */
> +static struct symbol_entry *find_sym_entry_by_name(char *name)
> +{
> + struct symbol_entry *found = NULL;
> + struct symbol_entry *e;
> +
> + list_for_each_entry(e, &symbols, list) {
> + if (strcmp(e->symbol_name, name) == 0) {
> +
> + /*
> + * If there exist multiple symbols with the same
> + * name then user-provided sympos is required
> + */
> + if (found) {
> + WARN("Define KLP_SYMPOS for the symbol: %s",
> + e->symbol_name);
> +
> + print_valid_module_relocs(name);
> + return NULL;
> + }
> + found = e;
> + }
> + }
> + if (found)
> + return found;
> +
> + return NULL;
Since found is either NULL or points to a symbol, the if condition can be
removed and return found directly.
> +}
> +
> +/* Checks if sympos is valid, otherwise prints valid sympos list */
> +static bool valid_sympos(struct sympos *sp)
<snip>
> +
> +/* Returns the right sympos respective to a symbol to be relocated */
> +static bool find_sympos(struct symbol *s, struct sympos *sp)
> +{
> + struct symbol_entry *entry;
> + struct converted_sym *cs;
> +
> + /* did we already convert this symbol? */
> + list_for_each_entry(cs, &converted_symbols, list) {
> + if (cs->symbol == s) {
> + *sp = cs->sympos;
> + return true;
> + }
> + }
> +
> + /* did the user specified via annotation? */
> + if (get_usr_sympos(s, sp)) {
> + if (valid_sympos(sp)) {
> + remember_sympos(s, sp);
> + return true;
> + }
> + return false;
> + }
> +
> + /* search symbol in symbols list */
> + entry = find_sym_entry_by_name(s->name);
> + if (entry) {
> + sp->symbol_name = entry->symbol_name;
> + sp->object_name = entry->object_name;
At this point I believe that it would be good to have a comment about sympos
being 0 means that the symbol wasn't specified by the user, so sympos 0 means
that the symbol is unique.
> + sp->pos = 0;
> + remember_sympos(s, sp);
> + return true;
> + }
> + return false;
> +}
<snip>
next prev parent reply other threads:[~2023-02-06 18:16 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-16 16:39 [RFC PATCH v6 00/12] livepatch: klp-convert tool Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 01/12] livepatch: Create and include UAPI headers Joe Lawrence
2022-04-14 8:50 ` Petr Mladek
2022-02-16 16:39 ` [RFC PATCH v6 02/12] kbuild: Support for symbols.klp creation Joe Lawrence
2022-04-14 9:35 ` Petr Mladek
2022-04-14 17:59 ` Nicolas Schier
2022-04-18 18:12 ` Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 03/12] livepatch: Add klp-convert tool Joe Lawrence
2022-02-16 16:46 ` Joe Lawrence
2022-02-16 16:56 ` Joe Lawrence
2022-04-14 15:03 ` elf API: was: " Petr Mladek
2022-04-18 18:01 ` Joe Lawrence
2023-02-06 18:16 ` Marcos Paulo de Souza [this message]
2022-02-16 16:39 ` [RFC PATCH v6 04/12] livepatch: Add klp-convert annotation helpers Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 05/12] modpost: Integrate klp-convert Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 06/12] livepatch: Add sample livepatch module Joe Lawrence
2023-02-07 12:52 ` Marcos Paulo de Souza
2022-02-16 16:39 ` [RFC PATCH v6 07/12] documentation: Update on livepatch elf format Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 08/12] livepatch/selftests: add klp-convert Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 09/12] livepatch/selftests: test multiple sections Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 10/12] livepatch/selftests: add __asm__ symbol renaming examples Joe Lawrence
2022-02-16 17:03 ` Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 11/12] livepatch/selftests: add data relocations test Joe Lawrence
2022-02-16 17:12 ` Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 12/12] livepatch/selftests: add static keys test Joe Lawrence
2022-02-16 17:17 ` [RFC PATCH v6 00/12] livepatch: klp-convert tool Joe Lawrence
2023-02-07 12:57 ` Marcos Paulo de Souza
2023-02-07 15:54 ` 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=Y+FD9pCLJUuNKHo2@daedalus.suse.de \
--to=mpdesouza@suse.de \
--cc=joe.lawrence@redhat.com \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
/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).