On Thu, 29 Oct 2020, Petr Mladek wrote: > On Thu 2020-10-29 14:51:06, Miroslav Benes wrote: > > On Wed, 28 Oct 2020, Steven Rostedt wrote: > > > Hm, I've always thought that we did not need any kind of recursion > > protection for our callback. It is marked as notrace and it does not call > > anything traceable. In fact, it does not call anything. I even have a note > > in my todo list to mark the callback as RECURSION_SAFE :) > > Well, it calls WARN_ON_ONCE() ;-) Oh my, I learned to ignore these. Of course there is printk hidden everywhere. > > At the same time, it probably does not hurt and the patch is still better > > than what we have now without RECURSION_SAFE if I understand the patch set > > correctly. > > And better be on the safe side. Agreed. > > > Cc: Josh Poimboeuf <jpoimboe@xxxxxxxxxx> > > > Cc: Jiri Kosina <jikos@xxxxxxxxxx> > > > Cc: Miroslav Benes <mbenes@xxxxxxx> > > > Cc: Petr Mladek <pmladek@xxxxxxxx> > > > Cc: Joe Lawrence <joe.lawrence@xxxxxxxxxx> > > > Cc: live-patching@xxxxxxxxxxxxxxx > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx> > > > --- > > > kernel/livepatch/patch.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c > > > index b552cf2d85f8..6c0164d24bbd 100644 > > > --- a/kernel/livepatch/patch.c > > > +++ b/kernel/livepatch/patch.c > > > @@ -45,9 +45,13 @@ static void notrace klp_ftrace_handler(unsigned long ip, > > > struct klp_ops *ops; > > > struct klp_func *func; > > > int patch_state; > > > + int bit; > > > > > > ops = container_of(fops, struct klp_ops, fops); > > > > > > + bit = ftrace_test_recursion_trylock(); > > > + if (bit < 0) > > > + return; > > > > This means that the original function will be called in case of recursion. > > That's probably fair, but I'm wondering if we should at least WARN about > > it. > > Yeah, the early return might break the consistency model and > unexpected things might happen. We should be aware of it. > Please use: > > if (WARN_ON_ONCE(bit < 0)) > return; > > WARN_ON_ONCE() might be part of the recursion. But it should happen > only once. IMHO, it is worth the risk. Agreed. Miroslav
