| Message ID | b1dbbb34a389a6f59eb6c99102d94c0070ddaf98.1587654213.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Enhance error handling with patch_instruction() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (47e80b4d8b45ae1bd3a1fe8577e95571cb8a976e) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
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
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
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
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
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
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 > > > >
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
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.
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
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. > > 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> A similar patch was merged as https://github.com/linuxppc/linux/commit/a3483c3dd18c136785a31406fe27210649fc4fba#diff-e084bb6dc223aec74e7fc4208b7b260acc571bd5b50c9b709ec3de175cb1a979 Christophe > --- > 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; > } > #else /* !CONFIG_STRICT_KERNEL_RWX */ >
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 */
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(-)