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

Message ID 20171114092910.20399-3-kamalesh@linux.vnet.ibm.com
State New
Headers show
Series
  • ppc64le: Add REL24 relocation support of livepatch symbols
Related show

Commit Message

Kamalesh Babulal Nov. 14, 2017, 9:29 a.m.
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(+)

Comments

Naveen N. Rao Nov. 14, 2017, 10:29 a.m. | #1
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))


- Naveen
Josh Poimboeuf Nov. 14, 2017, 3:53 p.m. | #2
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;
Kamalesh Babulal Nov. 15, 2017, 5:38 a.m. | #3
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;
>
Naveen N. Rao Nov. 15, 2017, 9:28 a.m. | #4
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?

That looks good to me (with a very minor nit below).
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

> 
> (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);
> +}
> +

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


- 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
> 
>
Josh Poimboeuf Nov. 16, 2017, 1:26 a.m. | #5
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?
Naveen N. Rao Nov. 16, 2017, 1:09 p.m. | #6
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.

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


- Naveen

Patch

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