Re: [PATCH v1 2/5] livepatch: Add klp-convert tool

From: Marcos Paulo de Souza
Date: Wed Jan 10 2024 - 14:03:50 EST

Next message: Duje Mihanović: "[PATCH v8 6/9] dt-bindings: marvell: Document PXA1908 SoC"
Previous message: attreyee-muk: "[PATCH] Documentation/core-api: fix spelling mistake in workqueue"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2024-01-05 at 14:29 +0100, Petr Mladek wrote:
> On Mon 2023-11-06 17:25:10, Lukas Hruska wrote:
> > Livepatches need to access external symbols which can't be handled
> > by the normal relocation mechanism. It is needed for two types
> > of symbols:
> >
> > + Symbols which can be local for the original livepatched
> > function.
> >     The alternative implementation in the livepatch sees them
> >     as external symbols.
> >
> > + Symbols in modules which are exported via EXPORT_SYMBOL*().
> > They
> >     must be handled special way otherwise the livepatch module
> > would
> >     depend on the livepatched one. Loading such livepatch would
> > cause
> >     loading the other module as well.
> >
> > The address of these symbols can be found via kallsyms. Or they can
>
> Please, remove the extra space at the end of the line.
>
> > be relocated using livepatch specific relocation sections as
> > 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 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.
> >
> > Allow to annotate such external symbols in the livepatch by a macro
> > KLP_RELOC_SYMBOL(). It will create symbol with all needed
> > metadata. For example:
> >
> > extern char *saved_command_line \
> >                  KLP_RELOC_SYMBOL(vmlinux, vmlinux,
> > saved_command_line, 0);
> >
> > would create symbol
> >
> > $>readelf -r -W <compiled livepatch module>:
> > Relocation section '.rela.text' at offset 0x32e60 contains 10
> > entries:
> >     Offset             Info             Type               Symbol's
> > Value Symbol's Name + Addend
> > [...]
> > 0000000000000068 0000003c00000002 R_X86_64_PC32
> > 0000000000000000 .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0
> > - 4
> > [...]
> >
> >
> > Also add scripts/livepatch/klp-convert. The tool transforms symbols
> > created by KLP_RELOC_SYMBOL() to object specific rela sections
> > and rela entries which would later be proceed when the livepatch
> > or the livepatched object is loaded.
> >
> > For example, klp-convert would replace the above symbols with:
>
> s/above symbols/above symbol/
>
> > $> readelf -r -W <livepatch_module_proceed_by_klp_convert>
> > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60
> > contains 1 entry:
> >     Offset             Info             Type               Symbol's
> > Value Symbol's Name + Addend
> > 0000000000000068 0000003c00000002 R_X86_64_PC32
> > 0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4
> >
> > 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.
> >
> > ---
> > MAINTAINERS                     |   1 +
> > include/linux/livepatch.h       | 19 +
> > scripts/Makefile                |   1 +
> > scripts/livepatch/.gitignore    |   1 +
> > scripts/livepatch/Makefile      |   5 +
> > scripts/livepatch/elf.c         | 817
> > ++++++++++++++++++++++++++++++++
> > scripts/livepatch/elf.h         | 73 +++
>
> I see a similar code in
>
>     tools/objtool/elf.c
>     tools/objtool/include/objtool/elf.h
>
> Both variants have been written by Josh. I wonder if we could share
> one implementation. Josh?
>
> > scripts/livepatch/klp-convert.c | 283 +++++++++++
> > scripts/livepatch/klp-convert.h | 42 ++
> > scripts/livepatch/list.h        | 391 +++++++++++++++
>
> And probably also the list.h

I understand that code that live on tools/ are usually self contained,
so I'm not sure how can this code be shared. Is it advisable to add
list.h, elf.h to tools/include/tools? I'm not sure about the elf.c
tough.

