On Wed, Feb 16, 2022 at 11:39:31AM -0500, Joe Lawrence wrote:
> From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
All the comments bellow are suggestions. Besides them being addressed or not:
Reviewed-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> +
> +/*
> + * 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>
