* [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback [not found] <20201028115244.995788961@goodmis.org> @ 2020-10-28 11:52 ` Steven Rostedt 2020-10-29 13:51 ` Miroslav Benes 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-10-28 11:52 UTC (permalink / raw) To: linux-kernel Cc: Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek, Joe Lawrence, live-patching From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> If a ftrace callback does not supply its own recursion protection and does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will make a helper trampoline to do so before calling the callback instead of just calling the callback directly. The default for ftrace_ops is going to assume recursion protection unless otherwise specified. Cc: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Jiri Kosina <jikos@kernel.org> Cc: Miroslav Benes <mbenes@suse.cz> Cc: Petr Mladek <pmladek@suse.com> Cc: Joe Lawrence <joe.lawrence@redhat.com> Cc: live-patching@vger.kernel.org Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- 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; /* * A variant of synchronize_rcu() is used to allow patching functions * where RCU is not watching, see klp_synchronize_transition(). @@ -117,6 +121,7 @@ static void notrace klp_ftrace_handler(unsigned long ip, unlock: preempt_enable_notrace(); + ftrace_test_recursion_unlock(bit); } /* -- 2.28.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-28 11:52 ` [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback Steven Rostedt @ 2020-10-29 13:51 ` Miroslav Benes 2020-10-29 14:37 ` Steven Rostedt 2020-10-29 14:57 ` Petr Mladek 0 siblings, 2 replies; 9+ messages in thread From: Miroslav Benes @ 2020-10-29 13:51 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence, live-patching On Wed, 28 Oct 2020, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > If a ftrace callback does not supply its own recursion protection and > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will > make a helper trampoline to do so before calling the callback instead of > just calling the callback directly. > > The default for ftrace_ops is going to assume recursion protection unless > otherwise specified. 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 :) 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. > Cc: Josh Poimboeuf <jpoimboe@redhat.com> > Cc: Jiri Kosina <jikos@kernel.org> > Cc: Miroslav Benes <mbenes@suse.cz> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Joe Lawrence <joe.lawrence@redhat.com> > Cc: live-patching@vger.kernel.org > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > 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. Thanks Miroslav ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-29 13:51 ` Miroslav Benes @ 2020-10-29 14:37 ` Steven Rostedt 2020-10-30 12:28 ` Steven Rostedt 2020-10-29 14:57 ` Petr Mladek 1 sibling, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-10-29 14:37 UTC (permalink / raw) To: Miroslav Benes Cc: linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence, live-patching On Thu, 29 Oct 2020 14:51:06 +0100 (CET) Miroslav Benes <mbenes@suse.cz> wrote: > > 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. It's probably what happens today. But if you add a WARN_ON_ONCE() it may not hurt. I also plan on adding code that reports when recursion has happened, because even if it's not a problem, recursion adds extra overhead. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-29 14:37 ` Steven Rostedt @ 2020-10-30 12:28 ` Steven Rostedt 0 siblings, 0 replies; 9+ messages in thread From: Steven Rostedt @ 2020-10-30 12:28 UTC (permalink / raw) To: Miroslav Benes Cc: linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Petr Mladek, Joe Lawrence, live-patching On Thu, 29 Oct 2020 10:37:44 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > I also plan on adding code that reports when recursion has happened, > because even if it's not a problem, recursion adds extra overhead. I did the above (will be posting that later, maybe next week), and found two bugs with the recursion code. :-/ One was in the nmi handling, where it never cleared the nmi bit (because it was zero, and thus ignored), and that caused all functions in NMI handlers to not be traced (because it thought it was a recursion). (see https://lore.kernel.org/r/20201030002722.766a22df@oasis.local.home) The second was the recursion algorithm depends on the preempt_count() being accurate, but when it transitions between context, and there's tracing in that transition, it could falsely record it as a recursion. I have a fix for both of these bugs and will be sending them up marked for stable after I finish testing them. This goes to show that the recursion reported should be implemented (but that will be for the next merge window). -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-29 13:51 ` Miroslav Benes 2020-10-29 14:37 ` Steven Rostedt @ 2020-10-29 14:57 ` Petr Mladek 2020-10-29 15:03 ` Miroslav Benes 2020-10-29 18:24 ` Steven Rostedt 1 sibling, 2 replies; 9+ messages in thread From: Petr Mladek @ 2020-10-29 14:57 UTC (permalink / raw) To: Miroslav Benes Cc: Steven Rostedt, linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Joe Lawrence, live-patching On Thu 2020-10-29 14:51:06, Miroslav Benes wrote: > On Wed, 28 Oct 2020, Steven Rostedt wrote: > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > > > If a ftrace callback does not supply its own recursion protection and > > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will > > make a helper trampoline to do so before calling the callback instead of > > just calling the callback directly. > > > > The default for ftrace_ops is going to assume recursion protection unless > > otherwise specified. It might be my lack skills to read English. But the above sentence sounds ambiguous to me. It is not clear to me who provides the recursion protection by default. Could you please make it more explicit, for example by: "The default for ftrace_ops is going to change. It will expect that handlers provide their own recursion protection." > 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() ;-) > 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. > > Cc: Josh Poimboeuf <jpoimboe@redhat.com> > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: Miroslav Benes <mbenes@suse.cz> > > Cc: Petr Mladek <pmladek@suse.com> > > Cc: Joe Lawrence <joe.lawrence@redhat.com> > > Cc: live-patching@vger.kernel.org > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > --- > > 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. Otherwise it looks good. Best Regards, Petr ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-29 14:57 ` Petr Mladek @ 2020-10-29 15:03 ` Miroslav Benes 2020-10-29 18:24 ` Steven Rostedt 1 sibling, 0 replies; 9+ messages in thread From: Miroslav Benes @ 2020-10-29 15:03 UTC (permalink / raw) To: Petr Mladek Cc: Steven Rostedt, linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Joe Lawrence, live-patching 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@redhat.com> > > > Cc: Jiri Kosina <jikos@kernel.org> > > > Cc: Miroslav Benes <mbenes@suse.cz> > > > Cc: Petr Mladek <pmladek@suse.com> > > > Cc: Joe Lawrence <joe.lawrence@redhat.com> > > > Cc: live-patching@vger.kernel.org > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-29 14:57 ` Petr Mladek 2020-10-29 15:03 ` Miroslav Benes @ 2020-10-29 18:24 ` Steven Rostedt 2020-10-30 9:48 ` Miroslav Benes 1 sibling, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-10-29 18:24 UTC (permalink / raw) To: Petr Mladek Cc: Miroslav Benes, linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Joe Lawrence, live-patching On Thu, 29 Oct 2020 15:57:09 +0100 Petr Mladek <pmladek@suse.com> wrote: > On Thu 2020-10-29 14:51:06, Miroslav Benes wrote: > > On Wed, 28 Oct 2020, Steven Rostedt wrote: > > > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > > > > > If a ftrace callback does not supply its own recursion protection and > > > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will > > > make a helper trampoline to do so before calling the callback instead of > > > just calling the callback directly. > > > > > > The default for ftrace_ops is going to assume recursion protection unless > > > otherwise specified. > > It might be my lack skills to read English. But the above sentence > sounds ambiguous to me. It is not clear to me who provides the > recursion protection by default. Could you please make it more > explicit, for example by: > > "The default for ftrace_ops is going to change. It will expect that > handlers provide their own recursion protection." It was originally written as something else, as my first series (that I didn't post) added the recursion flag, and then I needed one big nasty patch to remove them. Then I realized it would be fine to just keep the double recursion testing and remove the flag when it was no longer used. I then went back and wrote up that sentence, and yeah, it wasn't the best explanation. Your sentence is better, I'll update it. > > > > 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() ;-) > > > 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. And the WARN_ON_ONCE() use to cause a problem, until I fixed it: dfbf2897d0049 ("bug: set warn variable before calling WARN()") > > > > > Cc: Josh Poimboeuf <jpoimboe@redhat.com> > > > Cc: Jiri Kosina <jikos@kernel.org> > > > Cc: Miroslav Benes <mbenes@suse.cz> > > > Cc: Petr Mladek <pmladek@suse.com> > > > Cc: Joe Lawrence <joe.lawrence@redhat.com> > > > Cc: live-patching@vger.kernel.org > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > > --- > > > 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. > > Otherwise it looks good. Perhaps we can add that as a separate patch, because this patch doesn't add any real functionality change. It only moves the recursion testing from the helper function (which ftrace wraps all callbacks that do not have the RECURSION flags set, including this one) down to your callback. In keeping with one patch to do one thing principle, the added of WARN_ON_ONCE() should be a separate patch, as that will change the functionality. If that WARN_ON_ONCE() breaks things, I'd like it to be bisected to another patch other than this one. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-29 18:24 ` Steven Rostedt @ 2020-10-30 9:48 ` Miroslav Benes 2020-10-30 10:41 ` Petr Mladek 0 siblings, 1 reply; 9+ messages in thread From: Miroslav Benes @ 2020-10-30 9:48 UTC (permalink / raw) To: Steven Rostedt Cc: Petr Mladek, linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Joe Lawrence, live-patching > > > > + 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. > > > > Otherwise it looks good. > > Perhaps we can add that as a separate patch, because this patch doesn't add > any real functionality change. It only moves the recursion testing from the > helper function (which ftrace wraps all callbacks that do not have the > RECURSION flags set, including this one) down to your callback. > > In keeping with one patch to do one thing principle, the added of > WARN_ON_ONCE() should be a separate patch, as that will change the > functionality. > > If that WARN_ON_ONCE() breaks things, I'd like it to be bisected to another > patch other than this one. Works for me. Miroslav ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback 2020-10-30 9:48 ` Miroslav Benes @ 2020-10-30 10:41 ` Petr Mladek 0 siblings, 0 replies; 9+ messages in thread From: Petr Mladek @ 2020-10-30 10:41 UTC (permalink / raw) To: Miroslav Benes Cc: Steven Rostedt, linux-kernel, Masami Hiramatsu, Andrew Morton, Josh Poimboeuf, Jiri Kosina, Joe Lawrence, live-patching On Fri 2020-10-30 10:48:58, Miroslav Benes wrote: > > > > > + 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. > > > > > > Otherwise it looks good. > > > > Perhaps we can add that as a separate patch, because this patch doesn't add > > any real functionality change. It only moves the recursion testing from the > > helper function (which ftrace wraps all callbacks that do not have the > > RECURSION flags set, including this one) down to your callback. > > > > In keeping with one patch to do one thing principle, the added of > > WARN_ON_ONCE() should be a separate patch, as that will change the > > functionality. > > > > If that WARN_ON_ONCE() breaks things, I'd like it to be bisected to another > > patch other than this one. > > Works for me. +1 So, with the updated commit message: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-30 12:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20201028115244.995788961@goodmis.org> 2020-10-28 11:52 ` [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback Steven Rostedt 2020-10-29 13:51 ` Miroslav Benes 2020-10-29 14:37 ` Steven Rostedt 2020-10-30 12:28 ` Steven Rostedt 2020-10-29 14:57 ` Petr Mladek 2020-10-29 15:03 ` Miroslav Benes 2020-10-29 18:24 ` Steven Rostedt 2020-10-30 9:48 ` Miroslav Benes 2020-10-30 10:41 ` Petr Mladek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).