On Thu 2024-05-16 15:30:05, 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
> be relocated using livepatch specific relocation sections as specified
> in Documentation/livepatch/module-elf-format.txt.
>
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.c
> +/* 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 && 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;
There should be "poslen" instead of "sizeof(poslen)".
> +
> + 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;
> +}
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.h
> @@ -0,0 +1,23 @@
> +/* 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__)
Nit: I would remove "\n" here and add it to all callers. Half of the
callers already have it ;-)
> +
> +/*
> + * 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."
Otherwise, it looks good.
Best Regards,
Petr
