Hi,
several minor things and nits below. Otherwise it is ok.
On Wed, 3 Feb 2016, Jessica Yu wrote:
> + * 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;
strcspn() returns size_t, so we can check only for 0
if (!len)
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;
Same here.
> +
> + 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;
> +
This is really a nitpick, but you have a comment about mandatory format of
the name here in klp_check_symbol_format().
> + 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;
You don't check if section_name part is non-empty.
[...]
> +/* .klp.sym.[objname].symbol_name,sympos */
> +static int klp_get_sym_objname(char *s, char **result)
> +{
Do we need result to be a double-pointer? If I am not mistaken just 'char
*result' could be sufficient. You check the return value, so result could
be NULL or objname as found. No?
> + 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;
According to CodingStyle there should be curly braces even for else
branch.
> + return 0;
> +}
> +
> +/* .klp.sym.objname.[symbol_name],sympos */
> +static int klp_get_symbol_name(char *s, char **result)
Same here.
> +{
> + 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;
> +}
[...]
> +static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
> {
> - const struct kernel_symbol *sym;
> -
> - /* first, check if it's an exported symbol */
> - preempt_disable();
> - sym = find_symbol(name, NULL, NULL, true, true);
> - if (sym) {
> - *addr = sym->value;
> - preempt_enable();
> - return 0;
> + int i, ret = 0;
> + Elf_Rela *relas;
> + Elf_Sym *sym;
> + char *s, *symbol_name, *sym_objname;
> + unsigned long sympos;
> +
> + relas = (Elf_Rela *) relasec->sh_addr;
> + /* For each rela in this .klp.rela. section */
> + for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> + sym = pmod->symtab + ELF_R_SYM(relas[i].r_info);
> +
> + /* Check if the symbol is formatted correctly */
> + ret = klp_check_symbol_format(pmod, sym);
> + if (ret)
> + goto out;
> + /* Format: .klp.sym.objname.symbol_name,sympos */
> + s = pmod->strtab + sym->st_name;
> + ret = klp_get_symbol_name(s, &symbol_name);
> + if (ret)
> + goto out;
> + ret = klp_get_sym_objname(s, &sym_objname);
> + if (ret)
> + goto free_symbol_name;
> + ret = klp_get_sympos(s, &sympos);
> + if (ret)
> + goto free_objname;
> +
> + ret = klp_find_object_symbol(sym_objname, symbol_name, sympos,
> + (unsigned long *) &sym->st_value);
> +free_objname:
> + kfree(sym_objname);
> +free_symbol_name:
> + kfree(symbol_name);
> + if (ret)
> + goto out;
> }
> - preempt_enable();
> -
> - /*
> - * Check if it's in another .o within the patch module. This also
> - * checks that the external symbol is unique.
> - */
> - return klp_find_object_symbol(pmod->name, name, 0, addr);
> +out:
> + return ret;
> }
I wonder if 'break;' instead of 'goto out;' would generate
different/better/more readable code. Anyway out label is not necessary and
we can achieve the same with break. It is up to you though.
> static int klp_write_object_relocations(struct module *pmod,
> struct klp_object *obj)
> {
> - int ret = 0;
> - unsigned long val;
> - struct klp_reloc *reloc;
> + int i, ret = 0;
> + Elf_Shdr *sec;
>
> if (WARN_ON(!klp_is_object_loaded(obj)))
> return -EINVAL;
>
> - if (WARN_ON(!obj->relocs))
> - return -EINVAL;
> -
> module_disable_ro(pmod);
> + /* For each klp relocation section */
> + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> + sec = pmod->klp_info->sechdrs + i;
> + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> + continue;
>
> - for (reloc = obj->relocs; reloc->name; reloc++) {
> - /* discover the address of the referenced symbol */
> - if (reloc->external) {
> - if (reloc->sympos > 0) {
> - pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> - reloc->name);
> - ret = -EINVAL;
> - goto out;
> - }
> - ret = klp_find_external_symbol(pmod, reloc->name, &val);
> - } else
> - ret = klp_find_object_symbol(obj->name,
> - reloc->name,
> - reloc->sympos,
> - &val);
> + /* Check if the klp section is formatted correctly */
> + ret = klp_check_relasec_format(pmod, sec);
> if (ret)
> goto out;
>
> - ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> - val + reloc->addend);
> - if (ret) {
> - pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> - reloc->name, val, ret);
> + /* Check if the klp section belongs to obj */
> + if (!klp_relasec_matches_object(pmod, sec, obj))
> + continue;
> +
> + /* Resolve all livepatch syms referenced in this rela section */
> + ret = klp_resolve_symbols(sec, pmod);
> + if (ret)
> goto out;
> - }
> - }
>
> + ret = apply_relocate_add(pmod->klp_info->sechdrs, pmod->strtab,
> + pmod->klp_info->symndx, i, pmod);
> + if (ret)
> + goto out;
> + }
> out:
> module_enable_ro(pmod);
> return ret;
Same thing with break instead of all gotos.
Thanks,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
