lkml.org 
[lkml]   [2023]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 02/10] livepatch: Add klp-convert tool
    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.

    \
     
     \ /
      Last update: 2023-03-27 01:00    [W:9.296 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site