diff mbox series

[1/3] powerpc: Properly return error code from do_patch_instruction()

Message ID b1dbbb34a389a6f59eb6c99102d94c0070ddaf98.1587654213.git.naveen.n.rao@linux.vnet.ibm.com
State New
Headers show
Series powerpc: Enhance error handling with patch_instruction() | expand

Checks

Context Check Description
snowpatch_ozlabs/needsstable warning Please consider tagging this patch for stable!
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 24 lines checked
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (47e80b4d8b45ae1bd3a1fe8577e95571cb8a976e)

Commit Message

Naveen N. Rao April 23, 2020, 3:09 p.m. UTC
With STRICT_KERNEL_RWX, we are currently ignoring return value from
__patch_instruction() in do_patch_instruction(), resulting in the error
not being propagated back. Fix the same.

Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/lib/code-patching.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Christophe Leroy April 23, 2020, 4:21 p.m. UTC | #1
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> With STRICT_KERNEL_RWX, we are currently ignoring return value from
> __patch_instruction() in do_patch_instruction(), resulting in the error
> not being propagated back. Fix the same.

Good patch.

Be aware that there is ongoing work which tend to wanting to replace 
error reporting by BUG_ON() . See 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

> 
> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/lib/code-patching.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 3345f039a876..5c713a6c0bd8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>   
>   static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   {
> -	int err;
> +	int err, rc = 0;
>   	unsigned int *patch_addr = NULL;
>   	unsigned long flags;
>   	unsigned long text_poke_addr;
> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   	patch_addr = (unsigned int *)(text_poke_addr) +
>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>   
> -	__patch_instruction(addr, instr, patch_addr);
> +	rc = __patch_instruction(addr, instr, patch_addr);
>   
>   	err = unmap_patch_area(text_poke_addr);
>   	if (err)
> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   out:
>   	local_irq_restore(flags);
>   
> -	return err;
> +	return rc ? rc : err;

That's not really consistent. __patch_instruction() and 
unmap_patch_area() return a valid minus errno, while in case of 
map_patch_area() failure, err has value -1

>   }
>   #else /* !CONFIG_STRICT_KERNEL_RWX */
>   
> 

Christophe
Steven Rostedt April 24, 2020, 1:15 p.m. UTC | #2
On Thu, 23 Apr 2020 18:21:14 +0200
Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > __patch_instruction() in do_patch_instruction(), resulting in the error
> > not being propagated back. Fix the same.  
> 
> Good patch.
> 
> Be aware that there is ongoing work which tend to wanting to replace 
> error reporting by BUG_ON() . See 
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

Thanks for the reference. I still believe that WARN_ON() should be used in
99% of the cases, including here. And only do a BUG_ON() when you know
there's no recovering from it.

In fact, there's still BUG_ON()s in my code that I need to convert to
WARN_ON() (it was written when BUG_ON() was still acceptable ;-)

-- Steve
Naveen N. Rao April 24, 2020, 6:02 p.m. UTC | #3
Christophe Leroy wrote:
> 
> 
> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> __patch_instruction() in do_patch_instruction(), resulting in the error
>> not being propagated back. Fix the same.
> 
> Good patch.
> 
> Be aware that there is ongoing work which tend to wanting to replace 
> error reporting by BUG_ON() . See 
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

Hah, I see that you pointed out this exact issue in your review there!

I had noticed this when Russell's series for STRICT_MODULE_RWX started 
causing kretprobe failures, due to one of the early boot-time patching 
failing silently.

I'll defer to Michael on which patch he prefers to take, between this 
one and the series you point out above.