>
> > 10 files changed, 1633 insertions(+)
> > create mode 100644 scripts/livepatch/.gitignore
> > create mode 100644 scripts/livepatch/Makefile
> > create mode 100644 scripts/livepatch/elf.c
> > create mode 100644 scripts/livepatch/elf.h
> > create mode 100644 scripts/livepatch/klp-convert.c
> > create mode 100644 scripts/livepatch/klp-convert.h
> > create mode 100644 scripts/livepatch/list.h
> >
> > --- /dev/null
> > +++ b/scripts/livepatch/klp-convert.c
> > @@ -0,0 +1,283 @@
> [...]
> > +/* Converts rela symbol names */
> > +static bool convert_symbol(struct symbol *s)
> > +{
> > + char lp_obj_name[MODULE_NAME_LEN];
> > + char sym_obj_name[MODULE_NAME_LEN];
> > + char sym_name[KSYM_NAME_LEN];
> > + char *klp_sym_name;
> > + unsigned long sym_pos;
> > + int poslen;
> > + unsigned int length;
> > +
> > + static_assert(MODULE_NAME_LEN <= 56, "Update limit in the
> > below sscanf()");
>
> IMHO, there should be "< 56" instead of "<= 56". The sscanf is
> limited by %55.
>
> Also we should check KSYM_NAME_LEN. Similar to to check in
> klp_resolve_symbols()
>
> static_assert(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 512,
>       "Update limit in the below sscanf()");
>
> > +
> > + if (sscanf(s->name, KLP_SYM_RELA_PREFIX
> > "%55[^.].%55[^.].%511[^,],%lu",
> > + lp_obj_name, sym_obj_name, sym_name,
> > &sym_pos) != 4) {
> > + WARN("Invalid format of symbol (%s)\n", s->name);
> > + return false;
> > + }
> > +
> > + poslen = calc_digits(sym_pos);
> > +
> > + length = strlen(KLP_SYM_PREFIX) + strlen(sym_obj_name)
> > + + strlen(sym_name) + sizeof(poslen) + 3;
> > +
> > + klp_sym_name = calloc(1, length);
> > + if (!klp_sym_name) {
> > + WARN("Memory allocation failed (%s%s.%s,%lu)\n",
> > KLP_SYM_PREFIX,
> > + sym_obj_name, sym_name, sym_pos);
> > + return false;
> > + }
> > +
> > + if (safe_snprintf(klp_sym_name, length, KLP_SYM_PREFIX
> > "%s.%s,%lu",
> > + sym_obj_name, sym_name, sym_pos)) {
> > +
> > + WARN("Length error (%s%s.%s,%lu)", KLP_SYM_PREFIX,
> > + sym_obj_name, sym_name, sym_pos);
> > + free(klp_sym_name);
> > + return false;
> > + }
> > +
> > + s->name = klp_sym_name;
> > + s->sec = NULL;
> > + s->sym.st_name = -1;
> > + s->sym.st_shndx = SHN_LIVEPATCH;
> > +
> > + return true;
> > +}
> > +
> > diff --git a/scripts/livepatch/klp-convert.h
> > b/scripts/livepatch/klp-convert.h
> > new file mode 100644
> > index 000000000000..34842c50c711
> > --- /dev/null
> > +++ b/scripts/livepatch/klp-convert.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > + * Copyright (C) 2017 Joao Moreira   <jmoreira@xxxxxxx>
> > + *
> > + */
> > +
> > +#define SHN_LIVEPATCH 0xff20
> > +#define SHF_RELA_LIVEPATCH 0x00100000
> > +#define MODULE_NAME_LEN (64 - sizeof(GElf_Addr))
> > +#define WARN(format, ...) \
> > + fprintf(stderr, "klp-convert: " format "\n",
> > ##__VA_ARGS__)
> > +
> > +struct sympos {
> > + char *symbol_name;
> > + char *object_name;
> > + char *loading_obj_name;
> > + int pos;
> > +};
>
> It seems that this structure is not longer used.
>
> > +/*
> > + * klp-convert uses macros and structures defined in the linux
> > sources
> > + * package (see include/uapi/linux/livepatch.h). To prevent the
> > + * dependency when building locally, they are defined below. Also
> > notice
> > + * that these should match the definitions from the targeted
> > kernel.
> > + */
> > +
> > +#define KLP_RELA_PREFIX ".klp.rela."
> > +#define KLP_SYM_RELA_PREFIX ".klp.sym.rela."
> > +#define KLP_SYM_PREFIX ".klp.sym."
> > +
> > +#ifndef __packed
> > +#define __packed        __attribute__((packed))
> > +#endif
> > +
> > +struct klp_module_reloc {
> > + union {
> > + void *sym;
> > + uint64_t sym64; /* Force 64-bit width */
> > + };
> > + uint32_t sympos;
> > +} __packed;
>
> And this one as well.
>
> I do not see any other obvious problem. And it seems to work
> at least for the later added sample module.

I agree with Petr in all his other comments. With the comments
addressed, you can add:

Reviewed-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>

>
> Best Regards,
> Petr

Next message: Duje Mihanović: "[PATCH v8 6/9] dt-bindings: marvell: Document PXA1908 SoC"
Previous message: attreyee-muk: "[PATCH] Documentation/core-api: fix spelling mistake in workqueue"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]