Received-SPF: pass (google.com: domain of linux-kernel+bounces-22630-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <8c9698b7cabf76e783a0a1a6368de0c11134e659.camel@suse.com>
Subject: Re: [PATCH v1 2/5] livepatch: Add klp-convert tool
From: Marcos Paulo de Souza <mpdesouza@suse.com>
To: Petr Mladek <pmladek@suse.com>, Lukas Hruska <lhruska@suse.cz>
Cc: Miroslav Benes <mbenes@suse.cz>, Josh Poimboeuf <jpoimboe@kernel.org>, 
 Joe Lawrence <joe.lawrence@redhat.com>, live-patching@vger.kernel.org,
 linux-kernel@vger.kernel.org,  linux-kbuild@vger.kernel.org, Josh Poimboeuf
 <jpoimboe@redhat.com>
Date: Wed, 10 Jan 2024 16:03:32 -0300
In-Reply-To: <ZZgEHFIbfCbSWy4N@alley>
References: <20231106162513.17556-1-lhruska@suse.cz>
	 <20231106162513.17556-3-lhruska@suse.cz> <ZZgEHFIbfCbSWy4N@alley>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.1 
Precedence: bulk
MIME-Version: 1.0

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:
> >=20
> > =C2=A0 + Symbols which can be local for the original livepatched
> > function.
> > =C2=A0=C2=A0=C2=A0 The alternative implementation in the livepatch sees=
 them
> > =C2=A0=C2=A0=C2=A0 as external symbols.
> >=20
> > =C2=A0 + Symbols in modules which are exported via EXPORT_SYMBOL*().
> > They
> > =C2=A0=C2=A0=C2=A0 must be handled special way otherwise the livepatch =
module
> > would
> > =C2=A0=C2=A0=C2=A0 depend on the livepatched one. Loading such livepatc=
h would
> > cause
> > =C2=A0=C2=A0=C2=A0 loading the other module as well.
> >=20
> > The address of these symbols can be found via kallsyms. Or they can
>=20
> Please, remove the extra space at the end of the line.
>=20
> > be relocated using livepatch specific relocation sections as
> > specified
> > in Documentation/livepatch/module-elf-format.txt.
> >=20
> > 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.
> >=20
> > Given the above, create scripts/livepatch to hold tools developed
> > for
> > livepatches and add source files for klp-convert there.
> >=20
> > 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:
> >=20
> > =C2=A0 extern char *saved_command_line \
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 KLP_RELOC_SYMBOL(vmlinux, vmlinux,
> > saved_command_line, 0);
> >=20
> > would create symbol
> >=20
> > $>readelf -r -W <compiled livepatch module>:
> > Relocation section '.rela.text' at offset 0x32e60 contains 10
> > entries:
> > =C2=A0=C2=A0=C2=A0 Offset=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 Info=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Type=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Symbol's
> > Value=C2=A0 Symbol's Name + Addend
> > [...]
> > 0000000000000068=C2=A0 0000003c00000002 R_X86_64_PC32=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0
> > 0000000000000000 .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0
> > - 4
> > [...]
> >=20
> >=20
> > 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.
> >=20
> > For example, klp-convert would replace the above symbols with:
>=20
> s/above symbols/above symbol/
>=20
> > $> readelf -r -W <livepatch_module_proceed_by_klp_convert>
> > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60
> > contains 1 entry:
> > =C2=A0=C2=A0=C2=A0 Offset=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 Info=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Type=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Symbol's
> > Value=C2=A0 Symbol's Name + Addend
> > 0000000000000068=C2=A0 0000003c00000002 R_X86_64_PC32=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0
> > 0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4
> >=20
> > 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.
> >=20
> > Update Makefiles to correctly support the compilation of the new
> > tool,
> > update MAINTAINERS file and add a .gitignore file.
> >=20
> > ---
> > =C2=A0MAINTAINERS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=
=C2=A0 1 +
> > =C2=A0include/linux/livepatch.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=
=C2=A0 19 +
> > =C2=A0scripts/Makefile=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 1 +
> > =C2=A0scripts/livepatch/.gitignore=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 1 +
> > =C2=A0scripts/livepatch/Makefile=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=
=C2=A0 5 +
> > =C2=A0scripts/livepatch/elf.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 | 817
> > ++++++++++++++++++++++++++++++++
> > =C2=A0scripts/livepatch/elf.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 |=C2=A0 73 +++
>=20
> I see a similar code in
>=20
> =C2=A0=C2=A0=C2=A0 tools/objtool/elf.c
> =C2=A0=C2=A0=C2=A0 tools/objtool/include/objtool/elf.h
>=20
> Both variants have been written by Josh. I wonder if we could share
> one implementation. Josh?
>=20
> > =C2=A0scripts/livepatch/klp-convert.c | 283 +++++++++++
> > =C2=A0scripts/livepatch/klp-convert.h |=C2=A0 42 ++
> > =C2=A0scripts/livepatch/list.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 | 391 +++++++++++++++
>=20
> 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.

>=20
> > =C2=A010 files changed, 1633 insertions(+)
> > =C2=A0create mode 100644 scripts/livepatch/.gitignore
> > =C2=A0create mode 100644 scripts/livepatch/Makefile
> > =C2=A0create mode 100644 scripts/livepatch/elf.c
> > =C2=A0create mode 100644 scripts/livepatch/elf.h
> > =C2=A0create mode 100644 scripts/livepatch/klp-convert.c
> > =C2=A0create mode 100644 scripts/livepatch/klp-convert.h
> > =C2=A0create mode 100644 scripts/livepatch/list.h
> >=20
> > --- /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 <=3D 56, "Update limit in the
> > below sscanf()");
>=20
> IMHO, there should be "< 56" instead of "<=3D 56". The sscanf is
> limited by %55.
>=20
> Also we should check KSYM_NAME_LEN. Similar to to check in
> klp_resolve_symbols()
>=20
> 	static_assert(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN !=3D 512,
> 		=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Update limit in the below sscanf()");
>=20
> > +
> > +	if (sscanf(s->name, KLP_SYM_RELA_PREFIX
> > "%55[^.].%55[^.].%511[^,],%lu",
> > +			lp_obj_name, sym_obj_name, sym_name,
> > &sym_pos) !=3D 4) {
> > +		WARN("Invalid format of symbol (%s)\n", s->name);
> > +		return false;
> > +	}
> > +
> > +	poslen =3D calc_digits(sym_pos);
> > +
> > +	length =3D strlen(KLP_SYM_PREFIX) + strlen(sym_obj_name)
> > +		 + strlen(sym_name) + sizeof(poslen) + 3;
> > +
> > +	klp_sym_name =3D 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",
> > +			=C2=A0 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 =3D klp_sym_name;
> > +	s->sec =3D NULL;
> > +	s->sym.st_name =3D -1;
> > +	s->sym.st_shndx =3D 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@redhat.com>
> > + * Copyright (C) 2017 Joao Moreira=C2=A0=C2=A0 <jmoreira@suse.de>
> > + *
> > + */
> > +
> > +#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;
> > +};
>=20
> It seems that this structure is not longer used.
>=20
> > +/*
> > + * 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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __attribute=
__((packed))
> > +#endif
> > +
> > +struct klp_module_reloc {
> > +	union {
> > +		void *sym;
> > +		uint64_t sym64;	/* Force 64-bit width */
> > +	};
> > +	uint32_t sympos;
> > +} __packed;
>=20
> And this one as well.
>=20
> 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@suse.com>

>=20
> Best Regards,
> Petr