[PATCH v4 0/3] ppc64le: Add REL24 relocation support of livepatch symbols

This patchset drop the approach of creating new stub type for livepatch
symbols and offloads the issue of handling local function becoming global
to kpatch tool via gcc-plugin.

In function restore_r2(), a check for sibling call is added and also
improves the error message on unexpected op-code.

v4:
 - Drop creation of stubs for livepatch symbols and offload solution
   to kpatch tool.
 - Introduce check for sibling call, when restoring r2 after branch. (Josh)
 - Improve error message in restore_r2(). (Josh)

v3:
 - Defined FUNC_DESC_OFFSET to calculate func_desc offset from
   struct ppc64le_klp_stub_entry.
 - Replaced BUG_ON() with WARN_ON() in klp_stub_for_addr().
 - Major commit message re-write.

v2:
 - Changed klp_stub construction to re-use livepatch_handler and
   additional patch code required for klp_stub, instead of duplicating it.
 - Minor comments and commit body edit.

Josh Poimboeuf (2):
  powerpc/modules: Don't try to restore r2 after a sibling call
  powerpc/modules: Improve restore_r2() error message

Kamalesh Babulal (1):
  kernel/modules: Add REL24 relocation support of livepatch symbols

 arch/powerpc/kernel/module_64.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.9.3

Livepatch re-uses module loader function apply_relocate_add() to write
relocations, instead of managing them by arch-dependent
klp_write_module_reloc() function.

apply_relocate_add() doesn't understand livepatch symbols (marked with
SHN_LIVEPATCH symbol section index) and assumes them to be local symbols
by default for R_PPC64_REL24 relocation type. It fails with an error,
when trying to calculate offset with local_entry_offset():

 module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!

Whereas livepatch symbols are essentially SHN_UNDEF, should be
called via stub used for global calls. This issue can be fixed by
teaching apply_relocate_add() to handle both SHN_UNDEF/SHN_LIVEPATCH
symbols via the same stub. This patch extends SHN_UNDEF code to handle
livepatch symbols too.

Signed-off-by: Kamalesh Babulal <redacted>
CC: Balbir Singh <bsingharora@gmail.com>
Cc: Naveen N. Rao <redacted>
Cc: Josh Poimboeuf <redacted>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Ananth N Mavinakayanahalli <redacted>
Cc: Aravinda Prasad <redacted>
Cc: Torsten Duwe <redacted>
---
 arch/powerpc/kernel/module_64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f896..39b01fd 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -613,7 +613,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
 		case R_PPC_REL24:
 			/* FIXME: Handle weak symbols here --RR */
-			if (sym->st_shndx == SHN_UNDEF) {
+			if (sym->st_shndx == SHN_UNDEF ||
+			    sym->st_shndx == SHN_LIVEPATCH) {
 				/* External: go via stub */
 				value = stub_for_addr(sechdrs, value, me);
 				if (!value)

-- 
2.9.3

From: Josh Poimboeuf <redacted>

Print the function address associated with the restore_r2() error to
make it easier to debug the problem.

Also clarify the wording a bit.

Before:

  module_64: patch_foo: Expect noop after relocate, got 3c820000

After:

  module_64: patch_foo: Expected noop after call, got 7c630034 at netdev_has_upper_dev+0x54/0xb0 [patch_foo]

Signed-off-by: Josh Poimboeuf <redacted>
Signed-off-by: Kamalesh Babulal <redacted>
---
 arch/powerpc/kernel/module_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9e5391f..dad3ea5 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -494,8 +494,8 @@ static int restore_r2(u32 *instruction, struct module *me)
 		return 1;
 
 	if (*instruction != PPC_INST_NOP) {
-		pr_err("%s: Expect noop after relocate, got %08x\n",
-		       me->name, *instruction);
+		pr_err("%s: Expected noop after call, got %08x at %pS\n",
+			me->name, *instruction, instruction);
 		return 0;
 	}
 	/* ld r2,R2_STACK_OFFSET(r1) */

-- 
2.9.3

From: Josh Poimboeuf <redacted>

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>
Signed-off-by: Kamalesh Babulal <redacted>
---
 arch/powerpc/kernel/module_64.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
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 module *me)
 	if (is_early_mcount_callsite(instruction - 1))
 		return 1;
 
+	/* Sibling calls don't return, so they don't need to restore r2 */
+	if (instruction[-1] == PPC_INST_BRANCH)
+		return 1;
+
 	if (*instruction != PPC_INST_NOP) {
 		pr_err("%s: Expect noop after relocate, got %08x\n",
 		       me->name, *instruction);

-- 
2.9.3

Kamalesh Babulal wrote:

_64.c

e *me)

 	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;
+

This looks quite fragile, unless we know for sure that gcc will _always_
emit this instruction form for sibling calls with relocations.

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))