> 
>> 
>> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/lib/code-patching.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>> index 3345f039a876..5c713a6c0bd8 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>>   
>>   static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   {
>> -	int err;
>> +	int err, rc = 0;
>>   	unsigned int *patch_addr = NULL;
>>   	unsigned long flags;
>>   	unsigned long text_poke_addr;
>> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   	patch_addr = (unsigned int *)(text_poke_addr) +
>>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>>   
>> -	__patch_instruction(addr, instr, patch_addr);
>> +	rc = __patch_instruction(addr, instr, patch_addr);
>>   
>>   	err = unmap_patch_area(text_poke_addr);
>>   	if (err)
>> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   out:
>>   	local_irq_restore(flags);
>>   
>> -	return err;
>> +	return rc ? rc : err;
> 
> That's not really consistent. __patch_instruction() and 
> unmap_patch_area() return a valid minus errno, while in case of 
> map_patch_area() failure, err has value -1

Not sure I follow -- I'm not changing what would be returned in those 
cases, just also capturing return value from __patch_instruction().

If anything, I've considered the different return codes to be a good 
thing -- return code gives you a clear idea of what exactly failed.


- Naveen
Naveen N. Rao April 24, 2020, 6:07 p.m. UTC | #4
Hi Steve,

Steven Rostedt wrote:
> On Thu, 23 Apr 2020 18:21:14 +0200
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> 
>> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> > With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> > __patch_instruction() in do_patch_instruction(), resulting in the error
>> > not being propagated back. Fix the same.  
>> 
>> Good patch.
>> 
>> Be aware that there is ongoing work which tend to wanting to replace 
>> error reporting by BUG_ON() . See 
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
> 
> Thanks for the reference. I still believe that WARN_ON() should be used in
> 99% of the cases, including here. And only do a BUG_ON() when you know
> there's no recovering from it.

I'm not sure if you meant that we should have a WARN_ON() in 
patch_instruction(), or if it was about the users of 
patch_instruction(). As you're well aware, ftrace likes to do its own 
WARN_ON() if any of its operations fail through ftrace_bug(). That was 
the reason I didn't add anything here.


- Naveen
Steven Rostedt April 24, 2020, 6:29 p.m. UTC | #5
On Fri, 24 Apr 2020 23:37:06 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> >> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :  
> >> > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> >> > __patch_instruction() in do_patch_instruction(), resulting in the error
> >> > not being propagated back. Fix the same.    
> >> 
> >> Good patch.
> >> 
> >> Be aware that there is ongoing work which tend to wanting to replace 
> >> error reporting by BUG_ON() . See 
> >> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003  
> > 
> > Thanks for the reference. I still believe that WARN_ON() should be used in
> > 99% of the cases, including here. And only do a BUG_ON() when you know
> > there's no recovering from it.  
> 
> I'm not sure if you meant that we should have a WARN_ON() in 
> patch_instruction(), or if it was about the users of 
> patch_instruction(). As you're well aware, ftrace likes to do its own 
> WARN_ON() if any of its operations fail through ftrace_bug(). That was 
> the reason I didn't add anything here.

I'm fine with that too, and better reason not to call BUG_ON(), because I'm
guessing if we crash, we never make it to the ftrace_bug() which reports
information that can be used to debug what went wrong.

-- Steve
Christopher M. Riedl April 24, 2020, 7:26 p.m. UTC | #6
On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
> On Thu, 23 Apr 2020 18:21:14 +0200
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
> 
> > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > > __patch_instruction() in do_patch_instruction(), resulting in the error
> > > not being propagated back. Fix the same.  
> > 
> > Good patch.
> > 
> > Be aware that there is ongoing work which tend to wanting to replace 
> > error reporting by BUG_ON() . See 
> > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
>
> 
> Thanks for the reference. I still believe that WARN_ON() should be used
> in
> 99% of the cases, including here. And only do a BUG_ON() when you know
> there's no recovering from it.
>
> 
> In fact, there's still BUG_ON()s in my code that I need to convert to
> WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
>
Figured I'd chime in since I am working on that other series :) The
BUG_ON()s are _only_ in the init code to set things up to allow a
temporary mapping for patching a STRICT_RWX kernel later. There's no
ongoing work to "replace error reporting by BUG_ON()". If that initial
setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
warrants a BUG_ON(). I am still working on v2 of my RFC which does
return any __patch_instruction() error back to the caller of
patch_instruction() similar to this patch.
> 
> -- Steve
>
> 
>
>
Steven Rostedt April 25, 2020, 2:10 p.m. UTC | #7
On Fri, 24 Apr 2020 14:26:02 -0500
"Christopher M. Riedl" <cmr@informatik.wtf> wrote:

> On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
> > On Thu, 23 Apr 2020 18:21:14 +0200
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >   
> > > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :  
> > > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > > > __patch_instruction() in do_patch_instruction(), resulting in the error
> > > > not being propagated back. Fix the same.    
> > > 
> > > Good patch.
> > > 
> > > Be aware that there is ongoing work which tend to wanting to replace 
> > > error reporting by BUG_ON() . See 
> > > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003  
> >
> > 
> > Thanks for the reference. I still believe that WARN_ON() should be used
> > in
> > 99% of the cases, including here. And only do a BUG_ON() when you know
> > there's no recovering from it.
> >
> > 
> > In fact, there's still BUG_ON()s in my code that I need to convert to
> > WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
> >  
> Figured I'd chime in since I am working on that other series :) The
> BUG_ON()s are _only_ in the init code to set things up to allow a
> temporary mapping for patching a STRICT_RWX kernel later. There's no
> ongoing work to "replace error reporting by BUG_ON()". If that initial
> setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
> warrants a BUG_ON(). I am still working on v2 of my RFC which does
> return any __patch_instruction() error back to the caller of
> patch_instruction() similar to this patch.


I agree certain locations may warrant a BUG_ON(), but I wouldn't make a
generic operation like patch_instruction() BUG, as it may be used in
cases that do not warrant it (like setting up ftrace).

Deciding to BUG on not based on the return code of patch_instruction()
is the way to go IMO.

-- Steve
Steven Rostedt April 25, 2020, 2:11 p.m. UTC | #8
On Sat, 25 Apr 2020 10:10:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Deciding to BUG on not based on the return code of patch_instruction()

That was suppose to be "to BUG on or not,"

-- Steve

> is the way to go IMO.
Naveen N. Rao April 27, 2020, 5:14 p.m. UTC | #9
Christopher M. Riedl wrote:
> On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
>> On Thu, 23 Apr 2020 18:21:14 +0200
>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>> 
>> > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> > > __patch_instruction() in do_patch_instruction(), resulting in the error
>> > > not being propagated back. Fix the same.  
>> > 
>> > Good patch.
>> > 
>> > Be aware that there is ongoing work which tend to wanting to replace 
>> > error reporting by BUG_ON() . See 
>> > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
>>
>> 
>> Thanks for the reference. I still believe that WARN_ON() should be used
>> in
>> 99% of the cases, including here. And only do a BUG_ON() when you know
>> there's no recovering from it.
>>
>> 
>> In fact, there's still BUG_ON()s in my code that I need to convert to
>> WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
>>
> Figured I'd chime in since I am working on that other series :) The
> BUG_ON()s are _only_ in the init code to set things up to allow a
> temporary mapping for patching a STRICT_RWX kernel later. There's no
> ongoing work to "replace error reporting by BUG_ON()". If that initial
> setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
> warrants a BUG_ON(). I am still working on v2 of my RFC which does
> return any __patch_instruction() error back to the caller of
> patch_instruction() similar to this patch.

Ok, that's good to know. I will drop this patch from my series, since 
this can be done independently of the other changes.

- Naveen
diff mbox series

Patch

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 3345f039a876..5c713a6c0bd8 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -138,7 +138,7 @@  static inline int unmap_patch_area(unsigned long addr)
 
 static int do_patch_instruction(unsigned int *addr, unsigned int instr)
 {
-	int err;
+	int err, rc = 0;
 	unsigned int *patch_addr = NULL;
 	unsigned long flags;
 	unsigned long text_poke_addr;
@@ -163,7 +163,7 @@  static int do_patch_instruction(unsigned int *addr, unsigned int instr)
 	patch_addr = (unsigned int *)(text_poke_addr) +
 			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
 
-	__patch_instruction(addr, instr, patch_addr);
+	rc = __patch_instruction(addr, instr, patch_addr);
 
 	err = unmap_patch_area(text_poke_addr);
 	if (err)
@@ -172,7 +172,7 @@  static int do_patch_instruction(unsigned int *addr, unsigned int instr)
 out:
 	local_irq_restore(flags);
 
-	return err;
+	return rc ? rc : err;
 }
 #else /* !CONFIG_STRICT_KERNEL_RWX */