On Tue, 2024-04-23 at 15:09 +0530, Naveen N Rao wrote:
> On Mon, Mar 25, 2024 at 04:53:00PM +1100, Benjamin Gray wrote:
> > 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/
> > Signed-off-by: Benjamin Gray <bg...@linux.ibm.com>
> >
> > ---
> >
> > 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.
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.
>
> > ---
> > arch/powerpc/kernel/module_64.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/module_64.c
> > b/arch/powerpc/kernel/module_64.c
> > index 7112adc597a8..e9bab599d0c2 100644
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -651,12 +651,11 @@ static inline int create_stub(const
> > Elf64_Shdr *sechdrs,
> > // func_desc_t is 8 bytes if ABIv2, else 16 bytes
> > desc = func_desc(addr);
> > for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) {
> > - if (patch_instruction(((u32 *)&entry->funcdata) +
> > i,
> > - ppc_inst(((u32
> > *)(&desc))[i])))
> > + if (patch_u32(((u32 *)&entry->funcdata) + i, ((u32
> > *)&desc)[i]))
> > return 0;
> > }
> >
> > - if (patch_instruction(&entry->magic,
> > ppc_inst(STUB_MAGIC)))
> > + if (patch_u32(&entry->magic, STUB_MAGIC))
> > return 0;
> >
> > return 1;
> > --
> > 2.44.0
> >
