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
Use FTRACE_OPS_FL_PERMANENT flag to be immune to toggling the
'ftrace_enabled' sysctl knob.
Signed-off-by: Miroslav Benes <[email protected]>
---
kernel/livepatch/patch.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index bd43537702bd..b552cf2d85f8 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -196,7 +196,8 @@ static int klp_patch_func(struct klp_func *func)
ops->fops.func = klp_ftrace_handler;
ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS |
FTRACE_OPS_FL_DYNAMIC |
- FTRACE_OPS_FL_IPMODIFY;
+ FTRACE_OPS_FL_IPMODIFY |
+ FTRACE_OPS_FL_PERMANENT;
list_add(&ops->node, &klp_ops);
--
2.23.0
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
On Tue, 8 Oct 2019 15:35:34 -0400
Joe Lawrence <[email protected]> wrote:
>
> 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?)
I believe I suggested the "PERMANENT" flag, but disabling ftrace_enable
may be another approach. Might be much easier to maintain.
>
> 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).
>
I think I like your idea better. To prevent ftrace_enable from being
disabled if a "permanent" option is set. Then we only need to have a
permanent flag for the ftrace_ops, that will disable the ftrace_enable
from being cleared. We can also prevent the ftrace_ops from being
loaded if ftrace_enable is not set and the ftrace_ops has the PERMANENT
flag set.
-- Steve
On Tue, 8 Oct 2019, Steven Rostedt wrote:
> On Tue, 8 Oct 2019 15:35:34 -0400
> Joe Lawrence <[email protected]> wrote:
>
> >
> > 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?)
>
> I believe I suggested the "PERMANENT" flag, but disabling ftrace_enable
> may be another approach. Might be much easier to maintain.
You did.
> >
> > 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?
No, it is not consistent and was not intended.
> > 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).
Yes.
>
> I think I like your idea better. To prevent ftrace_enable from being
> disabled if a "permanent" option is set. Then we only need to have a
> permanent flag for the ftrace_ops, that will disable the ftrace_enable
> from being cleared. We can also prevent the ftrace_ops from being
> loaded if ftrace_enable is not set and the ftrace_ops has the PERMANENT
> flag set.
Agreed. Joe's approach is better. Let me prepare v2.
Thanks
Miroslav
On Tue 2019-10-08 15:35:34, Joe Lawrence wrote:
> 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?)
I am not sure that I understand it correctly.
ftrace_enables is a global flag. My expectation is that it can be
manipulated at any time. But it should affect only ftrace handlers
without FTRACE_OPS_FL_PERMANENT flag.
By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and
only these handlers should ignore the global flag.
To be even more precise. If a function has registered more ftrace
handlers then the global ftrace_enable setting shold affect only
the handlers without the flag.
Is this the plan, please?
Best Regards,
Petr
On Wed, Oct 09, 2019 at 01:22:34PM +0200, Petr Mladek wrote:
> On Tue 2019-10-08 15:35:34, Joe Lawrence wrote:
> > 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?)
>
> I am not sure that I understand it correctly.
>
> ftrace_enables is a global flag. My expectation is that it can be
> manipulated at any time. But it should affect only ftrace handlers
> without FTRACE_OPS_FL_PERMANENT flag.
>
> By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and
> only these handlers should ignore the global flag.
>
> To be even more precise. If a function has registered more ftrace
> handlers then the global ftrace_enable setting shold affect only
> the handlers without the flag.
>
Petr,
I believe this is more or less what the patchset implemented. I
pointed out a small inconsistency in that livepatches loaded after
ftrace_enable=0 successfully transitioned despite the ftrace handlers
not being enabled for that livepatch. Toggling ftrace_enable would have
the side effect of enabling those handlers.
From the POV of ftrace I suppose this may be expected behavior and
nobody should be mucking with sysctl's that they don't fully understand.
However if I put on my sysadmin hat, I think I would find this slightly
confusing. At the very least, the livepatch load should make some
mention that its replacement functions aren't actually live.
Shoring up this quirk so that the FTRACE_OPS_FL_PERMANENT always
registers and fires might be simple enough solution...
On the other hand, I suggested that the presence of
FTRACE_OPS_FL_PERMANENT flags could prevent turning off ftrace_enable
and vice versa. That would offer the user immediate feedback without
introducing potentially unexpected (and silent) behavior.
I'm happy with either solution as long as it's consistent for the user
and easy to maintain :)
-- Joe
On Wed, 9 Oct 2019 13:22:34 +0200
Petr Mladek <[email protected]> wrote:
> > 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?)
>
> I am not sure that I understand it correctly.
>
> ftrace_enables is a global flag. My expectation is that it can be
> manipulated at any time. But it should affect only ftrace handlers
> without FTRACE_OPS_FL_PERMANENT flag.
No, it should affect *all* ftrace_ops (which it currently does). The
addition of the "PERMANENT" flag was going to change that to what you
are saying here. But thinking about this more, I believe that is the
wrong approach.
>
> By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and
> only these handlers should ignore the global flag.
>
> To be even more precise. If a function has registered more ftrace
> handlers then the global ftrace_enable setting shold affect only
> the handlers without the flag.
>
> Is this the plan, please?
I think Joe's approach is much easier to understand and implement. The
"ftrace_enabled" is a global flag, and affects all things ftrace (the
function bindings). What this patch was doing, was what you said. Make
ftrace_enabled only affect the ftrace_ops without the "PERMANENT" flag
set. But that is complex and requires a bit more accounting in the
ftrace system. Something I think we should try to avoid.
What we are now proposing, is that if "ftrace_enabled" is not set, the
register_ftrace_function() will fail if the ftrace_ops passed to it has
the PERMANENT flag set (which would cause live patching to fail to
load). It also means that if ftrace_enabled was set, and we load a
ftrace_ops with the PERMANENT flag set, and the user tries to clear
ftrace_enabled, that operation will fail. That is, you will not be able
to clear ftrace_enabled if a ftrace_ops is loaded with the PERMANENT
flag set.
You will need to have your live kernel patching user space tooling make
sure that ftrace_enabled is set before loading, but that shouldn't be a
problem.
Does that make sense?
-- Steve
On Wed 2019-10-09 10:26:54, Steven Rostedt wrote:
> Petr Mladek <[email protected]> wrote:
> I think Joe's approach is much easier to understand and implement. The
> "ftrace_enabled" is a global flag, and affects all things ftrace (the
> function bindings). What this patch was doing, was what you said. Make
> ftrace_enabled only affect the ftrace_ops without the "PERMANENT" flag
> set. But that is complex and requires a bit more accounting in the
> ftrace system. Something I think we should try to avoid.
From my POV, the fact that livepatches make use of ftrace handlers
is just an implementation detail. Ideally, users should not know
about it. The livepatch handlers should be handled special way
and should not be affected by the ftrace sysfs interface.
And ideally, the ftrace sysfs interface should still be available
for other ftrace users.
I would understand if this would be too complicated and we would
need to find a compromise.
> What we are now proposing, is that if "ftrace_enabled" is not set, the
> register_ftrace_function() will fail if the ftrace_ops passed to it has
> the PERMANENT flag set (which would cause live patching to fail to
> load).
From the livepatch point of view it would be more reliable if
ftrace_ops with PERMANENT flag would force "ftrace_enabled"
to be always true.
It will make the flag unusable for other ftrace users. But it
will be already be the case when it can't be disabled.
Best Regards,
Petr
On Thu, 10 Oct 2019 10:50:35 +0200
Petr Mladek <[email protected]> wrote:
> It will make the flag unusable for other ftrace users. But it
> will be already be the case when it can't be disabled.
Honestly, I hate that flag. Most people don't even know about it. It
was added in the beginning of ftrace as a way to stop function tracing
in the latency tracer. But that use case has been obsoleted by
328df4759c03e ("tracing: Add function-trace option to disable function
tracing of latency tracers"). I may just remove the damn thing and only
add it back if somebody complains about it.
-- Steve
On Thu, 10 Oct 2019, Steven Rostedt wrote:
> On Thu, 10 Oct 2019 10:50:35 +0200
> Petr Mladek <[email protected]> wrote:
>
> > It will make the flag unusable for other ftrace users. But it
> > will be already be the case when it can't be disabled.
>
> Honestly, I hate that flag. Most people don't even know about it. It
> was added in the beginning of ftrace as a way to stop function tracing
> in the latency tracer. But that use case has been obsoleted by
> 328df4759c03e ("tracing: Add function-trace option to disable function
> tracing of latency tracers"). I may just remove the damn thing and only
> add it back if somebody complains about it.
That would of course solve the issue too and code removal is always
better.
Miroslav
On Thu, 10 Oct 2019 09:43:52 -0400
Steven Rostedt <[email protected]> wrote:
>
> Yes, but let's still add the patch that does the permanent check. And
> then I'll put the "remove this flag" patch on top (and revert
> everything else). This way, if somebody complains, and Linus reverts
> the removal patch, we don't end up breaking live kernel patching
> again ;-)
Not to mention, the PERMANENT flag patch can be marked as stable. The
removal of the switch, not so.
-- Steve
On Thu, 10 Oct 2019 15:38:20 +0200 (CEST)
Miroslav Benes <[email protected]> wrote:
> On Thu, 10 Oct 2019, Steven Rostedt wrote:
>
> > On Thu, 10 Oct 2019 10:50:35 +0200
> > Petr Mladek <[email protected]> wrote:
> >
> > > It will make the flag unusable for other ftrace users. But it
> > > will be already be the case when it can't be disabled.
> >
> > Honestly, I hate that flag. Most people don't even know about it. It
> > was added in the beginning of ftrace as a way to stop function tracing
> > in the latency tracer. But that use case has been obsoleted by
> > 328df4759c03e ("tracing: Add function-trace option to disable function
> > tracing of latency tracers"). I may just remove the damn thing and only
> > add it back if somebody complains about it.
>
> That would of course solve the issue too and code removal is always
> better.
>
Yes, but let's still add the patch that does the permanent check. And
then I'll put the "remove this flag" patch on top (and revert
everything else). This way, if somebody complains, and Linus reverts
the removal patch, we don't end up breaking live kernel patching
again ;-)
-- Steve