- Naveen

=

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

Kamalesh Babulal wrote:

quoted

From: Josh Poimboeuf <redacted>

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>
Signed-off-by: Kamalesh Babulal <redacted>
---
 arch/powerpc/kernel/module_64.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
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 module *me)
 	if (is_early_mcount_callsite(instruction - 1))
 		return 1;

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

This looks quite fragile, unless we know for sure that gcc will _always_
emit this instruction form for sibling calls with relocations.

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))

Yes, good point.  How about something like this?

(completely untested because I don't have access to a box at the moment)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..302e4368debc 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h

@@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags);
 int patch_instruction(unsigned int *addr, unsigned int instr);
 
 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 addr);
 unsigned long branch_target(const unsigned int *instr);
 unsigned int translate_branch(const unsigned int *dest,

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9cb007bc7075..b5148a206b4d 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -487,11 +487,13 @@ static bool is_early_mcount_callsite(u32 *instruction)
    restore r2. */
 static int restore_r2(u32 *instruction, struct module *me)
 {
-	if (is_early_mcount_callsite(instruction - 1))
+	u32 *prev_insn = instruction - 1;
+
+	if (is_early_mcount_callsite(prev_insn))
 		return 1;
 
 	/* Sibling calls don't return, so they don't need to restore r2 */
-	if (instruction[-1] == PPC_INST_BRANCH)
+	if (!instr_is_link_branch(*prev_insn))
 		return 1;
 
 	if (*instruction != PPC_INST_NOP) {

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c9de03e0c1f1..4727fafd37e4 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c

@@ -304,6 +304,12 @@ int instr_is_relative_branch(unsigned int instr)
 	return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
 }
 
+int instr_is_link_branch(unsigned int instr)
+{
+	return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+	       (instr & BRANCH_SET_LINK);
+}
+
 static unsigned long branch_iform_target(const unsigned int *instr)
 {
 	signed long imm;

On Tuesday 14 November 2017 09:23 PM, 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>

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>
Signed-off-by: Kamalesh Babulal <redacted>
---
 arch/powerpc/kernel/module_64.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
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 module *me)
 	if (is_early_mcount_callsite(instruction - 1))
 		return 1;

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

This looks quite fragile, unless we know for sure that gcc will _always_
emit this instruction form for sibling calls with relocations.

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))

Yes, good point.  How about something like this?

(completely untested because I don't have access to a box at the moment)

Reviewed-and-tested-by: Kamalesh Babulal [off-list ref]

-- 
cheers,
Kamalesh.

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

=

On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:

quoted

+int instr_is_link_branch(unsigned int instr)
+{
+	return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+	       (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
instr_is_relative_branch_link()), just so we're clear :)

My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or the
i-form (unconditional).  And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch.  Or did I miss
something?

-- 
Josh

Josh Poimboeuf wrote:

On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:

quoted

quoted

+int instr_is_link_branch(unsigned int instr)
+{
+	return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)=

) &&

quoted

quoted

+	       (instr & BRANCH_SET_LINK);
+}
+

=20
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
instr_is_relative_branch_link()), just so we're clear :)

=20
My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or the
i-form (unconditional).  And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch.  Or did I miss
something?

Ah, good point. I was coming from the fact that we are only considering=20
the i-form and b-form branches and not the lr/ctr/tar based branches,=20
which are always absolute branches, but can also set the link register.

Thinking about this more, aren't we only interested in relative branches
here (for relocations), so can we actually filter out the absolute=20
branches? Something like this?

int instr_is_relative_branch_link(unsigned int instr)
{
	return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
	       !(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK));
}


- Naveen

=

On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:

Josh Poimboeuf wrote:

quoted

On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:

quoted

quoted

+int instr_is_link_branch(unsigned int instr)
+{
+	return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+	       (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
instr_is_relative_branch_link()), just so we're clear :)

My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or the
i-form (unconditional).  And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch.  Or did I miss
something?

Ah, good point. I was coming from the fact that we are only considering the
i-form and b-form branches and not the lr/ctr/tar based branches, which are
always absolute branches, but can also set the link register.

Hm, RISC is more complicated than I realized ;-)

