* [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag @ 2019-10-07 8:17 Miroslav Benes 2019-10-07 8:17 ` [PATCH 1/3] ftrace: Make test_rec_ops_needs_regs() generic Miroslav Benes ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Miroslav Benes @ 2019-10-07 8:17 UTC (permalink / raw) To: rostedt, mingo, jpoimboe, jikos, pmladek, joe.lawrence Cc: linux-kernel, live-patching, Miroslav Benes 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] ftrace: Make test_rec_ops_needs_regs() generic 2019-10-07 8:17 [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes @ 2019-10-07 8:17 ` Miroslav Benes 2019-10-07 8:17 ` [PATCH 2/3] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes ` (2 subsequent siblings) 3 siblings, 0 replies; 15+ messages in thread From: Miroslav Benes @ 2019-10-07 8:17 UTC (permalink / raw) To: rostedt, mingo, jpoimboe, jikos, pmladek, joe.lawrence Cc: linux-kernel, live-patching, Miroslav Benes Function test_rec_ops_needs_regs() tests whether ftrace_ops registered on a record needs saved regs. That is, it tests for FTRACE_OPS_FL_SAVE_REGS being set. The same logic will be reused for newly introduced FTRACE_OPS_FL_PERMANENT flag, so make the function generic. Signed-off-by: Miroslav Benes <mbenes@suse.cz> --- kernel/trace/ftrace.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 62a50bf399d6..a37c1127599c 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -1602,24 +1602,24 @@ int ftrace_text_reserved(const void *start, const void *end) return (int)!!ret; } -/* Test if ops registered to this rec needs regs */ -static bool test_rec_ops_needs_regs(struct dyn_ftrace *rec) +/* Test if ops registered to this rec needs to have a specified flag set */ +static bool test_rec_ops_needs_flag(struct dyn_ftrace *rec, int flag) { struct ftrace_ops *ops; - bool keep_regs = false; + bool keep_flag = false; for (ops = ftrace_ops_list; ops != &ftrace_list_end; ops = ops->next) { /* pass rec in as regs to have non-NULL val */ if (ftrace_ops_test(ops, rec->ip, rec)) { - if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) { - keep_regs = true; + if (ops->flags & flag) { + keep_flag = true; break; } } } - return keep_regs; + return keep_flag; } static struct ftrace_ops * @@ -1750,7 +1750,8 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, if (ftrace_rec_count(rec) > 0 && rec->flags & FTRACE_FL_REGS && ops->flags & FTRACE_OPS_FL_SAVE_REGS) { - if (!test_rec_ops_needs_regs(rec)) + if (!test_rec_ops_needs_flag(rec, + FTRACE_OPS_FL_SAVE_REGS)) rec->flags &= ~FTRACE_FL_REGS; } -- 2.23.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] ftrace: Introduce PERMANENT ftrace_ops flag 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 ` Miroslav Benes 2019-10-07 8:17 ` [PATCH 3/3] livepatch: Use FTRACE_OPS_FL_PERMANENT Miroslav Benes 2019-10-08 19:35 ` [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Joe Lawrence 3 siblings, 0 replies; 15+ messages in thread From: Miroslav Benes @ 2019-10-07 8:17 UTC (permalink / raw) To: rostedt, mingo, jpoimboe, jikos, pmladek, joe.lawrence Cc: linux-kernel, live-patching, Miroslav Benes Livepatch uses ftrace for redirection to new patched functions. It means that if ftrace is disabled, all live patched functions are disabled as well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. 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. Signed-off-by: Miroslav Benes <mbenes@suse.cz> --- Documentation/trace/ftrace-uses.rst | 6 ++++++ Documentation/trace/ftrace.rst | 2 ++ include/linux/ftrace.h | 8 ++++++-- kernel/trace/ftrace.c | 32 ++++++++++++++++++++++++++++- 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/Documentation/trace/ftrace-uses.rst b/Documentation/trace/ftrace-uses.rst index 1fbc69894eed..e97b52de403f 100644 --- a/Documentation/trace/ftrace-uses.rst +++ b/Documentation/trace/ftrace-uses.rst @@ -170,6 +170,12 @@ FTRACE_OPS_FL_RCU a callback may be executed and RCU synchronization will not protect it. +FTRACE_OPS_FL_PERMANENT + If this is set, then the tracing of the function is not disabled when + the proc sysctl ftrace_enabled is switched off. Livepatch uses it not + to lose the function redirection, so the system stays protected. The + callbacks with the flag set can still be unregistered. + Filtering which functions to trace ================================== diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst index e3060eedb22d..e93987a79477 100644 --- a/Documentation/trace/ftrace.rst +++ b/Documentation/trace/ftrace.rst @@ -2977,6 +2977,8 @@ function tracer. By default it is enabled (when function tracing is enabled in the kernel). If it is disabled, all function tracing is disabled. This includes not only the function tracers for ftrace, but also for any other uses (perf, kprobes, stack tracing, profiling, etc). +Functions, whose ftrace_ops are registered with FTRACE_OPS_FL_PERMANENT set, +are the only exceptions. Tracing stays enabled there. Please disable this with care. diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 8a8cb3c401b2..55f074f248b2 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -142,6 +142,7 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops); * PID - Is affected by set_ftrace_pid (allows filtering on those pids) * RCU - Set when the ops can only be called when RCU is watching. * TRACE_ARRAY - The ops->private points to a trace_array descriptor. + * PERMAMENT - Set when the ops is permanent and should not be removed. */ enum { FTRACE_OPS_FL_ENABLED = 1 << 0, @@ -160,6 +161,7 @@ enum { FTRACE_OPS_FL_PID = 1 << 13, FTRACE_OPS_FL_RCU = 1 << 14, FTRACE_OPS_FL_TRACE_ARRAY = 1 << 15, + FTRACE_OPS_FL_PERMANENT = 1 << 16, }; #ifdef CONFIG_DYNAMIC_FTRACE @@ -330,6 +332,7 @@ bool is_ftrace_trampoline(unsigned long addr); * REGS_EN - the function is set up to save regs. * IPMODIFY - the record allows for the IP address to be changed. * DISABLED - the record is not ready to be touched yet + * PERMANENT - the record is permanent, do not remove it. * * When a new ftrace_ops is registered and wants a function to save * pt_regs, the rec->flag REGS is set. When the function has been @@ -345,10 +348,11 @@ enum { FTRACE_FL_TRAMP_EN = (1UL << 27), FTRACE_FL_IPMODIFY = (1UL << 26), FTRACE_FL_DISABLED = (1UL << 25), + FTRACE_FL_PERMANENT = (1UL << 24), }; -#define FTRACE_REF_MAX_SHIFT 25 -#define FTRACE_FL_BITS 7 +#define FTRACE_REF_MAX_SHIFT 24 +#define FTRACE_FL_BITS 8 #define FTRACE_FL_MASKED_BITS ((1UL << FTRACE_FL_BITS) - 1) #define FTRACE_FL_MASK (FTRACE_FL_MASKED_BITS << FTRACE_REF_MAX_SHIFT) #define FTRACE_REF_MAX ((1UL << FTRACE_REF_MAX_SHIFT) - 1) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index a37c1127599c..790a7c2dd0b4 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -1736,6 +1736,13 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, */ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) rec->flags |= FTRACE_FL_REGS; + + /* + * If any ops is permanent for this function, set it + * for the record. + */ + if (ops->flags & FTRACE_OPS_FL_PERMANENT) + rec->flags |= FTRACE_FL_PERMANENT; } else { if (FTRACE_WARN_ON(ftrace_rec_count(rec) == 0)) return false; @@ -1755,6 +1762,20 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, rec->flags &= ~FTRACE_FL_REGS; } + /* + * If the rec had PERMANENT enabled and the ops that is + * being removed had PERMANENT set, then see if there + * is still any ops for this record that wants to be + * permanent. If not, we can stop recording them. + */ + if (ftrace_rec_count(rec) > 0 && + rec->flags & FTRACE_FL_PERMANENT && + ops->flags & FTRACE_OPS_FL_PERMANENT) { + if (!test_rec_ops_needs_flag(rec, + FTRACE_OPS_FL_PERMANENT)) + rec->flags &= ~FTRACE_FL_PERMANENT; + } + /* * The TRAMP needs to be set only if rec count * is decremented to one, and the ops that is @@ -2032,6 +2053,8 @@ void ftrace_bug(int failed, struct dyn_ftrace *rec) pr_info("ftrace record flags: %lx\n", rec->flags); pr_cont(" (%ld)%s", ftrace_rec_count(rec), rec->flags & FTRACE_FL_REGS ? " R" : " "); + pr_cont(" (%ld)%s", ftrace_rec_count(rec), + rec->flags & FTRACE_FL_PERMANENT ? " P" : " "); if (rec->flags & FTRACE_FL_TRAMP_EN) { ops = ftrace_find_tramp_ops_any(rec); if (ops) { @@ -2129,6 +2152,12 @@ static int ftrace_check_record(struct dyn_ftrace *rec, bool enable, bool update) return FTRACE_UPDATE_MODIFY_CALL; } + /* Do not disable the permanent record */ + if (ftrace_rec_count(rec) && + (rec->flags & FTRACE_FL_PERMANENT)) { + return FTRACE_UPDATE_IGNORE; + } + if (update) { /* If there's no more users, clear all flags */ if (!ftrace_rec_count(rec)) @@ -3450,9 +3479,10 @@ static int t_show(struct seq_file *m, void *v) if (iter->flags & FTRACE_ITER_ENABLED) { struct ftrace_ops *ops; - seq_printf(m, " (%ld)%s%s", + seq_printf(m, " (%ld)%s%s%s", ftrace_rec_count(rec), rec->flags & FTRACE_FL_REGS ? " R" : " ", + rec->flags & FTRACE_FL_PERMANENT ? " P" : " ", rec->flags & FTRACE_FL_IPMODIFY ? " I" : " "); if (rec->flags & FTRACE_FL_TRAMP_EN) { ops = ftrace_find_tramp_ops_any(rec); -- 2.23.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] livepatch: Use FTRACE_OPS_FL_PERMANENT 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 ` Miroslav Benes 2019-10-08 19:35 ` [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Joe Lawrence 3 siblings, 0 replies; 15+ messages in thread From: Miroslav Benes @ 2019-10-07 8:17 UTC (permalink / raw) To: rostedt, mingo, jpoimboe, jikos, pmladek, joe.lawrence Cc: linux-kernel, live-patching, Miroslav Benes Use FTRACE_OPS_FL_PERMANENT flag to be immune to toggling the 'ftrace_enabled' sysctl knob. Signed-off-by: Miroslav Benes <mbenes@suse.cz> --- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-07 8:17 [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes ` (2 preceding siblings ...) 2019-10-07 8:17 ` [PATCH 3/3] livepatch: Use FTRACE_OPS_FL_PERMANENT Miroslav Benes @ 2019-10-08 19:35 ` Joe Lawrence 2019-10-08 19:50 ` Steven Rostedt 2019-10-09 11:22 ` Petr Mladek 3 siblings, 2 replies; 15+ messages in thread From: Joe Lawrence @ 2019-10-08 19:35 UTC (permalink / raw) To: Miroslav Benes Cc: rostedt, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-08 19:35 ` [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Joe Lawrence @ 2019-10-08 19:50 ` Steven Rostedt 2019-10-09 10:33 ` Miroslav Benes 2019-10-09 11:22 ` Petr Mladek 1 sibling, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2019-10-08 19:50 UTC (permalink / raw) To: Joe Lawrence Cc: Miroslav Benes, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching On Tue, 8 Oct 2019 15:35:34 -0400 Joe Lawrence <joe.lawrence@redhat.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-08 19:50 ` Steven Rostedt @ 2019-10-09 10:33 ` Miroslav Benes 0 siblings, 0 replies; 15+ messages in thread From: Miroslav Benes @ 2019-10-09 10:33 UTC (permalink / raw) To: Steven Rostedt Cc: Joe Lawrence, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching On Tue, 8 Oct 2019, Steven Rostedt wrote: > On Tue, 8 Oct 2019 15:35:34 -0400 > Joe Lawrence <joe.lawrence@redhat.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-08 19:35 ` [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Joe Lawrence 2019-10-08 19:50 ` Steven Rostedt @ 2019-10-09 11:22 ` Petr Mladek 2019-10-09 14:23 ` Joe Lawrence 2019-10-09 14:26 ` Steven Rostedt 1 sibling, 2 replies; 15+ messages in thread From: Petr Mladek @ 2019-10-09 11:22 UTC (permalink / raw) To: Joe Lawrence Cc: Miroslav Benes, rostedt, jikos, jpoimboe, mingo, linux-kernel, live-patching 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-09 11:22 ` Petr Mladek @ 2019-10-09 14:23 ` Joe Lawrence 2019-10-09 14:26 ` Steven Rostedt 1 sibling, 0 replies; 15+ messages in thread From: Joe Lawrence @ 2019-10-09 14:23 UTC (permalink / raw) To: Petr Mladek Cc: Miroslav Benes, rostedt, jikos, jpoimboe, mingo, linux-kernel, live-patching 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 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 1 sibling, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2019-10-09 14:26 UTC (permalink / raw) To: Petr Mladek Cc: Joe Lawrence, Miroslav Benes, jikos, jpoimboe, mingo, linux-kernel, live-patching On Wed, 9 Oct 2019 13:22:34 +0200 Petr Mladek <pmladek@suse.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-09 14:26 ` Steven Rostedt @ 2019-10-10 8:50 ` Petr Mladek 2019-10-10 13:14 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Petr Mladek @ 2019-10-10 8:50 UTC (permalink / raw) To: Steven Rostedt Cc: jikos, Joe Lawrence, jpoimboe, mingo, Miroslav Benes, linux-kernel, live-patching On Wed 2019-10-09 10:26:54, Steven Rostedt wrote: > Petr Mladek <pmladek@suse.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-10 8:50 ` Petr Mladek @ 2019-10-10 13:14 ` Steven Rostedt 2019-10-10 13:38 ` Miroslav Benes 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2019-10-10 13:14 UTC (permalink / raw) To: Petr Mladek Cc: jikos, Joe Lawrence, jpoimboe, mingo, Miroslav Benes, linux-kernel, live-patching On Thu, 10 Oct 2019 10:50:35 +0200 Petr Mladek <pmladek@suse.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-10 13:14 ` Steven Rostedt @ 2019-10-10 13:38 ` Miroslav Benes 2019-10-10 13:43 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Miroslav Benes @ 2019-10-10 13:38 UTC (permalink / raw) To: Steven Rostedt Cc: Petr Mladek, jikos, Joe Lawrence, jpoimboe, mingo, linux-kernel, live-patching On Thu, 10 Oct 2019, Steven Rostedt wrote: > On Thu, 10 Oct 2019 10:50:35 +0200 > Petr Mladek <pmladek@suse.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-10 13:38 ` Miroslav Benes @ 2019-10-10 13:43 ` Steven Rostedt 2019-10-10 13:44 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2019-10-10 13:43 UTC (permalink / raw) To: Miroslav Benes Cc: Petr Mladek, jikos, Joe Lawrence, jpoimboe, mingo, linux-kernel, live-patching On Thu, 10 Oct 2019 15:38:20 +0200 (CEST) Miroslav Benes <mbenes@suse.cz> wrote: > On Thu, 10 Oct 2019, Steven Rostedt wrote: > > > On Thu, 10 Oct 2019 10:50:35 +0200 > > Petr Mladek <pmladek@suse.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-10 13:43 ` Steven Rostedt @ 2019-10-10 13:44 ` Steven Rostedt 0 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2019-10-10 13:44 UTC (permalink / raw) To: Miroslav Benes Cc: Petr Mladek, jikos, Joe Lawrence, jpoimboe, mingo, linux-kernel, live-patching On Thu, 10 Oct 2019 09:43:52 -0400 Steven Rostedt <rostedt@goodmis.org> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-10-10 13:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 ` [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag Joe Lawrence 2019-10-08 19:50 ` 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
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).