* [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag @ 2019-10-14 10:59 Miroslav Benes 2019-10-14 14:26 ` Petr Mladek ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Miroslav Benes @ 2019-10-14 10:59 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 on any ftrace ops, the tracing cannot be disabled by disabling ftrace_enabled. Equally, a callback with the flag set cannot be registered if ftrace_enabled is disabled. Signed-off-by: Miroslav Benes <mbenes@suse.cz> --- v1->v2: - different logic, proposed by Joe Lawrence Two things I am not sure about much: - return codes. I chose EBUSY, because it seemed the least inappropriate. I usually pick the wrong one, so suggestions are welcome. - I did not add any pr_* reporting the problem to make it consistent with the existing code. Documentation/trace/ftrace-uses.rst | 8 ++++++++ Documentation/trace/ftrace.rst | 4 +++- include/linux/ftrace.h | 3 +++ kernel/livepatch/patch.c | 3 ++- kernel/trace/ftrace.c | 23 +++++++++++++++++++++-- 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/Documentation/trace/ftrace-uses.rst b/Documentation/trace/ftrace-uses.rst index 1fbc69894eed..740bd0224d35 100644 --- a/Documentation/trace/ftrace-uses.rst +++ b/Documentation/trace/ftrace-uses.rst @@ -170,6 +170,14 @@ FTRACE_OPS_FL_RCU a callback may be executed and RCU synchronization will not protect it. +FTRACE_OPS_FL_PERMANENT + If this is set on any ftrace ops, then the tracing cannot disabled by + writing 0 to the proc sysctl ftrace_enabled. Equally, a callback with + the flag set cannot be registered if ftrace_enabled is 0. + + Livepatch uses it not to lose the function redirection, so the system + stays protected. + Filtering which functions to trace ================================== diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst index e3060eedb22d..d2b5657ed33e 100644 --- a/Documentation/trace/ftrace.rst +++ b/Documentation/trace/ftrace.rst @@ -2976,7 +2976,9 @@ Note, the proc sysctl ftrace_enable is a big on/off switch for the 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). +also for any other uses (perf, kprobes, stack tracing, profiling, etc). It +cannot be disabled if there is a callback with FTRACE_OPS_FL_PERMANENT set +registered. Please disable this with care. diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 8a8cb3c401b2..c2cad29dc557 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -142,6 +142,8 @@ 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 affected by + * ftrace_enabled. */ enum { FTRACE_OPS_FL_ENABLED = 1 << 0, @@ -160,6 +162,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 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); diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 62a50bf399d6..d2992ea29fe1 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -325,6 +325,8 @@ int __register_ftrace_function(struct ftrace_ops *ops) if (ops->flags & FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED) ops->flags |= FTRACE_OPS_FL_SAVE_REGS; #endif + if (!ftrace_enabled && (ops->flags & FTRACE_OPS_FL_PERMANENT)) + return -EBUSY; if (!core_kernel_data((unsigned long)ops)) ops->flags |= FTRACE_OPS_FL_DYNAMIC; @@ -6723,6 +6725,18 @@ int unregister_ftrace_function(struct ftrace_ops *ops) } EXPORT_SYMBOL_GPL(unregister_ftrace_function); +static bool is_permanent_ops_registered(void) +{ + struct ftrace_ops *op; + + do_for_each_ftrace_op(op, ftrace_ops_list) { + if (op->flags & FTRACE_OPS_FL_PERMANENT) + return true; + } while_for_each_ftrace_op(op); + + return false; +} + int ftrace_enable_sysctl(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) goto out; - last_ftrace_enabled = !!ftrace_enabled; - if (ftrace_enabled) { /* we are starting ftrace again */ @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, ftrace_startup_sysctl(); } else { + if (is_permanent_ops_registered()) { + ftrace_enabled = last_ftrace_enabled; + ret = -EBUSY; + goto out; + } + /* stopping ftrace calls (just send to ftrace_stub) */ ftrace_trace_function = ftrace_stub; ftrace_shutdown_sysctl(); } + last_ftrace_enabled = !!ftrace_enabled; out: mutex_unlock(&ftrace_lock); return ret; -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-14 10:59 [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes @ 2019-10-14 14:26 ` Petr Mladek 2019-10-14 15:17 ` Steven Rostedt ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Petr Mladek @ 2019-10-14 14:26 UTC (permalink / raw) To: Miroslav Benes Cc: rostedt, jikos, joe.lawrence, jpoimboe, mingo, linux-kernel, live-patching On Mon 2019-10-14 12:59:23, Miroslav Benes wrote: > 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 on any ftrace ops, the tracing cannot be > disabled by disabling ftrace_enabled. Equally, a callback with the flag > set cannot be registered if ftrace_enabled is disabled. > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> Looks fine to me. I finally understand which ftrace_enabled toggle we are talking about ;-) Reviewed-by: Petr Mladek <pmladek@suse.com> > --- > - return codes. I chose EBUSY, because it seemed the least > inappropriate. I usually pick the wrong one, so suggestions are > welcome. -EBUSY is perfectly fine in ftrace_enable_sysctl(). It is not ideal in __register_ftrace_function(). But it still looks better than -ENODEV there. Best Regards, Petr ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-14 10:59 [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes 2019-10-14 14:26 ` Petr Mladek @ 2019-10-14 15:17 ` Steven Rostedt 2019-10-15 7:45 ` Petr Mladek 2019-10-14 22:31 ` Joe Lawrence 2019-10-16 5:02 ` Kamalesh Babulal 3 siblings, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2019-10-14 15:17 UTC (permalink / raw) To: Miroslav Benes Cc: mingo, jpoimboe, jikos, pmladek, joe.lawrence, linux-kernel, live-patching On Mon, 14 Oct 2019 12:59:23 +0200 Miroslav Benes <mbenes@suse.cz> wrote: > int > ftrace_enable_sysctl(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, > @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) > goto out; > > - last_ftrace_enabled = !!ftrace_enabled; > - > if (ftrace_enabled) { > > /* we are starting ftrace again */ > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > ftrace_startup_sysctl(); > > } else { > + if (is_permanent_ops_registered()) { > + ftrace_enabled = last_ftrace_enabled; Although this is not incorrect, but may be somewhat confusing. At this location, last_ftrace_enabled is always true. I'm thinking this would be better to simply set it to false here. > + ret = -EBUSY; > + goto out; > + } > + > /* stopping ftrace calls (just send to ftrace_stub) */ > ftrace_trace_function = ftrace_stub; > > ftrace_shutdown_sysctl(); > } > > + last_ftrace_enabled = !!ftrace_enabled; > out: And move the assignment of last_ftrace_enabled to after the "out:" label. -- Steve > mutex_unlock(&ftrace_lock); > return ret; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-14 15:17 ` Steven Rostedt @ 2019-10-15 7:45 ` Petr Mladek 2019-10-15 10:50 ` Miroslav Benes 0 siblings, 1 reply; 12+ messages in thread From: Petr Mladek @ 2019-10-15 7:45 UTC (permalink / raw) To: Steven Rostedt Cc: Miroslav Benes, jikos, joe.lawrence, jpoimboe, mingo, linux-kernel, live-patching On Mon 2019-10-14 11:17:19, Steven Rostedt wrote: > On Mon, 14 Oct 2019 12:59:23 +0200 > Miroslav Benes <mbenes@suse.cz> wrote: > > > int > > ftrace_enable_sysctl(struct ctl_table *table, int write, > > void __user *buffer, size_t *lenp, > > @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > > if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) > > goto out; > > > > - last_ftrace_enabled = !!ftrace_enabled; > > - > > if (ftrace_enabled) { > > > > /* we are starting ftrace again */ > > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > > ftrace_startup_sysctl(); > > > > } else { > > + if (is_permanent_ops_registered()) { > > + ftrace_enabled = last_ftrace_enabled; > > Although this is not incorrect, but may be somewhat confusing. > > At this location, last_ftrace_enabled is always true. > > I'm thinking this would be better to simply set it to false here. IMHO, we want to set ftrace_enabled = true here. It was set to "false" by writing to the sysfs file. But the change gets rejected. ftrace will stay enabled. So, we should set the value back to "true". > > + ret = -EBUSY; > > + goto out; > > + } > > + > > /* stopping ftrace calls (just send to ftrace_stub) */ > > ftrace_trace_function = ftrace_stub; > > > > ftrace_shutdown_sysctl(); > > } > > > > + last_ftrace_enabled = !!ftrace_enabled; > > out: > > And move the assignment of last_ftrace_enabled to after the "out:" > label. This change might make sense anyway. But it is not strictly necessary from my POV. Best Regards, Petr ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-15 7:45 ` Petr Mladek @ 2019-10-15 10:50 ` Miroslav Benes 2019-10-15 13:36 ` Steven Rostedt 0 siblings, 1 reply; 12+ messages in thread From: Miroslav Benes @ 2019-10-15 10:50 UTC (permalink / raw) To: Petr Mladek Cc: Steven Rostedt, jikos, joe.lawrence, jpoimboe, mingo, linux-kernel, live-patching On Tue, 15 Oct 2019, Petr Mladek wrote: > On Mon 2019-10-14 11:17:19, Steven Rostedt wrote: > > On Mon, 14 Oct 2019 12:59:23 +0200 > > Miroslav Benes <mbenes@suse.cz> wrote: > > > > > int > > > ftrace_enable_sysctl(struct ctl_table *table, int write, > > > void __user *buffer, size_t *lenp, > > > @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > > > if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) > > > goto out; > > > > > > - last_ftrace_enabled = !!ftrace_enabled; > > > - > > > if (ftrace_enabled) { > > > > > > /* we are starting ftrace again */ > > > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > > > ftrace_startup_sysctl(); > > > > > > } else { > > > + if (is_permanent_ops_registered()) { > > > + ftrace_enabled = last_ftrace_enabled; > > > > Although this is not incorrect, but may be somewhat confusing. > > > > At this location, last_ftrace_enabled is always true. > > > > I'm thinking this would be better to simply set it to false here. > > IMHO, we want to set ftrace_enabled = true here. > > It was set to "false" by writing to the sysfs file. But the change > gets rejected. ftrace will stay enabled. So, we should set > the value back to "true". That's correct. I can make it explicit as proposed. I just thought that 'ftrace_enabled = last_ftrace_enabled' was clear enough given Petr's explanation. > > > + ret = -EBUSY; > > > + goto out; > > > + } > > > + > > > /* stopping ftrace calls (just send to ftrace_stub) */ > > > ftrace_trace_function = ftrace_stub; > > > > > > ftrace_shutdown_sysctl(); > > > } > > > > > > + last_ftrace_enabled = !!ftrace_enabled; > > > out: > > > > And move the assignment of last_ftrace_enabled to after the "out:" > > label. > > This change might make sense anyway. But it is not strictly necessary > from my POV. If it stays before "out:" label, last_ftrace_enabled is set if and only if it has to be set. I think it is better, but I can, of course, move it in v3 if Steven prefers it. Thanks Miroslav ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-15 10:50 ` Miroslav Benes @ 2019-10-15 13:36 ` Steven Rostedt 0 siblings, 0 replies; 12+ messages in thread From: Steven Rostedt @ 2019-10-15 13:36 UTC (permalink / raw) To: Miroslav Benes Cc: Petr Mladek, jikos, joe.lawrence, jpoimboe, mingo, linux-kernel, live-patching On Tue, 15 Oct 2019 12:50:59 +0200 (CEST) Miroslav Benes <mbenes@suse.cz> wrote: > > > > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > > > > ftrace_startup_sysctl(); > > > > > > > > } else { > > > > + if (is_permanent_ops_registered()) { > > > > + ftrace_enabled = last_ftrace_enabled; > > > > > > Although this is not incorrect, but may be somewhat confusing. > > > > > > At this location, last_ftrace_enabled is always true. > > > > > > I'm thinking this would be better to simply set it to false here. > > > > IMHO, we want to set ftrace_enabled = true here. Yes, I meant true (don't know why I said false :-/ ) > > > > It was set to "false" by writing to the sysfs file. But the change > > gets rejected. ftrace will stay enabled. So, we should set > > the value back to "true". > > That's correct. > > I can make it explicit as proposed. I just thought that 'ftrace_enabled = > last_ftrace_enabled' was clear enough given Petr's explanation. > > > > > + ret = -EBUSY; > > > > + goto out; > > > > + } > > > > + > > > > /* stopping ftrace calls (just send to ftrace_stub) */ > > > > ftrace_trace_function = ftrace_stub; > > > > > > > > ftrace_shutdown_sysctl(); > > > > } > > > > > > > > + last_ftrace_enabled = !!ftrace_enabled; > > > > out: > > > > > > And move the assignment of last_ftrace_enabled to after the "out:" > > > label. > > > > This change might make sense anyway. But it is not strictly necessary > > from my POV. > > If it stays before "out:" label, last_ftrace_enabled is set if and only if > it has to be set. I think it is better, but I can, of course, move it in > v3 if Steven prefers it. I don't have any strong feelings here. If you want to keep it like this, I wont argue. -- Steve ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-14 10:59 [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes 2019-10-14 14:26 ` Petr Mladek 2019-10-14 15:17 ` Steven Rostedt @ 2019-10-14 22:31 ` Joe Lawrence 2019-10-15 11:23 ` Miroslav Benes 2019-10-16 5:02 ` Kamalesh Babulal 3 siblings, 1 reply; 12+ messages in thread From: Joe Lawrence @ 2019-10-14 22:31 UTC (permalink / raw) To: Miroslav Benes Cc: rostedt, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching On Mon, Oct 14, 2019 at 12:59:23PM +0200, Miroslav Benes wrote: > 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 on any ftrace ops, the tracing cannot be > disabled by disabling ftrace_enabled. Equally, a callback with the flag > set cannot be registered if ftrace_enabled is disabled. > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > --- > v1->v2: > - different logic, proposed by Joe Lawrence > > Two things I am not sure about much: > > - return codes. I chose EBUSY, because it seemed the least > inappropriate. I usually pick the wrong one, so suggestions are > welcome. > - I did not add any pr_* reporting the problem to make it consistent > with the existing code. > > Documentation/trace/ftrace-uses.rst | 8 ++++++++ > Documentation/trace/ftrace.rst | 4 +++- > include/linux/ftrace.h | 3 +++ > kernel/livepatch/patch.c | 3 ++- > kernel/trace/ftrace.c | 23 +++++++++++++++++++++-- > 5 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/Documentation/trace/ftrace-uses.rst b/Documentation/trace/ftrace-uses.rst > index 1fbc69894eed..740bd0224d35 100644 > --- a/Documentation/trace/ftrace-uses.rst > +++ b/Documentation/trace/ftrace-uses.rst > @@ -170,6 +170,14 @@ FTRACE_OPS_FL_RCU > a callback may be executed and RCU synchronization will not protect > it. > > +FTRACE_OPS_FL_PERMANENT > + If this is set on any ftrace ops, then the tracing cannot disabled by > + writing 0 to the proc sysctl ftrace_enabled. Equally, a callback with > + the flag set cannot be registered if ftrace_enabled is 0. > + > + Livepatch uses it not to lose the function redirection, so the system > + stays protected. > + > > Filtering which functions to trace > ================================== > diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst > index e3060eedb22d..d2b5657ed33e 100644 > --- a/Documentation/trace/ftrace.rst > +++ b/Documentation/trace/ftrace.rst > @@ -2976,7 +2976,9 @@ Note, the proc sysctl ftrace_enable is a big on/off switch for the > 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). > +also for any other uses (perf, kprobes, stack tracing, profiling, etc). It > +cannot be disabled if there is a callback with FTRACE_OPS_FL_PERMANENT set > +registered. > > Please disable this with care. > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 8a8cb3c401b2..c2cad29dc557 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -142,6 +142,8 @@ 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 affected by > + * ftrace_enabled. > */ > enum { > FTRACE_OPS_FL_ENABLED = 1 << 0, > @@ -160,6 +162,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 > 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); > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 62a50bf399d6..d2992ea29fe1 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -325,6 +325,8 @@ int __register_ftrace_function(struct ftrace_ops *ops) > if (ops->flags & FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED) > ops->flags |= FTRACE_OPS_FL_SAVE_REGS; > #endif > + if (!ftrace_enabled && (ops->flags & FTRACE_OPS_FL_PERMANENT)) > + return -EBUSY; > > if (!core_kernel_data((unsigned long)ops)) > ops->flags |= FTRACE_OPS_FL_DYNAMIC; > @@ -6723,6 +6725,18 @@ int unregister_ftrace_function(struct ftrace_ops *ops) > } > EXPORT_SYMBOL_GPL(unregister_ftrace_function); > > +static bool is_permanent_ops_registered(void) > +{ > + struct ftrace_ops *op; > + > + do_for_each_ftrace_op(op, ftrace_ops_list) { > + if (op->flags & FTRACE_OPS_FL_PERMANENT) > + return true; > + } while_for_each_ftrace_op(op); > + > + return false; > +} > + > int > ftrace_enable_sysctl(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, > @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) > goto out; > > - last_ftrace_enabled = !!ftrace_enabled; > - > if (ftrace_enabled) { > > /* we are starting ftrace again */ > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > ftrace_startup_sysctl(); > > } else { > + if (is_permanent_ops_registered()) { > + ftrace_enabled = last_ftrace_enabled; > + ret = -EBUSY; > + goto out; > + } > + > /* stopping ftrace calls (just send to ftrace_stub) */ > ftrace_trace_function = ftrace_stub; > > ftrace_shutdown_sysctl(); > } > > + last_ftrace_enabled = !!ftrace_enabled; > out: > mutex_unlock(&ftrace_lock); > return ret; > -- > 2.23.0 > Hi Miroslav, Maybe we should add a test to verify this new behavior? See sample version below (lightly tested). We can add to this one, or patch seperately if you prefer. -- Joe -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001 From: Joe Lawrence <joe.lawrence@redhat.com> Date: Mon, 14 Oct 2019 18:25:01 -0400 Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled Since livepatching depends upon ftrace handlers to implement "patched" functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> --- tools/testing/selftests/livepatch/Makefile | 3 +- .../testing/selftests/livepatch/functions.sh | 18 +++++ .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile index fd405402c3ff..1886d9d94b88 100644 --- a/tools/testing/selftests/livepatch/Makefile +++ b/tools/testing/selftests/livepatch/Makefile @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh TEST_PROGS := \ test-livepatch.sh \ test-callbacks.sh \ - test-shadow-vars.sh + test-shadow-vars.sh \ + test-ftrace.sh include ../lib.mk diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index 79b0affd21fb..556252efece0 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -52,6 +52,24 @@ function set_dynamic_debug() { EOF } +function push_ftrace_enabled() { + FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled) +} +function pop_ftrace_enabled() { + if [[ -n "$FTRACE_ENABLED" ]]; then + sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" + fi +} +# set_ftrace_enabled() - save the current ftrace_enabled config and tweak +# it for the self-tests. Set a script exit trap +# that restores the original value. +function set_ftrace_enabled() { + local sysctl="$1" + trap pop_ftrace_enabled EXIT INT TERM HUP + result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ') + echo "livepatch: $result" > /dev/kmsg +} + # loop_until(cmd) - loop a command until it is successful or $MAX_RETRIES, # sleep $RETRY_INTERVAL between attempts # cmd - command and its arguments to run diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index 000000000000..016576883a33 --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh @@ -0,0 +1,65 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 Joe Lawrence <joe.lawrence@redhat.com> + +. $(dirname $0)/functions.sh + +MOD_LIVEPATCH=test_klp_livepatch + +set_dynamic_debug + + +# TEST: livepatch interaction with ftrace_enabled sysctl +# - turn ftrace_enabled OFF and verify livepatches can't load +# - turn ftrace_enabled ON and verify livepatch can load +# - verify that ftrace_enabled can't be turned OFF while a livepatch is loaded + +echo -n "TEST: livepatch interaction with ftrace_enabled sysctl ... " +dmesg -C + +set_ftrace_enabled 0 +load_failing_mod $MOD_LIVEPATCH + +set_ftrace_enabled 1 +load_lp $MOD_LIVEPATCH +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then + echo -e "FAIL\n\n" + die "livepatch kselftest(s) failed" +fi + +set_ftrace_enabled 0 +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then + echo -e "FAIL\n\n" + die "livepatch kselftest(s) failed" +fi +disable_lp $MOD_LIVEPATCH +unload_lp $MOD_LIVEPATCH + +check_result "livepatch: kernel.ftrace_enabled = 0 +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16) +livepatch: failed to patch object 'vmlinux' +livepatch: failed to enable patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': canceling patching transition, going to unpatch +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +modprobe: ERROR: could not insert '$MOD_LIVEPATCH': Device or resource busy +livepatch: kernel.ftrace_enabled = 1 +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: '$MOD_LIVEPATCH': starting patching transition +livepatch: '$MOD_LIVEPATCH': completing patching transition +livepatch: '$MOD_LIVEPATCH': patching complete +livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource busy kernel.ftrace_enabled = 0 +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition +livepatch: '$MOD_LIVEPATCH': starting unpatching transition +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +% rmmod $MOD_LIVEPATCH" + + +exit 0 -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-14 22:31 ` Joe Lawrence @ 2019-10-15 11:23 ` Miroslav Benes 2019-10-15 13:25 ` Joe Lawrence 2019-10-15 14:58 ` Joe Lawrence 0 siblings, 2 replies; 12+ messages in thread From: Miroslav Benes @ 2019-10-15 11:23 UTC (permalink / raw) To: Joe Lawrence Cc: rostedt, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching > Hi Miroslav, > > Maybe we should add a test to verify this new behavior? See sample > version below (lightly tested). We can add to this one, or patch > seperately if you prefer. Thanks a lot, Joe. It looks nice. I'll include it in v3. One question below. > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- > > > >From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001 > From: Joe Lawrence <joe.lawrence@redhat.com> > Date: Mon, 14 Oct 2019 18:25:01 -0400 > Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled > > Since livepatching depends upon ftrace handlers to implement "patched" > functionality, verify that the ftrace_enabled sysctl value interacts > with livepatch registration as expected. > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> > --- > tools/testing/selftests/livepatch/Makefile | 3 +- > .../testing/selftests/livepatch/functions.sh | 18 +++++ > .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ > 3 files changed, 85 insertions(+), 1 deletion(-) > create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh > > diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile > index fd405402c3ff..1886d9d94b88 100644 > --- a/tools/testing/selftests/livepatch/Makefile > +++ b/tools/testing/selftests/livepatch/Makefile > @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh > TEST_PROGS := \ > test-livepatch.sh \ > test-callbacks.sh \ > - test-shadow-vars.sh > + test-shadow-vars.sh \ > + test-ftrace.sh > > include ../lib.mk > diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh > index 79b0affd21fb..556252efece0 100644 > --- a/tools/testing/selftests/livepatch/functions.sh > +++ b/tools/testing/selftests/livepatch/functions.sh > @@ -52,6 +52,24 @@ function set_dynamic_debug() { > EOF > } > > +function push_ftrace_enabled() { > + FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled) > +} Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the test script? set_dynamic_debug() calls its push_dynamic_debug() directly, but set_ftrace_enabled() is different, because it is called more than once in the script. One could argue that ftrace_enabled has to be true at the beginning of testing anyway, but I think it would be cleaner. Btw, we should probably guarantee that ftrace_enabled is true when livepatch selftests are invoked. > +function pop_ftrace_enabled() { > + if [[ -n "$FTRACE_ENABLED" ]]; then > + sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" > + fi > +} > +# set_ftrace_enabled() - save the current ftrace_enabled config and tweak > +# it for the self-tests. Set a script exit trap > +# that restores the original value. > +function set_ftrace_enabled() { > + local sysctl="$1" > + trap pop_ftrace_enabled EXIT INT TERM HUP > + result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ') > + echo "livepatch: $result" > /dev/kmsg > +} > + > # loop_until(cmd) - loop a command until it is successful or $MAX_RETRIES, > # sleep $RETRY_INTERVAL between attempts > # cmd - command and its arguments to run Miroslav ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-15 11:23 ` Miroslav Benes @ 2019-10-15 13:25 ` Joe Lawrence 2019-10-15 14:02 ` Miroslav Benes 2019-10-15 14:58 ` Joe Lawrence 1 sibling, 1 reply; 12+ messages in thread From: Joe Lawrence @ 2019-10-15 13:25 UTC (permalink / raw) To: Miroslav Benes Cc: rostedt, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching On 10/15/19 7:23 AM, Miroslav Benes wrote: >> Hi Miroslav, >> >> Maybe we should add a test to verify this new behavior? See sample >> version below (lightly tested). We can add to this one, or patch >> seperately if you prefer. > > Thanks a lot, Joe. It looks nice. I'll include it in v3. One question > below. > >> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- >> >> >> >From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001 >> From: Joe Lawrence <joe.lawrence@redhat.com> >> Date: Mon, 14 Oct 2019 18:25:01 -0400 >> Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled >> >> Since livepatching depends upon ftrace handlers to implement "patched" >> functionality, verify that the ftrace_enabled sysctl value interacts >> with livepatch registration as expected. >> >> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> >> --- >> tools/testing/selftests/livepatch/Makefile | 3 +- >> .../testing/selftests/livepatch/functions.sh | 18 +++++ >> .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ >> 3 files changed, 85 insertions(+), 1 deletion(-) >> create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh >> >> diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile >> index fd405402c3ff..1886d9d94b88 100644 >> --- a/tools/testing/selftests/livepatch/Makefile >> +++ b/tools/testing/selftests/livepatch/Makefile >> @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh >> TEST_PROGS := \ >> test-livepatch.sh \ >> test-callbacks.sh \ >> - test-shadow-vars.sh >> + test-shadow-vars.sh \ >> + test-ftrace.sh >> >> include ../lib.mk >> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh >> index 79b0affd21fb..556252efece0 100644 >> --- a/tools/testing/selftests/livepatch/functions.sh >> +++ b/tools/testing/selftests/livepatch/functions.sh >> @@ -52,6 +52,24 @@ function set_dynamic_debug() { >> EOF >> } >> >> +function push_ftrace_enabled() { >> + FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled) >> +} > > Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the > test script? set_dynamic_debug() calls its push_dynamic_debug() directly, > but set_ftrace_enabled() is different, because it is called more than once > in the script. > > One could argue that ftrace_enabled has to be true at the beginning of > testing anyway, but I think it would be cleaner. Btw, we should probably > guarantee that ftrace_enabled is true when livepatch selftests are > invoked. > Ah yes, that occurred to me while creating that piece of the patch. Something like setup_test_config() that pushes both ftrace and the debugfs, etc. would be cleaner for all scripts. If you're onboard with that idea, I can make that revision. -- Joe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-15 13:25 ` Joe Lawrence @ 2019-10-15 14:02 ` Miroslav Benes 0 siblings, 0 replies; 12+ messages in thread From: Miroslav Benes @ 2019-10-15 14:02 UTC (permalink / raw) To: Joe Lawrence Cc: rostedt, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching On Tue, 15 Oct 2019, Joe Lawrence wrote: > On 10/15/19 7:23 AM, Miroslav Benes wrote: > >> Hi Miroslav, > >> > >> Maybe we should add a test to verify this new behavior? See sample > >> version below (lightly tested). We can add to this one, or patch > >> seperately if you prefer. > > > > Thanks a lot, Joe. It looks nice. I'll include it in v3. One question > > below. > > > >> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- > >> > >> > >> >From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001 > >> From: Joe Lawrence <joe.lawrence@redhat.com> > >> Date: Mon, 14 Oct 2019 18:25:01 -0400 > >> Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled > >> > >> Since livepatching depends upon ftrace handlers to implement "patched" > >> functionality, verify that the ftrace_enabled sysctl value interacts > >> with livepatch registration as expected. > >> > >> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> > >> --- > >> tools/testing/selftests/livepatch/Makefile | 3 +- > >> .../testing/selftests/livepatch/functions.sh | 18 +++++ > >> .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ > >> 3 files changed, 85 insertions(+), 1 deletion(-) > >> create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh > >> > >> diff --git a/tools/testing/selftests/livepatch/Makefile > >> b/tools/testing/selftests/livepatch/Makefile > >> index fd405402c3ff..1886d9d94b88 100644 > >> --- a/tools/testing/selftests/livepatch/Makefile > >> +++ b/tools/testing/selftests/livepatch/Makefile > >> @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh > >> TEST_PROGS := \ > >> test-livepatch.sh \ > >> test-callbacks.sh \ > >> - test-shadow-vars.sh > >> + test-shadow-vars.sh \ > >> + test-ftrace.sh > >> > >> include ../lib.mk > >> diff --git a/tools/testing/selftests/livepatch/functions.sh > >> b/tools/testing/selftests/livepatch/functions.sh > >> index 79b0affd21fb..556252efece0 100644 > >> --- a/tools/testing/selftests/livepatch/functions.sh > >> +++ b/tools/testing/selftests/livepatch/functions.sh > >> @@ -52,6 +52,24 @@ function set_dynamic_debug() { > >> EOF > >> } > >> > >> +function push_ftrace_enabled() { > >> + FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled) > >> +} > > > > Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the > > test script? set_dynamic_debug() calls its push_dynamic_debug() directly, > > but set_ftrace_enabled() is different, because it is called more than once > > in the script. > > > > One could argue that ftrace_enabled has to be true at the beginning of > > testing anyway, but I think it would be cleaner. Btw, we should probably > > guarantee that ftrace_enabled is true when livepatch selftests are > > invoked. > > > > Ah yes, that occurred to me while creating that piece of the patch. Something > like setup_test_config() that pushes both ftrace and the debugfs, etc. would > be cleaner for all scripts. If you're onboard with that idea, I can make that > revision. Yes, that would be perfect. Thanks. Miroslav ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-15 11:23 ` Miroslav Benes 2019-10-15 13:25 ` Joe Lawrence @ 2019-10-15 14:58 ` Joe Lawrence 1 sibling, 0 replies; 12+ messages in thread From: Joe Lawrence @ 2019-10-15 14:58 UTC (permalink / raw) To: Miroslav Benes Cc: rostedt, mingo, jpoimboe, jikos, pmladek, linux-kernel, live-patching On Tue, Oct 15, 2019 at 01:23:10PM +0200, Miroslav Benes wrote: > > Hi Miroslav, > > > > Maybe we should add a test to verify this new behavior? See sample > > version below (lightly tested). We can add to this one, or patch > > seperately if you prefer. > > Thanks a lot, Joe. It looks nice. I'll include it in v3. One question > below. > > [ ... snip ... ] > > Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the > test script? set_dynamic_debug() calls its push_dynamic_debug() directly, > but set_ftrace_enabled() is different, because it is called more than once > in the script. > > One could argue that ftrace_enabled has to be true at the beginning of > testing anyway, but I think it would be cleaner. Btw, we should probably > guarantee that ftrace_enabled is true when livepatch selftests are > invoked. > Here's a new version that combines the ftrace_enabled configuration with the debugfs stuff (so its only pushed/pop once per test) and updates all the tests accordingly. Feel free to any tweaks or flourishes when adding to v3. Thanks, -- Joe -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- From a22ca55b3f429b7c9ceed6be87a571f77520994c Mon Sep 17 00:00:00 2001 From: Joe Lawrence <joe.lawrence@redhat.com> Date: Tue, 15 Oct 2019 10:33:18 -0400 Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests. Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> --- tools/testing/selftests/livepatch/Makefile | 3 +- .../testing/selftests/livepatch/functions.sh | 34 +++++++--- .../selftests/livepatch/test-callbacks.sh | 2 +- .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ .../selftests/livepatch/test-livepatch.sh | 2 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +- 6 files changed, 95 insertions(+), 13 deletions(-) create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile index fd405402c3ff..1886d9d94b88 100644 --- a/tools/testing/selftests/livepatch/Makefile +++ b/tools/testing/selftests/livepatch/Makefile @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh TEST_PROGS := \ test-livepatch.sh \ test-callbacks.sh \ - test-shadow-vars.sh + test-shadow-vars.sh \ + test-ftrace.sh include ../lib.mk diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index 79b0affd21fb..31eb09e38729 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -29,29 +29,45 @@ function die() { exit 1 } -function push_dynamic_debug() { - DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \ - awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}') +function push_config() { + DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \ + awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}') + FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled) } -function pop_dynamic_debug() { +function pop_config() { if [[ -n "$DYNAMIC_DEBUG" ]]; then echo -n "$DYNAMIC_DEBUG" > /sys/kernel/debug/dynamic_debug/control fi + if [[ -n "$FTRACE_ENABLED" ]]; then + sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" &> /dev/null + fi } -# set_dynamic_debug() - save the current dynamic debug config and tweak -# it for the self-tests. Set a script exit trap -# that restores the original config. function set_dynamic_debug() { - push_dynamic_debug - trap pop_dynamic_debug EXIT INT TERM HUP cat <<-EOF > /sys/kernel/debug/dynamic_debug/control file kernel/livepatch/* +p func klp_try_switch_task -p EOF } +function set_ftrace_enabled() { + local sysctl="$1" + result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ') + echo "livepatch: $result" > /dev/kmsg +} + +# setup_config - save the current config and set a script exit trap that +# restores the original config. Setup the dynamic debug +# for verbose livepatching output and turn on +# the ftrace_enabled sysctl. +function setup_config() { + push_config + set_dynamic_debug + set_ftrace_enabled 1 + trap pop_config EXIT INT TERM HUP +} + # loop_until(cmd) - loop a command until it is successful or $MAX_RETRIES, # sleep $RETRY_INTERVAL between attempts # cmd - command and its arguments to run diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh index e97a9dcb73c7..a35289b13c9c 100755 --- a/tools/testing/selftests/livepatch/test-callbacks.sh +++ b/tools/testing/selftests/livepatch/test-callbacks.sh @@ -9,7 +9,7 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2 MOD_TARGET=test_klp_callbacks_mod MOD_TARGET_BUSY=test_klp_callbacks_busy -set_dynamic_debug +setup_config # TEST: target module before livepatch diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index 000000000000..e2a76887f40a --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh @@ -0,0 +1,65 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 Joe Lawrence <joe.lawrence@redhat.com> + +. $(dirname $0)/functions.sh + +MOD_LIVEPATCH=test_klp_livepatch + +setup_config + + +# TEST: livepatch interaction with ftrace_enabled sysctl +# - turn ftrace_enabled OFF and verify livepatches can't load +# - turn ftrace_enabled ON and verify livepatch can load +# - verify that ftrace_enabled can't be turned OFF while a livepatch is loaded + +echo -n "TEST: livepatch interaction with ftrace_enabled sysctl ... " +dmesg -C + +set_ftrace_enabled 0 +load_failing_mod $MOD_LIVEPATCH + +set_ftrace_enabled 1 +load_lp $MOD_LIVEPATCH +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then + echo -e "FAIL\n\n" + die "livepatch kselftest(s) failed" +fi + +set_ftrace_enabled 0 +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then + echo -e "FAIL\n\n" + die "livepatch kselftest(s) failed" +fi +disable_lp $MOD_LIVEPATCH +unload_lp $MOD_LIVEPATCH + +check_result "livepatch: kernel.ftrace_enabled = 0 +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16) +livepatch: failed to patch object 'vmlinux' +livepatch: failed to enable patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': canceling patching transition, going to unpatch +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +modprobe: ERROR: could not insert '$MOD_LIVEPATCH': Device or resource busy +livepatch: kernel.ftrace_enabled = 1 +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: '$MOD_LIVEPATCH': starting patching transition +livepatch: '$MOD_LIVEPATCH': completing patching transition +livepatch: '$MOD_LIVEPATCH': patching complete +livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource busy kernel.ftrace_enabled = 0 +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition +livepatch: '$MOD_LIVEPATCH': starting unpatching transition +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +% rmmod $MOD_LIVEPATCH" + + +exit 0 diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh index f05268aea859..493e3df415a1 100755 --- a/tools/testing/selftests/livepatch/test-livepatch.sh +++ b/tools/testing/selftests/livepatch/test-livepatch.sh @@ -7,7 +7,7 @@ MOD_LIVEPATCH=test_klp_livepatch MOD_REPLACE=test_klp_atomic_replace -set_dynamic_debug +setup_config # TEST: basic function patching diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh index 04a37831e204..1aae73299114 100755 --- a/tools/testing/selftests/livepatch/test-shadow-vars.sh +++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh @@ -6,7 +6,7 @@ MOD_TEST=test_klp_shadow_vars -set_dynamic_debug +setup_config # TEST: basic shadow variable API -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag 2019-10-14 10:59 [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes ` (2 preceding siblings ...) 2019-10-14 22:31 ` Joe Lawrence @ 2019-10-16 5:02 ` Kamalesh Babulal 3 siblings, 0 replies; 12+ messages in thread From: Kamalesh Babulal @ 2019-10-16 5:02 UTC (permalink / raw) To: Miroslav Benes, rostedt, mingo, jpoimboe, jikos, pmladek, joe.lawrence Cc: linux-kernel, live-patching On 10/14/19 4:29 PM, Miroslav Benes wrote: > 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 on any ftrace ops, the tracing cannot be > disabled by disabling ftrace_enabled. Equally, a callback with the flag > set cannot be registered if ftrace_enabled is disabled. > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> The patch looks good to me. A minor typo in flag description below. Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> [...] > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 8a8cb3c401b2..c2cad29dc557 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -142,6 +142,8 @@ 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 affected by > + * ftrace_enabled. > */ s/PERMAMENT/PERMANENT > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 62a50bf399d6..d2992ea29fe1 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > ftrace_startup_sysctl(); > > } else { > + if (is_permanent_ops_registered()) { > + ftrace_enabled = last_ftrace_enabled; > + ret = -EBUSY; > + goto out; > + } > + > /* stopping ftrace calls (just send to ftrace_stub) */ > ftrace_trace_function = ftrace_stub; > > ftrace_shutdown_sysctl(); > } > > + last_ftrace_enabled = !!ftrace_enabled; No strong feelings on last_ftrace_enabled placement, leaving it to your preference. > out: > mutex_unlock(&ftrace_lock); > return ret; > -- Kamalesh ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-10-16 5:02 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-14 10:59 [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag Miroslav Benes 2019-10-14 14:26 ` Petr Mladek 2019-10-14 15:17 ` Steven Rostedt 2019-10-15 7:45 ` Petr Mladek 2019-10-15 10:50 ` Miroslav Benes 2019-10-15 13:36 ` Steven Rostedt 2019-10-14 22:31 ` Joe Lawrence 2019-10-15 11:23 ` Miroslav Benes 2019-10-15 13:25 ` Joe Lawrence 2019-10-15 14:02 ` Miroslav Benes 2019-10-15 14:58 ` Joe Lawrence 2019-10-16 5:02 ` Kamalesh Babulal
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).