From: Joe Lawrence <joe.lawrence@redhat.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: rostedt@goodmis.org, mingo@redhat.com, jpoimboe@redhat.com,
jikos@kernel.org, pmladek@suse.com, linux-kernel@vger.kernel.org,
live-patching@vger.kernel.org
Subject: Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag
Date: Tue, 8 Oct 2019 15:35:34 -0400 [thread overview]
Message-ID: <20191008193534.GA16675@redhat.com> (raw)
In-Reply-To: <20191007081714.20259-1-mbenes@suse.cz>
On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote:
> Livepatch uses ftrace for redirection to new patched functions. It is
> thus directly affected by ftrace sysctl knobs such as ftrace_enabled.
> Setting ftrace_enabled to 0 also disables all live patched functions. It
> is not a problem per se, because only administrator can set sysctl
> values, but it still may be surprising.
>
> Introduce PERMANENT ftrace_ops flag to amend this. If the
> FTRACE_OPS_FL_PERMANENT is set, the tracing of the function is not
> disabled. Such ftrace_ops can still be unregistered in a standard way.
>
> The patch set passes ftrace and livepatch kselftests.
>
> Miroslav Benes (3):
> ftrace: Make test_rec_ops_needs_regs() generic
> ftrace: Introduce PERMANENT ftrace_ops flag
> livepatch: Use FTRACE_OPS_FL_PERMANENT
>
> Documentation/trace/ftrace-uses.rst | 6 ++++
> Documentation/trace/ftrace.rst | 2 ++
> include/linux/ftrace.h | 8 +++--
> kernel/livepatch/patch.c | 3 +-
> kernel/trace/ftrace.c | 47 ++++++++++++++++++++++++-----
> 5 files changed, 55 insertions(+), 11 deletions(-)
>
> --
> 2.23.0
>
Hi Miroslav,
I wonder if the opposite would be more intuitive: when ftrace_enabled is
not set, don't allow livepatches to register ftrace filters and
likewise, don't allow ftrace_enabled to be unset if any livepatches are
already registered. I guess you could make an argument either way, but
just offering another option. Perhaps livepatches should follow similar
behavior of other ftrace clients (like perf probes?)
As for the approach in this patchset, is it consistent that livepatches
loaded after setting ftrace_enabled to 0 will successfully load, but not
execute their new code... but then when ftrace_enabled is toggled, the
new livepatch code remains on?
For example:
1 - Turn ftrace_enabled off and load the /proc/cmdline livepatch test
case, note that it reports a success patching transition, but
doesn't run new its code:
% dmesg -C
% sysctl kernel.ftrace_enabled=0
kernel.ftrace_enabled = 0
% insmod lib/livepatch/test_klp_livepatch.ko
% echo $?
0
% dmesg
[ 450.579980] livepatch: enabling patch 'test_klp_livepatch'
[ 450.581243] livepatch: 'test_klp_livepatch': starting patching transition
[ 451.942971] livepatch: 'test_klp_livepatch': patching complete
% cat /proc/cmdline
BOOT_IMAGE=(hd0,msdos1)/boot/vmlinuz-5.4.0-rc2+ root=UUID=c42bb089-b5c1-4e17-82bd-132f55bee54c ro console=ttyS0 console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto
2 - Turn ftrace_enabled on and see that the livepatch now works:
% sysctl kernel.ftrace_enabled=1
kernel.ftrace_enabled = 1
% cat /proc/cmdline
test_klp_livepatch: this has been live patched
3 - Turn ftrace_enabled off and see that it's still enabled:
% sysctl kernel.ftrace_enabled=0
kernel.ftrace_enabled = 0
% cat /proc/cmdline
test_klp_livepatch: this has been live patched
Steps 2 and 3 match the behavior described by the patchset, but I was
particularly wondering what you thought about step 1.
IMHO, I would expect step 1 to fully enable the livepatch, or at the
very least, not report a patch transition (though that may confuse
userspace tools waiting for that report).
Thanks,
-- Joe
next prev parent reply other threads:[~2019-10-08 19:35 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-07 8:17 [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes
2019-10-07 8:17 ` [PATCH 1/3] ftrace: Make test_rec_ops_needs_regs() generic Miroslav Benes
2019-10-07 8:17 ` [PATCH 2/3] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes
2019-10-07 8:17 ` [PATCH 3/3] livepatch: Use FTRACE_OPS_FL_PERMANENT Miroslav Benes
2019-10-08 19:35 ` Joe Lawrence [this message]
2019-10-08 19:50 ` [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Steven Rostedt
2019-10-09 10:33 ` Miroslav Benes
2019-10-09 11:22 ` Petr Mladek
2019-10-09 14:23 ` Joe Lawrence
2019-10-09 14:26 ` Steven Rostedt
2019-10-10 8:50 ` Petr Mladek
2019-10-10 13:14 ` Steven Rostedt
2019-10-10 13:38 ` Miroslav Benes
2019-10-10 13:43 ` Steven Rostedt
2019-10-10 13:44 ` Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191008193534.GA16675@redhat.com \
--to=joe.lawrence@redhat.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=mingo@redhat.com \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).