Thinking about this more, aren't we only interested in relative branches
here (for relocations), so can we actually filter out the absolute branches?
Something like this?

int instr_is_relative_branch_link(unsigned int instr)
{
	return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
	       !(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK));

Yeah, makes sense to me.  Here's another try (also untested).  If this
looks ok, Kamalesh would you mind testing again?

----8<----

From: Josh Poimboeuf <redacted>
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>
---
 arch/powerpc/include/asm/code-patching.h |  1 +
 arch/powerpc/kernel/module_64.c          | 12 +++++++++++-
 arch/powerpc/lib/code-patching.c         |  5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..2c895e8d07f7 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h

@@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags);
 int patch_instruction(unsigned int *addr, unsigned int instr);
 
 int instr_is_relative_branch(unsigned int instr);
+int instr_is_relative_link_branch(unsigned int instr);
 int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
 unsigned long branch_target(const unsigned int *instr);
 unsigned int translate_branch(const unsigned int *dest,

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b99f9f..180c16f04063 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -487,7 +487,17 @@ static bool is_early_mcount_callsite(u32 *instruction)
    restore r2. */
 static int restore_r2(u32 *instruction, struct module *me)
 {
-	if (is_early_mcount_callsite(instruction - 1))
+	u32 *prev_insn = instruction - 1;
+
+	if (is_early_mcount_callsite(prev_insn))
+		return 1;
+
+	/*
+	 * Make sure the branch isn't a sibling call.  Sibling calls aren't
+	 * "link" branches and they don't return, so they don't need the r2
+	 * restore afterwards.
+	 */
+	if (!instr_is_relative_link_branch(*prev_insn))
 		return 1;
 
 	if (*instruction != PPC_INST_NOP) {

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c9de03e0c1f1..d81aab7441f7 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c

@@ -304,6 +304,11 @@ int instr_is_relative_branch(unsigned int instr)
 	return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
 }
 
+int instr_is_relative_link_branch(unsigned int instr)
+{
+	return instr_is_relative_branch(instr) && (instr & BRANCH_SET_LINK);
+}
+
 static unsigned long branch_iform_target(const unsigned int *instr)
 {
 	signed long imm;

-- 
2.13.6

On Thursday 16 November 2017 11:15 PM, Josh Poimboeuf wrote:

On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:

quoted

Josh Poimboeuf wrote:

quoted

On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:

quoted

quoted

+int instr_is_link_branch(unsigned int instr)
+{
+	return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+	       (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
instr_is_relative_branch_link()), just so we're clear :)

My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or the
i-form (unconditional).  And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch.  Or did I miss
something?

Ah, good point. I was coming from the fact that we are only considering the
i-form and b-form branches and not the lr/ctr/tar based branches, which are
always absolute branches, but can also set the link register.

Hm, RISC is more complicated than I realized ;-)

quoted

Thinking about this more, aren't we only interested in relative branches
here (for relocations), so can we actually filter out the absolute branches?
Something like this?

int instr_is_relative_branch_link(unsigned int instr)
{
	return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
	       !(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK));

Yeah, makes sense to me.  Here's another try (also untested).  If this
looks ok, Kamalesh would you mind testing again?

----8<----

From: Josh Poimboeuf <redacted>
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>

Reviewed-and-tested-by: Kamalesh Babulal [off-list ref]

Kamalesh Babulal wrote:

On Thursday 16 November 2017 11:15 PM, Josh Poimboeuf wrote:

quoted

On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:

quoted

Josh Poimboeuf wrote:

quoted

On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:

quoted

quoted

+int instr_is_link_branch(unsigned int instr)
+{
+	return (instr_is_branch_iform(instr) || instr_is_branch_bform(inst=

r)) &&

quoted

quoted

quoted

quoted

quoted

+	       (instr & BRANCH_SET_LINK);
+}
+

Nitpicking here, but since we're not considering the other branch for=

ms,

quoted

quoted

quoted

quoted

perhaps this can be renamed to instr_is_link_relative_branch() (or ma=

ybe

quoted

quoted

quoted

quoted

instr_is_relative_branch_link()), just so we're clear :)

My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or th=

e

quoted

quoted

quoted

i-form (unconditional).  And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch.  Or did I mis=

s

quoted

quoted

quoted

something?

Ah, good point. I was coming from the fact that we are only considering=

 the

quoted

quoted

i-form and b-form branches and not the lr/ctr/tar based branches, which=

 are

quoted

quoted

always absolute branches, but can also set the link register.

Hm, RISC is more complicated than I realized ;-)

As long as 'RISC' gets people to take a look ;D

quoted

quoted

Thinking about this more, aren't we only interested in relative branche=

s

quoted

quoted

here (for relocations), so can we actually filter out the absolute bran=

ches?

quoted

quoted

Something like this?

int instr_is_relative_branch_link(unsigned int instr)
{
	return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr))=

 &&

quoted

quoted

	       !(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK));

