On Tuesday 31 October 2017 07:49 PM, Naveen N . Rao wrote:
Hi Kamalesh,
Sorry for the late review. Overall, the patch looks good to me. So:
Acked-by: Naveen N. Rao <naveen.n....@linux.vnet.ibm.com>
However, I have a few minor comments which can be addressed in a
subsequent patch.
Thanks for the review.
[...]
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f896..005aaea 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
[...]
@@ -239,10 +257,19 @@ static void relaswap(void *_x, void *_y, int size)
/* Get size of potential trampolines required. */
static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
- const Elf64_Shdr *sechdrs)
+ const Elf64_Shdr *sechdrs,
+ struct module *me)
{
/* One extra reloc so it's always 0-funcaddr terminated */
unsigned long relocs = 1;
You might want to get rid of 'relocs' above and simply use the below
two.
+ /*
+ * size of livepatch stub is 28 instructions, whereas the
+ * non-livepatch stub requires 7 instructions. Account for
+ * different stub sizes and track the livepatch relocation
+ * count in me->arch.klp_relocs.
+ */
+ unsigned long sec_relocs = 0;
+ unsigned long klp_relocs = 0;
You should also initialize this to 1 (similar to relocs above) for use
in the iterators below. Or, update the iterators to use
me->arch.klp_relocs (1)
relocs is Sum(sec_relocs), whereas per section relocation count is
assigned to sec_relocs. If the section is livepatch section, then it
added to klp_relocs or else to relocs. sec_relocs acts like a temp
variable to hold the current section relocation count.
unsigned i;
/* Every relocated section... */
@@ -262,9 +289,14 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
sechdrs[i].sh_size / sizeof(Elf64_Rela),
sizeof(Elf64_Rela), relacmp, relaswap);
- relocs += count_relocs((void *)sechdrs[i].sh_addr,
+ sec_relocs = count_relocs((void *)sechdrs[i].sh_addr,
sechdrs[i].sh_size
/ sizeof(Elf64_Rela));
How about also capturing 'sec_relocs' in 'struct mod_arch_specific'? (2)
That will help simplify a lot of the calculations here and elsewhere.
Note that there are many other places where the number of stubs is
derived looking at 'sh_size', which is incorrect since we now have klp
stubs as well (create_ftrace_stub() for instance).
+ relocs += sec_relocs;
+#ifdef CONFIG_LIVEPATCH
+ if (sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+ klp_relocs += sec_relocs;
+#endif
If a module has SHF_RELA_LIVEPATCH, but the kernel is compiled without
CONFIG_LIVEPATCH, should we refuse to load the kernel module?
Yes, the module load will fail.
You might want to consider removing the above #ifdef and processing some
of these flags regardless of CONFIG_LIVEPATCH.
ifdef guard, can be replaced with check for SHF_RELA_LIVEPATCH in
section flag.
}
}
@@ -273,6 +305,15 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
relocs++;
#endif
+ relocs -= klp_relocs;
+#ifdef CONFIG_LIVEPATCH
+ me->arch.klp_relocs = klp_relocs;
+
+ pr_debug("Looks like a total of %lu stubs, (%lu) livepatch stubs,
max\n",
^^^^^
(%lu livepatch stubs)
Just wondering why the brackets are the way they are...
Makes it better to use the brackets like you suggested.
+ relocs, klp_relocs);
+ return (relocs * sizeof(struct ppc64_stub_entry) +
+ klp_relocs * sizeof(struct ppc64le_klp_stub_entry));
+#endif
pr_debug("Looks like a total of %lu stubs, max\n", relocs);
return relocs * sizeof(struct ppc64_stub_entry);
}
[...]
+#ifdef CONFIG_LIVEPATCH
+static unsigned long klp_stub_for_addr(const Elf64_Shdr *sechdrs,
+ unsigned long addr,
+ struct module *me)
+{
+ struct ppc64le_klp_stub_entry *klp_stubs;
+ unsigned int num_klp_stubs = me->arch.klp_relocs;
+ unsigned int i, num_stubs;
+
+ num_stubs = (sechdrs[me->arch.stubs_section].sh_size -
+ (num_klp_stubs * sizeof(*klp_stubs))) /
+ sizeof(struct ppc64_stub_entry);
(2) This can be simplified if we have me->arch.sec_relocs.
Having it will make the calculation look simple:
num_stubs = (me->arch.sec_relocs * size(struct ppc64_stub_entry).
+
+ /*
+ * Create livepatch stubs after the regular stubs.
+ */
+ klp_stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr +
+ (num_stubs * sizeof(struct ppc64_stub_entry));
+ for (i = 0; stub_func_addr(klp_stubs[i].funcdata); i++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(1) This needs us to allocate one additional stub.
Good catch, yes there should an extra stub.
--
cheers,
Kamalesh.
