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

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

[Kamalesh Babulal]

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

[PATCH v4 1/3] kernel/modules: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

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 <kamalesh@linux.vnet.ibm.com>
CC: Balbir Singh <bsingharora@gmail.com>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
Cc: Torsten Duwe <duwe@lst.de>
---
 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

Re: [v4, 1/3] kernel/modules: Add REL24 relocation support of livepatch symbols

[Michael Ellerman]

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 <kamalesh@linux.vnet.ibm.com>
> CC: Balbir Singh <bsingharora@gmail.com>
> Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Cc: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> Cc: Torsten Duwe <duwe@lst.de>

Applied to powerpc next, thanks.

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

cheers

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

[Kamalesh Babulal]

From: Josh Poimboeuf <jpoimboe@redhat.com>

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 <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
---
 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

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

[Naveen N. Rao]

Kamalesh Babulal wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
>=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>
>                          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
> a sibling call, so it never returns.  The nop isn't needed after the
> branch in that case.
>=20
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> ---
>  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/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 modul=
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

=

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

[Josh Poimboeuf]

On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote:
> Kamalesh Babulal wrote:
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > 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 <jpoimboe@redhat.com>
> > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> > ---
> >  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;

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

[Kamalesh Babulal]

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:
>> Kamalesh Babulal wrote:
>>> From: Josh Poimboeuf <jpoimboe@redhat.com>
>>>
>>> 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 <jpoimboe@redhat.com>
>>> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
>>> ---
>>>  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 <kamalesh@linux.vnet.ibm.com>

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


-- 
cheers,
Kamalesh.

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

[Naveen N. Rao]

Josh Poimboeuf wrote:
> On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote:
>> Kamalesh Babulal wrote:
>> > From: Josh Poimboeuf <jpoimboe@redhat.com>
>> >=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=
>
>> >                          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
>> > a sibling call, so it never returns.  The nop isn't needed after the
>> > branch in that case.
>> >=20
>> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
>> > ---
>> >  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
>> > 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)
>> >  	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 <naveen.n.rao@linux.vnet.ibm.com>

>=20
> (completely untested because I don't have access to a box at the moment)
>=20
>=20
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/incl=
ude/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 targ=
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);
>  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 *instructi=
on)
>     restore r2. */
>  static int restore_r2(u32 *instruction, struct module *me)
>  {
> -	if (is_early_mcount_callsite(instruction - 1))
> +	u32 *prev_insn =3D instruction - 1;
> +
> +	if (is_early_mcount_callsite(prev_insn))
>  		return 1;
>=20
>  	/* Sibling calls don't return, so they don't need to restore r2 */
> -	if (instruction[-1] =3D=3D PPC_INST_BRANCH)
> +	if (!instr_is_link_branch(*prev_insn))
>  		return 1;
>=20
>  	if (*instruction !=3D PPC_INST_NOP) {
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-pat=
ching.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);
>  }
>=20
> +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=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
=

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

[Josh Poimboeuf]

On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
> > +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

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

[Naveen N. Rao]

Josh Poimboeuf wrote:
> On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
>> > +int instr_is_link_branch(unsigned int instr)
>> > +{
>> > +	return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)=
) &&
>> > +	       (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

=

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

[Josh Poimboeuf]

On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:
> Josh Poimboeuf wrote:
> > On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
> > > > +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 <jpoimboe@redhat.com>
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 <jpoimboe@redhat.com>
---
 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

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

[Kamalesh Babulal]

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:
>> Josh Poimboeuf wrote:
>>> On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
>>>>> +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 <jpoimboe@redhat.com>
> 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 <jpoimboe@redhat.com>

Reviewed-and-tested-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

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

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

[Naveen N. Rao]

Kamalesh Babulal wrote:
> 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:
>>> Josh Poimboeuf wrote:
>>>> On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
>>>>>> +int instr_is_link_branch(unsigned int instr)
>>>>>> +{
>>>>>> +	return (instr_is_branch_iform(instr) || instr_is_branch_bform(inst=
r)) &&
>>>>>> +	       (instr & BRANCH_SET_LINK);
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Nitpicking here, but since we're not considering the other branch for=
ms,
>>>>> perhaps this can be renamed to instr_is_link_relative_branch() (or ma=
ybe
>>>>> 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
>>>> 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
>>>> 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 ;-)

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

>>
>>> Thinking about this more, aren't we only interested in relative branche=
s
>>> here (for relocations), so can we actually filter out the absolute bran=
ches?
>>> 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?

Thanks. That looks good to me.
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

>>
>> ----8<----
>>
>> From: Josh Poimboeuf <jpoimboe@redhat.com>
>> Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a s=
ibling 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 <jpoimboe@redhat.com>
>=20
> Reviewed-and-tested-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

Thanks, Kamalesh!


- Naveen

=

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

[Michael Ellerman]

On Thu, 2017-11-16 at 17:45:37 UTC, Josh Poimboeuf wrote:
> 
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 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 <jpoimboe@redhat.com>
> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

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

cheers

[PATCH v4 3/3] powerpc/modules: Improve restore_r2() error message

[Kamalesh Babulal]

From: Josh Poimboeuf <jpoimboe@redhat.com>

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 <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
---
 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

Re: [PATCH v4 3/3] powerpc/modules: Improve restore_r2() error message

[Michael Ellerman]

Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> writes:

> From: Josh Poimboeuf <jpoimboe@redhat.com>
>
> 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

Re: [v4,3/3] powerpc/modules: Improve restore_r2() error message

[Michael Ellerman]

On Tue, 2017-11-14 at 09:29:10 UTC, Kamalesh Babulal wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> 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 <jpoimboe@redhat.com>
> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

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

cheers