Hi Jessica,
Nice patch series. Minor issues below, but they're really
just nits which can be patched afterwards if you're getting sick of
rebasing :)
> +#ifdef CONFIG_LIVEPATCH
> +/*
> + * copy_module_elf - preserve Elf information about a module
> + *
> + * Copy relevant Elf information from the load_info struct.
> + * Note: not all fields from the original load_info are
> + * copied into mod->info.
This makes me nervous, to have a struct which is half-initialized.
Better would be to have a separate type for this, eg:
struct livepatch_modinfo {
Elf_Ehdr hdr;
Elf_Shdr *sechdrs;
char *secstrings;
/* FIXME: Which of these do you need? */
struct {
unsigned int sym, str, mod, vers, info, pcpu;
} index;
};
This also avoids an extra allocation as hdr is no longer a ptr.
> + /*
> + * Update symtab's sh_addr to point to a valid
> + * symbol table, as the temporary symtab in module
> + * init memory will be freed
> + */
> + mod->info->sechdrs[mod->info->index.sym].sh_addr = (unsigned long)mod->core_symtab;
This comment is a bit misleading: it's actually pointing into the
temporary module copy, which will be discarded. The init section is
slightly different...
> +#ifdef CONFIG_LIVEPATCH
> +static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> + mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> + return 0;
> +}
> +#else
> +static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> + if (get_modinfo(info, "livepatch"))
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif
The term "check" implies no side-effects; I'd usually call this
int find_livepatch_modinfo(), and make sure you use the error code in
the caller:
> + err = check_livepatch_modinfo(mod, info);
> + if (err)
> + return -ENOEXEC;
> +
Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
