Re: [PATCH v4 51/63] objtool/klp: Introduce klp diff subcommand for diffing object files

[Petr Mladek <pmladek@suse.com>]

On Wed 2025-09-17 09:03:59, Josh Poimboeuf wrote:
> Add a new klp diff subcommand which performs a binary diff between two
> object files and extracts changed functions into a new object which can
> then be linked into a livepatch module.
> 
> This builds on concepts from the longstanding out-of-tree kpatch [1]
> project which began in 2012 and has been used for many years to generate
> livepatch modules for production kernels.  However, this is a complete
> rewrite which incorporates hard-earned lessons from 12+ years of
> maintaining kpatch.
> 
> --- /dev/null
> +++ b/tools/objtool/klp-diff.c
> +static int read_exports(void)
> +{
> +	const char *symvers = "Module.symvers";
> +	char line[1024], *path = NULL;
> +	unsigned int line_num = 1;
> +	FILE *file;
> +
> +	file = fopen(symvers, "r");
> +	if (!file) {
> +		path = top_level_dir(symvers);
> +		if (!path) {
> +			ERROR("can't open '%s', \"objtool diff\" should be run from the kernel tree", symvers);
> +			return -1;
> +		}
> +
> +		file = fopen(path, "r");
> +		if (!file) {
> +			ERROR_GLIBC("fopen");
> +			return -1;
> +		}
> +	}
> +
> +	while (fgets(line, 1024, file)) {

Nit: It might be more safe to replace 1024 with sizeof(line).
     It might be fixed later in a separate patch.

> +		char *sym, *mod, *type;
> +		struct export *export;
> +
> +		sym = strchr(line, '\t');
> +		if (!sym) {
> +			ERROR("malformed Module.symvers (sym) at line %d", line_num);
> +			return -1;
> +		}
> +

[...]

> +/*
> + * Klp relocations aren't allowed for __jump_table and .static_call_sites if
> + * the referenced symbol lives in a kernel module, because such klp relocs may
> + * be applied after static branch/call init, resulting in code corruption.
> + *
> + * Validate a special section entry to avoid that.  Note that an inert
> + * tracepoint is harmless enough, in that case just skip the entry and print a
> + * warning.  Otherwise, return an error.
> + *
> + * This is only a temporary limitation which will be fixed when livepatch adds
> + * support for submodules: fully self-contained modules which are embedded in
> + * the top-level livepatch module's data and which can be loaded on demand when
> + * their corresponding to-be-patched module gets loaded.  Then klp relocs can
> + * be retired.

I wonder how temporary this is ;-) The comment looks optimistic. I am
just curious. Do you have any plans to implement the support for
the submodules... ?

> + * Return:
> + *   -1: error: validation failed
> + *    1: warning: tracepoint skipped
> + *    0: success
> + */
> +static int validate_special_section_klp_reloc(struct elfs *e, struct symbol *sym)
> +{
> +	bool static_branch = !strcmp(sym->sec->name, "__jump_table");
> +	bool static_call   = !strcmp(sym->sec->name, ".static_call_sites");
> +	struct symbol *code_sym = NULL;
> +	unsigned long code_offset = 0;
> +	struct reloc *reloc;
> +	int ret = 0;
> +
> +	if (!static_branch && !static_call)
> +		return 0;
> +

Best Regards,
Petr

PS: To make some expectations. I am not doing a deep review.
    I am just looking at the patchset to see how far and mature
    it is. And I just comment what catches my eye.

    My first impression is that it is already in a pretty good state.
    And I do not see any big problem there. Well, some documentation
    would be fine ;-)

    What are your plans, please?