Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations

[Miroslav Benes]

On Tue, 9 Feb 2016, Petr Mladek wrote:

> On Wed 2016-02-03 20:11:09, Jessica Yu wrote:
> > Reuse module loader code to write relocations, thereby eliminating the need
> > for architecture specific relocation code in livepatch. Specifically, reuse
> > the apply_relocate_add() function in the module loader to write relocations
> > instead of duplicating functionality in livepatch's arch-dependent
> > klp_write_module_reloc() function.
> > 
> > In order to accomplish this, livepatch modules manage their own relocation
> > sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> > livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> > index). To apply livepatch relocation sections, livepatch symbols
> > referenced by relocs are resolved and then apply_relocate_add() is called
> > to apply those relocations.
> > 
> > In addition, remove x86 livepatch relocation code and the s390
> > klp_write_module_reloc() function stub. They are no longer needed since
> > relocation work has been offloaded to module loader.
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 7aa975d..c1fe57c 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -28,6 +28,9 @@
> >  #include <linux/list.h>
> >  #include <linux/kallsyms.h>
> >  #include <linux/livepatch.h>
> > +#include <linux/elf.h>
> > +#include <linux/string.h>
> > +#include <linux/moduleloader.h>
> >  #include <asm/cacheflush.h>
> >  
> >  /**
> > @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
> >  	return !obj->name || obj->mod;
> >  }
> >  
> > +/*
> > + * Check if a livepatch symbol is formatted properly.
> > + *
> > + * See Documentation/livepatch/module-elf-format.txt for a
> > + * detailed outline of requirements.
> > + */
> > +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> > +{
> > +	size_t len;
> > +	char *s, *objname, *symname;
> > +
> > +	if (sym->st_shndx != SHN_LIVEPATCH)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Livepatch symbol names must follow this format:
> > +	 * .klp.sym.objname.symbol_name,sympos
> > +	 */
> > +	s = pmod->strtab + sym->st_name;
> > +	/* [.klp.sym.]objname.symbol_name,sympos */
> > +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> > +		return -EINVAL;
> > +
> > +	/* .klp.sym.[objname].symbol_name,sympos */
> > +	objname = s + KLP_SYM_PREFIX_LEN;
> > +	len = strcspn(objname, ".");
> > +	if (!(len > 0))
> > +		return -EINVAL;
> > +
> > +	/* .klp.sym.objname.symbol_name,[sympos] */
> > +	if (!strchr(s, ','))
> > +		return -EINVAL;
> > +
> > +	/* .klp.sym.objname.[symbol_name],sympos */
> > +	symname = objname + len + 1;
> > +	len = strcspn(symname, ",");
> > +	if (!(len > 0))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check if a livepatch relocation section is formatted properly.
> > + *
> > + * See Documentation/livepatch/module-elf-format.txt for a
> > + * detailed outline of requirements.
> > + */
> > +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> > +{
> > +	char *secname;
> > +	size_t len;
> > +
> > +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> > +	/* [.klp.rela.]objname.section_name */
> > +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> > +				KLP_RELASEC_PREFIX_LEN))
> > +		return -EINVAL;
> > +
> > +	/* .klp.rela.[objname].section_name */
> > +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> > +	if (!(len > 0))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check if obj->name matches the objname encoded in the rela
> > + * section name (.klp.rela.[objname].section_name)
> > + *
> > + * Must pass klp_check_relasec_format() before calling this.
> > + */
> > +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
> > +				       struct klp_object *obj)
> > +{
> > +	size_t len;
> > +	const char *obj_objname, *sec_objname, *secname;
> > +
> > +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> > +	/* .klp.rela.[objname].section_name */
> > +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> > +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > +
> > +	/* Get length of the objname encoded in the section name */
> > +	len = strcspn(sec_objname, ".");
> > +
> > +	if (strlen(obj_objname) != len)
> > +		return false;
> > +
> > +	return strncmp(sec_objname, obj_objname, len) ? false : true;
> > +}
> > +
> > +/*
> > + * klp_get_* helper functions
> > + *
> > + * klp_get_* functions extract different components of the name
> > + * of a livepatch symbol. The full symbol name from the strtab
> > + * is passed in as parameter @s, and @result is filled in with
> > + * the extracted component.
> > + *
> > + * These functions assume a correctly formatted symbol and the
> > + * klp_check_symbol_format() test *must* pass before calling any
> > + * of these functions.
> > + */
> > +
> > +/* .klp.sym.[objname].symbol_name,sympos */
> > +static int klp_get_sym_objname(char *s, char **result)
> > +{
> > +	size_t len;
> > +	char *objname, *objname_start;
> > +
> > +	/* .klp.sym.[objname].symbol_name,sympos */
> > +	objname_start = s + KLP_SYM_PREFIX_LEN;
> > +	len = strcspn(objname_start, ".");
> > +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> > +	if (objname == NULL)
> > +		return -ENOMEM;
> > +
> > +	/* klp_find_object_symbol() treats NULL as vmlinux */
> > +	if (!strcmp(objname, "vmlinux")) {
> > +		*result = NULL;
> > +		kfree(objname);
> > +	} else
> > +		*result = objname;
> > +
> > +	return 0;
> > +}
> > +
> > +/* .klp.sym.objname.[symbol_name],sympos */
> > +static int klp_get_symbol_name(char *s, char **result)
> > +{
> > +	size_t len;
> > +	char *objname, *symname;
> > +
> > +	/* .klp.sym.[objname].symbol_name,sympos */
> > +	objname = s + KLP_SYM_PREFIX_LEN;
> > +	len = strcspn(objname, ".");
> > +
> > +	/* .klp.sym.objname.[symbol_name],sympos */
> > +	symname = objname + len + 1;
> > +	len = strcspn(symname, ",");
> > +
> > +	*result = kstrndup(symname, len, GFP_KERNEL);
> > +	if (*result == NULL)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +
> > +/* .klp.sym.objname.symbol_name,[sympos] */
> > +static int klp_get_sympos(char *s, unsigned long *result)
> > +{
> > +	char *sympos;
> > +
> > +	/* .klp.sym.symbol_name,[sympos] */
> > +	sympos = strchr(s, ',') + 1;
> > +	return kstrtol(sympos, 10, result);
> > +}
> 
> The usage of the helper function is nicely strightforward.
> Also I like a lot all the comments with the [] brackets
> that highlight what each check or search is for.
> 
> But I think that there is a lot of duplicated code in
> the check_ and in the get_ functions. Also there is
> a lot of strdup/free games.
> 
> The length of the elemets is limited by definition.
> The functions are called under klp_mutex.
> 
> Therefore it might be easier to maintain a two parse
> function that would fill static buffers and return
> error in case of wrong value.
> 
> I mean something like:
> 
> static char mod_name[MODULE_NAME_LEN + 1];
> static char symbol_name[KSYM_SYMBOL_LEN + 1];
> 
> static int
> klp_parse_symbol_format(struct module *pmod, Elf_Sym *sym,
> 			char *mod_name, char *symbol_name,
> 			int *sympos)
> {
> 	char *substr;
> 
> 
> 	/*
> 	 * Livepatch symbol names must follow this format:
> 	 * .klp.sym.objname.symbol_name,sympos
> 	 */
> 	s = pmod->strtab + sym->st_name;
> 	/* [.klp.sym.]objname.symbol_name,sympos */
> 	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> 		return -EINVAL;
> 
> 	/* .klp.sym.[objname].symbol_name,sympos */
> 	substr = s + KLP_SYM_PREFIX_LEN;
> 	len = strcspn(substr, ".");
> 	if (!len || len > MODULE_NAME_LEN)
> 		return -EINVAL;
> 	strncpy(mod_name, substr, len);
> 	mod_name[len] = '\0';
> 
> 	/* .klp.sym.objname.[symbol_name],sympos */
> 	substr = substr + len + 1;
> 	len = strcspn(substr, ",");
> 	if (!len || len > KSYM_SYMBOL_LEN)
> 		return -EINVAL;
> 	strncpy(symbol_name, substr, len);
> 	mod_name[len] = '\0';
> 
> 	/* .klp.sym.objname.symbol_name,[sympos] */
> 	substr = substr + len + 1;
> 	len = strlen(substr);
> 	if (!len)
> 		return -EINVAL;
> 
> 	return kstrtol(substr, 10, sympos);
> }
> 
> How does that sound, please?

It sounds good to me. And it would be even better with Josh's idea of 
sscanf included.

Miroslav