Re: [PATCH v4 2/3] powerpc/modules: Don't try to restore r2 after a sibling call

Josh Poimboeuf wrote:

On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote:

quoted

Kamalesh Babulal wrote:

quoted

From: Josh Poimboeuf <redacted>
=20
When attempting to load a livepatch module, I got the following error:
=20
  module_64: patch_module: Expect noop after relocate, got 3c820000
=20
The error was triggered by the following code in
unregister_netdevice_queue():
=20
  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c=

quoted

quoted

                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0
=20
GCC didn't insert a nop after the branch to net_set_todo() because it'=

s

quoted

quoted

a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.
=20
Signed-off-by: Josh Poimboeuf <redacted>
Signed-off-by: Kamalesh Babulal <redacted>
---
 arch/powerpc/kernel/module_64.c | 4 ++++
 1 file changed, 4 insertions(+)
=20

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/mod=

ule_64.c

quoted

quoted

index 39b01fd..9e5391f 100644

--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct mo=

dule *me)

quoted

quoted

 	if (is_early_mcount_callsite(instruction - 1))
 		return 1;
=20
+	/* Sibling calls don't return, so they don't need to restore r2 */
+	if (instruction[-1] =3D=3D PPC_INST_BRANCH)
+		return 1;
+

=20
This looks quite fragile, unless we know for sure that gcc will _always_
emit this instruction form for sibling calls with relocations.
=20
As an alternative, does it make sense to do the following check instead?
	if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn))
		&& !(insn & 0x1))

=20
Yes, good point.  How about something like this?

That looks good to me (with a very minor nit below).
Acked-by: Naveen N. Rao <redacted>

ude/asm/code-patching.h

et, int flags);

 int patch_instruction(unsigned int *addr, unsigned int instr);
=20
 int instr_is_relative_branch(unsigned int instr);
+int instr_is_link_branch(unsigned int instr);
 int instr_is_branch_to_addr(const unsigned int *instr, unsigned long add=

r);

_64.c

on)

ching.c

&

+	       (instr & BRANCH_SET_LINK);
+}
+

Nitpicking here, but since we're not considering the other branch forms,
perhaps this can be renamed to instr_is_link_relative_branch() (or maybe=20
instr_is_relative_branch_link()), just so we're clear :)


- Naveen

 static unsigned long branch_iform_target(const unsigned int *instr)
 {
 	signed long imm;
--
To unsubscribe from this list: send the line "unsubscribe live-patching" =

in

the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=20
=20

=