Yeah, makes sense to me.  Here's another try (also untested).  If this
looks ok, Kamalesh would you mind testing again?

Thanks. That looks good to me.
Acked-by: Naveen N. Rao <redacted>

quoted

----8<----

From: Josh Poimboeuf <redacted>
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a s=

ibling call

quoted

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>

=20
Reviewed-and-tested-by: Kamalesh Babulal [off-list ref]

Thanks, Kamalesh!


- Naveen

=

Kamalesh Babulal [off-list ref] writes:

From: Josh Poimboeuf <redacted>

Print the function address associated with the restore_r2() error to
make it easier to debug the problem.

Also clarify the wording a bit.

Before:

  module_64: patch_foo: Expect noop after relocate, got 3c820000

After:

  module_64: patch_foo: Expected noop after call, got 7c630034 at netdev_has_upper_dev+0x54/0xb0 [patch_foo]

I renamed noop to nop, as that's the name of the instruction.

cheers

On Tue, 2017-11-14 at 09:29:10 UTC, Kamalesh Babulal wrote:

From: Josh Poimboeuf <redacted>

Print the function address associated with the restore_r2() error to
make it easier to debug the problem.

Also clarify the wording a bit.

Before:

  module_64: patch_foo: Expect noop after relocate, got 3c820000

After:

  module_64: patch_foo: Expected noop after call, got 7c630034 at netdev_has_upper_dev+0x54/0xb0 [patch_foo]

Signed-off-by: Josh Poimboeuf <redacted>
Signed-off-by: Kamalesh Babulal <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/1ea61ea23985c0f15c027e4c0ac022

cheers

On Tue, 2017-11-14 at 09:29:08 UTC, Kamalesh Babulal wrote:

Livepatch re-uses module loader function apply_relocate_add() to write
relocations, instead of managing them by arch-dependent
klp_write_module_reloc() function.

apply_relocate_add() doesn't understand livepatch symbols (marked with
SHN_LIVEPATCH symbol section index) and assumes them to be local symbols
by default for R_PPC64_REL24 relocation type. It fails with an error,
when trying to calculate offset with local_entry_offset():

 module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!

Whereas livepatch symbols are essentially SHN_UNDEF, should be
called via stub used for global calls. This issue can be fixed by
teaching apply_relocate_add() to handle both SHN_UNDEF/SHN_LIVEPATCH
symbols via the same stub. This patch extends SHN_UNDEF code to handle
livepatch symbols too.

Signed-off-by: Kamalesh Babulal <redacted>
CC: Balbir Singh <bsingharora@gmail.com>
Cc: Naveen N. Rao <redacted>
Cc: Josh Poimboeuf <redacted>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Ananth N Mavinakayanahalli <redacted>
Cc: Aravinda Prasad <redacted>
Cc: Torsten Duwe <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/a443bf6e8a7674b86221f4922cae82

cheers

On Thu, 2017-11-16 at 17:45:37 UTC, Josh Poimboeuf wrote:

From: Josh Poimboeuf <redacted>
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call

When attempting to load a livepatch module, I got the following error:

  module_64: patch_module: Expect noop after relocate, got 3c820000

The error was triggered by the following code in
unregister_netdevice_queue():

  14c:   00 00 00 48     b       14c <unregister_netdevice_queue+0x14c>
                         14c: R_PPC64_REL24      net_set_todo
  150:   00 00 82 3c     addis   r4,r2,0

GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns.  The nop isn't needed after the
branch in that case.

Signed-off-by: Josh Poimboeuf <redacted>
Acked-by: Naveen N. Rao <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/b9eab08d012fa093947b230f9a8725

cheers