Livepatch module usually used to modify kernel functions.
If the patched function have bug, it may cause serious result
such as kernel crash.
This is a kobject attribute of klp_func. Sysfs interface named
"called" is introduced to livepatch which will be set as true
if the patched function is called.
/sys/kernel/livepatch/<patch>/<object>/<function,sympos>/called
This value "called" is quite necessary for kernel stability
assurance for livepatching module of a running system.
Testing process is important before a livepatch module apply to
a production system. With this interface, testing process can
easily find out which function is successfully called.
Any testing process can make sure they have successfully cover
all the patched function that changed with the help of this interface.
Signed-off-by: Wardenjohn <[email protected]>
---
include/linux/livepatch.h | 2 ++
kernel/livepatch/core.c | 18 ++++++++++++++++++
kernel/livepatch/patch.c | 2 ++
3 files changed, 22 insertions(+)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 51a258c24ff5..026431825593 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,6 +37,7 @@
* @nop: temporary patch to use the original code again; dyn. allocated
* @patched: the func has been added to the klp_ops list
* @transition: the func is currently being applied or reverted
+ * @called: the func is called
*
* The patched and transition variables define the func's patching state. When
* patching, a func is always in one of the following states:
@@ -75,6 +76,7 @@ struct klp_func {
bool nop;
bool patched;
bool transition;
+ bool called;
};
struct klp_object;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 52426665eecc..a840ddd41d00 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -470,6 +470,22 @@ static struct attribute *klp_object_attrs[] = {
};
ATTRIBUTE_GROUPS(klp_object);
+static ssize_t called_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct klp_func *func;
+
+ func = container_of(kobj, struct klp_func, kobj);
+ return sysfs_emit(buf, "%d\n", func->called);
+}
+
+static struct kobj_attribute called_kobj_attr = __ATTR_RO(called);
+static struct attribute *klp_func_attrs[] = {
+ &called_kobj_attr.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(klp_func);
+
static void klp_free_object_dynamic(struct klp_object *obj)
{
kfree(obj->name);
@@ -631,6 +647,7 @@ static void klp_kobj_release_func(struct kobject *kobj)
static const struct kobj_type klp_ktype_func = {
.release = klp_kobj_release_func,
.sysfs_ops = &kobj_sysfs_ops,
+ .default_groups = klp_func_groups,
};
static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
@@ -903,6 +920,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
static void klp_init_func_early(struct klp_object *obj,
struct klp_func *func)
{
+ func->called = false;
kobject_init(&func->kobj, &klp_ktype_func);
list_add_tail(&func->node, &obj->func_list);
}
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 90408500e5a3..75b9603a183f 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -118,6 +118,8 @@ static void notrace klp_ftrace_handler(unsigned long ip,
if (func->nop)
goto unlock;
+ if (!func->called)
+ func->called = true;
ftrace_regs_set_instruction_pointer(fregs, (unsigned long)func->new_func);
unlock:
--
2.37.3
Hi,
On Mon, 20 May 2024, Wardenjohn wrote:
> Livepatch module usually used to modify kernel functions.
> If the patched function have bug, it may cause serious result
> such as kernel crash.
>
> This is a kobject attribute of klp_func. Sysfs interface named
> "called" is introduced to livepatch which will be set as true
> if the patched function is called.
>
> /sys/kernel/livepatch/<patch>/<object>/<function,sympos>/called
>
> This value "called" is quite necessary for kernel stability
> assurance for livepatching module of a running system.
> Testing process is important before a livepatch module apply to
> a production system. With this interface, testing process can
> easily find out which function is successfully called.
> Any testing process can make sure they have successfully cover
> all the patched function that changed with the help of this interface.
Even easier is to use the existing tracing infrastructure in the kernel
(ftrace for example) to track the new function. You can obtain much more
information with that than the new attribute provides.
Regards,
Miroslav
> On May 20, 2024, at 14:46, Miroslav Benes <[email protected]> wrote:
>
> Hi,
>
> On Mon, 20 May 2024, Wardenjohn wrote:
>
>> Livepatch module usually used to modify kernel functions.
>> If the patched function have bug, it may cause serious result
>> such as kernel crash.
>>
>> This is a kobject attribute of klp_func. Sysfs interface named
>> "called" is introduced to livepatch which will be set as true
>> if the patched function is called.
>>
>> /sys/kernel/livepatch/<patch>/<object>/<function,sympos>/called
>>
>> This value "called" is quite necessary for kernel stability
>> assurance for livepatching module of a running system.
>> Testing process is important before a livepatch module apply to
>> a production system. With this interface, testing process can
>> easily find out which function is successfully called.
>> Any testing process can make sure they have successfully cover
>> all the patched function that changed with the help of this interface.
>
> Even easier is to use the existing tracing infrastructure in the kernel
> (ftrace for example) to track the new function. You can obtain much more
> information with that than the new attribute provides.
>
> Regards,
> Miroslav
Hi Miroslav
First, in most cases, testing process is should be automated, which make using existing tracing infrastructure inconvenient. Second, livepatch is already use ftrace for functional replacement, I don’t think it is a good choice of using kernel tracing tool to trace a patched function.
At last, this attribute can be thought of as a state of a livepatch function. It is a state, like the "patched" "transition" state of a klp_patch. Adding this state will not break the state consistency of livepatch.
Regards,
Wardenjohn
Please add a version identifier to the message subject.
…
> If the patched function have bug, it may cause serious result
> such as kernel crash.
Wording suggestion:
If the patched function has a bug, it might cause serious side effects
like a kernel crash.
> This is a kobject attribute of klp_func. Sysfs interface named
> "called" is introduced to livepatch …
Under which circumstances will imperative wordings be applied for
another improved change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n94
…
> ---
> include/linux/livepatch.h | 2 ++
…
You may present version descriptions behind the marker line.
Would you like to indicate any adjustments according to your change approach
(from yesterday)?
https://lore.kernel.org/lkml/[email protected]/
Regards,
Markus
OK, I will try to optimize my description after the patch is reviewed. I am sure there are something still need to be fix for that patch.
> On May 20, 2024, at 16:00, Markus Elfring <[email protected]> wrote:
>
> Please add a version identifier to the message subject.
>
>
> …
>> If the patched function have bug, it may cause serious result
>> such as kernel crash.
>
> Wording suggestion:
>
> If the patched function has a bug, it might cause serious side effects
> like a kernel crash.
>
>
>> This is a kobject attribute of klp_func. Sysfs interface named
>> "called" is introduced to livepatch …
>
> Under which circumstances will imperative wordings be applied for
> another improved change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n94
>
>
> …
>> ---
>> include/linux/livepatch.h | 2 ++
> …
>
> You may present version descriptions behind the marker line.
> Would you like to indicate any adjustments according to your change approach
> (from yesterday)?
> https://lore.kernel.org/lkml/[email protected]/
>
> Regards,
> Markus
Hello,
On Mon, 20 May 2024, zhang warden wrote:
>
>
> > On May 20, 2024, at 14:46, Miroslav Benes <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, 20 May 2024, Wardenjohn wrote:
> >
> >> Livepatch module usually used to modify kernel functions.
> >> If the patched function have bug, it may cause serious result
> >> such as kernel crash.
> >>
> >> This is a kobject attribute of klp_func. Sysfs interface named
> >> "called" is introduced to livepatch which will be set as true
> >> if the patched function is called.
> >>
> >> /sys/kernel/livepatch/<patch>/<object>/<function,sympos>/called
> >>
> >> This value "called" is quite necessary for kernel stability
> >> assurance for livepatching module of a running system.
> >> Testing process is important before a livepatch module apply to
> >> a production system. With this interface, testing process can
> >> easily find out which function is successfully called.
> >> Any testing process can make sure they have successfully cover
> >> all the patched function that changed with the help of this interface.
> >
> > Even easier is to use the existing tracing infrastructure in the kernel
> > (ftrace for example) to track the new function. You can obtain much more
> > information with that than the new attribute provides.
> >
> > Regards,
> > Miroslav
> Hi Miroslav
>
> First, in most cases, testing process is should be automated, which make
> using existing tracing infrastructure inconvenient.
could you elaborate, please? We use ftrace exactly for this purpose and
our testing process is also more or less automated.
> Second, livepatch is
> already use ftrace for functional replacement, I don?t think it is a
> good choice of using kernel tracing tool to trace a patched function.
Why?
> At last, this attribute can be thought of as a state of a livepatch
> function. It is a state, like the "patched" "transition" state of a
> klp_patch. Adding this state will not break the state consistency of
> livepatch.
Yes, but the information you get is limited compared to what is available
now. You would obtain the information that a patched function was called
but ftrace could also give you the context and more.
It would not hurt to add a new attribute per se but I am wondering about
the reason and its usage. Once we have it, the commit message should be
improved based on that.
Regards,
Miroslav
On Tue 2024-05-21 08:34:46, Miroslav Benes wrote:
> Hello,
>
> On Mon, 20 May 2024, zhang warden wrote:
>
> >
> >
> > > On May 20, 2024, at 14:46, Miroslav Benes <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, 20 May 2024, Wardenjohn wrote:
> > >
> > >> Livepatch module usually used to modify kernel functions.
> > >> If the patched function have bug, it may cause serious result
> > >> such as kernel crash.
> > >>
> > >> This is a kobject attribute of klp_func. Sysfs interface named
> > >> "called" is introduced to livepatch which will be set as true
> > >> if the patched function is called.
> > >>
> > >> /sys/kernel/livepatch/<patch>/<object>/<function,sympos>/called
> > >>
> > >> This value "called" is quite necessary for kernel stability
> > >> assurance for livepatching module of a running system.
> > >> Testing process is important before a livepatch module apply to
> > >> a production system. With this interface, testing process can
> > >> easily find out which function is successfully called.
> > >> Any testing process can make sure they have successfully cover
> > >> all the patched function that changed with the help of this interface.
> > >
> > > Even easier is to use the existing tracing infrastructure in the kernel
> > > (ftrace for example) to track the new function. You can obtain much more
> > > information with that than the new attribute provides.
> > >
> > > Regards,
> > > Miroslav
> > Hi Miroslav
> >
> > First, in most cases, testing process is should be automated, which make
> > using existing tracing infrastructure inconvenient.
>
> could you elaborate, please? We use ftrace exactly for this purpose and
> our testing process is also more or less automated.
>
> > Second, livepatch is
> > already use ftrace for functional replacement, I don’t think it is a
> > good choice of using kernel tracing tool to trace a patched function.
>
> Why?
>
> > At last, this attribute can be thought of as a state of a livepatch
> > function. It is a state, like the "patched" "transition" state of a
> > klp_patch. Adding this state will not break the state consistency of
> > livepatch.
>
> Yes, but the information you get is limited compared to what is available
> now. You would obtain the information that a patched function was called
> but ftrace could also give you the context and more.
Another motivation to use ftrace for testing is that it does not
affect the performance in production.
We should keep klp_ftrace_handler() as fast as possible so that we
could livepatch also performance sensitive functions.
Best Regards,
Petr
> On May 21, 2024, at 16:04, Petr Mladek <[email protected]> wrote:
>
> Another motivation to use ftrace for testing is that it does not
> affect the performance in production.
>
> We should keep klp_ftrace_handler() as fast as possible so that we
> could livepatch also performance sensitive functions.
>
How about using unlikely() for branch testing? If we use unlikely, maybe there is no negative effect to klp_ftrace_handler() once this function is called.
Regards,
Wardenjohn
Hi Bros,
How about my patch? I do think it is a viable feature to show the state of the patched function. If we add an unlikely branch test before we set the 'called' state, once this function is called, there maybe no negative effect to the performance.
Please give me some advice.
Regards,
Wardenjohn
Hi,
On Fri, 31 May 2024, zhang warden wrote:
>
> Hi Bros,
>
> How about my patch? I do think it is a viable feature to show the state
> of the patched function. If we add an unlikely branch test before we set
> the 'called' state, once this function is called, there maybe no
> negative effect to the performance.
you have not replied to my questions/feedback yet.
Also, I do not think that unlikely changes anything here. It is a simple
branch after all.
Miroslav
> On May 31, 2024, at 15:21, Miroslav Benes <[email protected]> wrote:
>
> Hi,
>
> On Fri, 31 May 2024, zhang warden wrote:
>
> you have not replied to my questions/feedback yet.
>
> Also, I do not think that unlikely changes anything here. It is a simple
> branch after all.
>
> Miroslav
Hi Miroslav,
Sorry for my carelessness. I apologise for my ignorance.
> Second, livepatch is
> already use ftrace for functional replacement, I don’t think it is a
> good choice of using kernel tracing tool to trace a patched function.
I admit that ftrace can use for tracing the new patched function. But for some cases, user who what to know the state of this function can easily cat the 'called' interface.
It is more convenient than using ftrace to trace the state.
And for the unlikely branch, isn’t the complier will compile this branch into a cold branch that will do no harm to the function performance?
Regards,
Wardenjohn
> And for the unlikely branch, isn’t the complier will compile this branch
> into a cold branch that will do no harm to the function performance?
The test (cmp insn or something like that) still needs to be there. Since
there is only a simple assignment in the branch, the compiler may just
choose not to have a cold branch in this case. The only difference is in
which case you would jump here. You can see for yourself (and prove me
wrong if it comes to it).
Miroslav
On Tue, May 21, 2024 at 08:34:46AM +0200, Miroslav Benes wrote:
> Hello,
>
> On Mon, 20 May 2024, zhang warden wrote:
>
> >
> >
> > > On May 20, 2024, at 14:46, Miroslav Benes <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, 20 May 2024, Wardenjohn wrote:
> > >
> > >> Livepatch module usually used to modify kernel functions.
> > >> If the patched function have bug, it may cause serious result
> > >> such as kernel crash.
> > >>
> > >> This is a kobject attribute of klp_func. Sysfs interface named
> > >> "called" is introduced to livepatch which will be set as true
> > >> if the patched function is called.
> > >>
> > >> /sys/kernel/livepatch/<patch>/<object>/<function,sympos>/called
> > >>
> > >> This value "called" is quite necessary for kernel stability
> > >> assurance for livepatching module of a running system.
> > >> Testing process is important before a livepatch module apply to
> > >> a production system. With this interface, testing process can
> > >> easily find out which function is successfully called.
> > >> Any testing process can make sure they have successfully cover
> > >> all the patched function that changed with the help of this interface.
> > >
> > > Even easier is to use the existing tracing infrastructure in the kernel
> > > (ftrace for example) to track the new function. You can obtain much more
> > > information with that than the new attribute provides.
> > >
> > > Regards,
> > > Miroslav
> > Hi Miroslav
> >
> > First, in most cases, testing process is should be automated, which make
> > using existing tracing infrastructure inconvenient.
>
> could you elaborate, please? We use ftrace exactly for this purpose and
> our testing process is also more or less automated.
>
> > Second, livepatch is
> > already use ftrace for functional replacement, I don’t think it is a
> > good choice of using kernel tracing tool to trace a patched function.
>
> Why?
>
> > At last, this attribute can be thought of as a state of a livepatch
> > function. It is a state, like the "patched" "transition" state of a
> > klp_patch. Adding this state will not break the state consistency of
> > livepatch.
>
> Yes, but the information you get is limited compared to what is available
> now. You would obtain the information that a patched function was called
> but ftrace could also give you the context and more.
>
> It would not hurt to add a new attribute per se but I am wondering about
> the reason and its usage. Once we have it, the commit message should be
> improved based on that.
>
I agree with Miroslav about using ftrace, or Dan in the other thread
about using gcov if even more granular coverage is needed.
Admittedly, I would have to go find documentation to remember how to do
either, but at least I'd be using well-tested facilities designed for
this exact purpose.
Adding these attributes to livepatch sysfs would be expedient and
probably easier for us to use, but imposes a recurring burden on us to
maintain and test (where is the documentation and kselftest for this new
interface?). Or, we could let the other tools handle all of that for
us.
Perhaps if someone already has an off-the-shelf script that is using
ftrace to monitor livepatched code, it could be donated to
Documentation/livepatch/? I can ask our QE folks if they have something
like this.
Regards,
--
Joe
> On Jun 1, 2024, at 03:16, Joe Lawrence <[email protected]> wrote:
>
> Adding these attributes to livepatch sysfs would be expedient and
> probably easier for us to use, but imposes a recurring burden on us to
> maintain and test (where is the documentation and kselftest for this new
> interface?). Or, we could let the other tools handle all of that for
> us.
How this attribute imposes a recurring burden to maintain and test?
> Perhaps if someone already has an off-the-shelf script that is using
> ftrace to monitor livepatched code, it could be donated to
> Documentation/livepatch/? I can ask our QE folks if they have something
> like this.
My intention to introduce this attitude to sysfs is that user who what to see if this function is called can just need to show this function attribute in the livepatch sysfs interface.
User who have no experience of using ftrace will have problems to get the calling state of the patched function. After all, ftrace is a professional kernel tracing tools.
Adding this attribute will be more easier for us to show if this patched function is called. Actually, I have never try to use ftrace to trace a patched function. Is it OK of using ftrace for a livepatched function?
Regards,
Wardenjohn
> On May 31, 2024, at 22:06, Miroslav Benes <[email protected]> wrote:
>
>> And for the unlikely branch, isn’t the complier will compile this branch
>> into a cold branch that will do no harm to the function performance?
>
> The test (cmp insn or something like that) still needs to be there. Since
> there is only a simple assignment in the branch, the compiler may just
> choose not to have a cold branch in this case. The only difference is in
> which case you would jump here. You can see for yourself (and prove me
> wrong if it comes to it).
>
> Miroslav
Hi Miroslav,
Yes, more tests should be done in this case according to your opinion.
Regards,
Wardenjohn
> My intention to introduce this attitude to sysfs is that user who what to see if this function is called can just need to show this function attribute in the livepatch sysfs interface.
>
Sorry bros,
There is a typo in my word : attitude -> attribute
Autocomplete make it wrong….lol..
Wardenjohn
On Tue, Jun 04, 2024 at 04:14:51PM +0800, zhang warden wrote:
>
>
> > On Jun 1, 2024, at 03:16, Joe Lawrence <[email protected]> wrote:
> >
> > Adding these attributes to livepatch sysfs would be expedient and
> > probably easier for us to use, but imposes a recurring burden on us to
> > maintain and test (where is the documentation and kselftest for this new
> > interface?). Or, we could let the other tools handle all of that for
> > us.
> How this attribute imposes a recurring burden to maintain and test?
>
Perhaps "responsibility" is a better description. This would introduce
an attribute that someone's userspace utility is relying on. It should
at least have a kselftest to ensure a random patch in 2027 doesn't break
it.
> > Perhaps if someone already has an off-the-shelf script that is using
> > ftrace to monitor livepatched code, it could be donated to
> > Documentation/livepatch/? I can ask our QE folks if they have something
> > like this.
>
> My intention to introduce this attitude to sysfs is that user who what to see if this function is called can just need to show this function attribute in the livepatch sysfs interface.
>
> User who have no experience of using ftrace will have problems to get the calling state of the patched function. After all, ftrace is a professional kernel tracing tools.
>
> Adding this attribute will be more easier for us to show if this patched function is called. Actually, I have never try to use ftrace to trace a patched function. Is it OK of using ftrace for a livepatched function?
>
If you build with CONFIG_SAMPLE_LIVEPATCH=m, you can try it out (or with
one of your own livepatches):
# Convenience variable
$ SYSFS=/sys/kernel/debug/tracing
# Install the livepatch sample demo module
$ insmod samples/livepatch/livepatch-sample.ko
# Verify that ftrace can filter on our functions
$ grep cmdline_proc_show $SYSFS/available_filter_functions
cmdline_proc_show
livepatch_cmdline_proc_show [livepatch_sample]
# Turn off any existing tracing and filter functions
$ echo 0 > $SYSFS/tracing_on
$ echo > $SYSFS/set_ftrace_filter
# Set up the function tracer and add the kernel's cmdline_proc_show()
# and livepatch-sample's livepatch_cmdline_proc_show()
$ echo function > $SYSFS/current_tracer
$ echo cmdline_proc_show >> $SYSFS/set_ftrace_filter
$ echo livepatch_cmdline_proc_show >> $SYSFS/set_ftrace_filter
$ cat $SYSFS/set_ftrace_filter
cmdline_proc_show
livepatch_cmdline_proc_show [livepatch_sample]
# Turn on the ftracing and force execution of the original and
# livepatched functions
$ echo 1 > $SYSFS/tracing_on
$ cat /proc/cmdline
this has been live patched
# Checkout out the trace file results
$ cat $SYSFS/trace
# tracer: function
#
# entries-in-buffer/entries-written: 2/2 #P:8
#
# _-----=> irqs-off/BH-disabled
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / _-=> migrate-disable
# |||| / delay
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
cat-254 [002] ...2. 363.043498: cmdline_proc_show <-seq_read_iter
cat-254 [002] ...1. 363.043501: livepatch_cmdline_proc_show <-seq_read_iter
The kernel docs provide a lot of explanation of the complete ftracing
interface. It's pretty power stuff, though you may also go the other
direction and look into using the trace-cmd front end to simplify all of
the sysfs manipulation. Julia Evans wrote a blog [1] a while back that
provides a some more examples.
[1] https://jvns.ca/blog/2017/03/19/getting-started-with-ftrace/
--
Joe
Hi Joe,
>
> Perhaps "responsibility" is a better description. This would introduce
> an attribute that someone's userspace utility is relying on. It should
> at least have a kselftest to ensure a random patch in 2027 doesn't break
> it.
I sent this patch to see the what the community thinks about this attribute (although it think it is necessary and this will be more convenient for users).
If this patch is seems to be good, I will add a kselftest to this attribute.
As Miroslav and Petr said, keeping klp_ftrace_handler() as fast as possible is also important, which I need to find a way to keep it fast (or just setting the state to be true instead of a judgement?).
> The kernel docs provide a lot of explanation of the complete ftracing
> interface. It's pretty power stuff, though you may also go the other
> direction and look into using the trace-cmd front end to simplify all of
> the sysfs manipulation. Julia Evans wrote a blog [1] a while back that
> provides a some more examples.
>
> [1] https://jvns.ca/blog/2017/03/19/getting-started-with-ftrace/
>
> --
> Joe
Nice of you! Thanks! I will learn it!
Regards,
Wardenjohn
On Tue, May 21, 2024 at 1:04 AM Petr Mladek <[email protected]> wrote:
[...]
> >
> > Yes, but the information you get is limited compared to what is available
> > now. You would obtain the information that a patched function was called
> > but ftrace could also give you the context and more.
>
> Another motivation to use ftrace for testing is that it does not
> affect the performance in production.
>
> We should keep klp_ftrace_handler() as fast as possible so that we
> could livepatch also performance sensitive functions.
At LPC last year, we discussed about adding a counter to each
klp_func, like:
struct klp_func {
...
u64 __percpu counter;
...
};
With some static_key (+ sysctl), this should give us a way to estimate
the overhead of livepatch. If we have the counter, this patch is not
needed any more. Does this (adding the counter) sound like
something we still want to pursue?
Thanks,
Song
Hi Wardenjohn,
To follow up, Red Hat kpatch QE pointed me toward this test:
https://gitlab.com/redhat/centos-stream/tests/kernel/kernel-tests/-/tree/main/general/kpatch/kpatch-trace?ref_type=heads
which reports a few interesting things via systemd service and ftrace:
- Patched functions
- Traced functions
- Code that was modified, but didn't run
- Other code that ftrace saw, but is not listed in the sysfs directory
The code looks to be built on top of the same basic ftrace commands that
I sent the other day. You may be able to reuse/copy/etc bits from the
scripts if they are handy.
--
Joe
> On Jun 6, 2024, at 23:01, Joe Lawrence <[email protected]> wrote:
>
> Hi Wardenjohn,
>
> To follow up, Red Hat kpatch QE pointed me toward this test:
>
> https://gitlab.com/redhat/centos-stream/tests/kernel/kernel-tests/-/tree/main/general/kpatch/kpatch-trace?ref_type=heads
>
> which reports a few interesting things via systemd service and ftrace:
>
> - Patched functions
> - Traced functions
> - Code that was modified, but didn't run
> - Other code that ftrace saw, but is not listed in the sysfs directory
>
> The code looks to be built on top of the same basic ftrace commands that
> I sent the other day. You may be able to reuse/copy/etc bits from the
> scripts if they are handy.
>
> --
> Joe
>
Thank you so much, you really helped me, Joe!
I will try to learn the script and make it suitable for our system.
Again, thank you, Joe!
Regards,
Wardenjohn
Hi,
On Tue, 4 Jun 2024, Song Liu wrote:
> On Tue, May 21, 2024 at 1:04 AM Petr Mladek <[email protected]> wrote:
> [...]
> > >
> > > Yes, but the information you get is limited compared to what is available
> > > now. You would obtain the information that a patched function was called
> > > but ftrace could also give you the context and more.
> >
> > Another motivation to use ftrace for testing is that it does not
> > affect the performance in production.
> >
> > We should keep klp_ftrace_handler() as fast as possible so that we
> > could livepatch also performance sensitive functions.
>
> At LPC last year, we discussed about adding a counter to each
> klp_func, like:
>
> struct klp_func {
> ...
> u64 __percpu counter;
> ...
> };
>
> With some static_key (+ sysctl), this should give us a way to estimate
> the overhead of livepatch. If we have the counter, this patch is not
> needed any more. Does this (adding the counter) sound like
> something we still want to pursue?
It would be better than this patch but given what was mentioned in the
thread I wonder if it is possible to use ftrace even for this. See
/sys/kernel/tracing/trace_stat/function*. It already gathers the number of
hits.
Would it be sufficient for you? I guess it depends on what the intention
is. If there is no time limit, klp_func.counter might be better to provide
some kind of overall statistics (but I am not sure if it has any value)
and to avoid having ftrace registered on a live patched function for
infinite period of time. If the intention is to gather data for some
limited period, trace_stat sounds like much better approach to me.
Regards
Miroslav
Hi Miroslav,
On Fri, Jun 7, 2024 at 2:07 AM Miroslav Benes <[email protected]> wrote:
>
> Hi,
>
> On Tue, 4 Jun 2024, Song Liu wrote:
>
> > On Tue, May 21, 2024 at 1:04 AM Petr Mladek <[email protected]> wrote:
> > [...]
> > > >
> > > > Yes, but the information you get is limited compared to what is available
> > > > now. You would obtain the information that a patched function was called
> > > > but ftrace could also give you the context and more.
> > >
> > > Another motivation to use ftrace for testing is that it does not
> > > affect the performance in production.
> > >
> > > We should keep klp_ftrace_handler() as fast as possible so that we
> > > could livepatch also performance sensitive functions.
> >
> > At LPC last year, we discussed about adding a counter to each
> > klp_func, like:
> >
> > struct klp_func {
> > ...
> > u64 __percpu counter;
> > ...
> > };
> >
> > With some static_key (+ sysctl), this should give us a way to estimate
> > the overhead of livepatch. If we have the counter, this patch is not
> > needed any more. Does this (adding the counter) sound like
> > something we still want to pursue?
>
> It would be better than this patch but given what was mentioned in the
> thread I wonder if it is possible to use ftrace even for this. See
> /sys/kernel/tracing/trace_stat/function*. It already gathers the number of
> hits.
I didn't know about the trace_stat API until today. :) It somehow doesn't
exist on some older kernels. (I haven't debugged it.)
> Would it be sufficient for you? I guess it depends on what the intention
> is. If there is no time limit, klp_func.counter might be better to provide
> some kind of overall statistics (but I am not sure if it has any value)
> and to avoid having ftrace registered on a live patched function for
> infinite period of time. If the intention is to gather data for some
> limited period, trace_stat sounds like much better approach to me.
We don't have very urgent use for this. As we discussed, various tracing
tools are sufficient in most cases. I brought this up in the context of the
"called" entry: if we are really adding a new entry, let's do "counter"
instead of "called".
Thanks,
Song
> We don't have very urgent use for this. As we discussed, various tracing
> tools are sufficient in most cases. I brought this up in the context of the
> "called" entry: if we are really adding a new entry, let's do "counter"
> instead of "called".
>
> Thanks,
> Song
Hi, Song
I hope to find a way to optimize "called" will be set once in klp_ftrace_ops function because as Petr said this function should be as fast as possible. But if use "count", this variable will be called every time klp_ftrace_ops run.
Regards,
Wardenjohn