On Fri, 24 Apr 2020 14:26:02 -0500 "Christopher M. Riedl" <c...@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.le...@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
