Re: [PATCH v3 3/5] powerpc/64: Convert patch_instruction() to patch_u32()

[Benjamin Gray <bgray@linux.ibm.com>]

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 <bgray@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
> >