On Tue, May 14, 2024 at 04:39:30AM GMT, Christophe Leroy wrote:

Le 14/05/2024 à 04:59, Benjamin Gray a écrit :

quoted

On Tue, 2024-04-23 at 15:09 +0530, Naveen N Rao wrote:

quoted

On Mon, Mar 25, 2024 at 04:53:00PM +1100, Benjamin Gray wrote:

quoted

This use of patch_instruction() is working on 32 bit data, and can
fail
if the data looks like a prefixed instruction and the extra write
crosses a page boundary. Use patch_u32() to fix the write size.

Fixes: 8734b41b3efe ("powerpc/module_64: Fix livepatching for RO
modules")
Link: https://lore.kernel.org/all/20230203004649.1f59dbd4@yea/ (local)
Signed-off-by: Benjamin Gray <redacted>

---

v2: * Added the fixes tag, it seems appropriate even if the subject
does
       mention a more robust solution being required.

patch_u64() should be more efficient, but judging from the bug
report
it doesn't seem like the data is doubleword aligned.

Asking again, is that still the case? It looks like at least the
first
fix below can be converted to patch_u64().

- Naveen

Sorry, I think I forgot this question last time. Reading the commit
descriptions you linked, I don't see any mention of "entry->funcdata
will always be doubleword aligned because XYZ". If the patch makes it
doubleword aligned anyway, I wouldn't be confident asserting all
callers will always do this without looking into it a lot more.

No worries. I was asking primarily to check if you had noticed a 
specific issue with alignment.

As Christophe mentions, the structure is aligned. It is primarily 
allotted in a separate stubs section for modules. Looking at it closer 
though, I wonder if we need the below:

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index cccb1f78e058..0226d73a0007 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -428,8 +428,11 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
 
        /* Find .toc and .stubs sections, symtab and strtab */
        for (i = 1; i < hdr->e_shnum; i++) {
-               if (strcmp(secstrings + sechdrs[i].sh_name, ".stubs") == 0)
+               if (strcmp(secstrings + sechdrs[i].sh_name, ".stubs") == 0) {
                        me->arch.stubs_section = i;
+                       if (sechdrs[i].sh_addralign < 8)
+                               sechdrs[i].sh_addralign = 8;
+               }
 #ifdef CONFIG_PPC_KERNEL_PCREL
                else if (strcmp(secstrings + sechdrs[i].sh_name, ".data..percpu") == 0)
                        me->arch.pcpu_section = i;

quoted

Perhaps a separate series could optimise it with appropriate
justification/assertions to catch bad alignment. But I think leaving it
out of this series is fine because the original works in words, so it's
not regressing anything.

That should be fine.

As far as I can see, the struct is 64 bits aligned by definition so 
funcdata field is aligned too as there are just 8x u32 before it:

struct ppc64_stub_entry {
	/*
	 * 28 byte jump instruction sequence (7 instructions) that can
	 * hold ppc64_stub_insns or stub_insns. Must be 8-byte aligned
	 * with PCREL kernels that use prefix instructions in the stub.
	 */
	u32 jump[7];
	/* Used by ftrace to identify stubs */
	u32 magic;
	/* Data for the above code */
	func_desc_t funcdata;
} __aligned(8);

Thanks,
Naveen