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 :)
quoted hunk ↗ jump to hunk
+#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.
quoted hunk ↗ jump to hunk
+ /*
+ * 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...
quoted hunk ↗ jump to hunk
+#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:
quoted hunk ↗ jump to hunk
+ err = check_livepatch_modinfo(mod, info);
+ if (err)
+ return -ENOEXEC;
+
Thanks,
Rusty.