[PATCH] livepatch: introduce klp_func called interface

[PATCH] livepatch: introduce klp_func called interface

[Wardenjohn]

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 <zhangwarden@gmail.com>
---
 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Miroslav Benes]

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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On May 20, 2024, at 14:46, Miroslav Benes <mbenes@suse.cz> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Miroslav Benes]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

Hello,

On Mon, 20 May 2024, zhang warden wrote:

> 
> 
> > On May 20, 2024, at 14:46, Miroslav Benes <mbenes@suse.cz> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Petr Mladek]

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 <mbenes@suse.cz> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On May 21, 2024, at 16:04, Petr Mladek <pmladek@suse.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

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

Re: [PATCH] livepatch: introduce klp_func called interface

[Miroslav Benes]

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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On May 31, 2024, at 15:21, Miroslav Benes <mbenes@suse.cz> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Miroslav Benes]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On May 31, 2024, at 22:06, Miroslav Benes <mbenes@suse.cz> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Song Liu]

On Tue, May 21, 2024 at 1:04 AM Petr Mladek <pmladek@suse.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Miroslav Benes]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

Hi,

On Tue, 4 Jun 2024, Song Liu wrote:

> On Tue, May 21, 2024 at 1:04 AM Petr Mladek <pmladek@suse.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Song Liu]

Hi Miroslav,

On Fri, Jun 7, 2024 at 2:07 AM Miroslav Benes <mbenes@suse.cz> wrote:
>
> Hi,
>
> On Tue, 4 Jun 2024, Song Liu wrote:
>
> > On Tue, May 21, 2024 at 1:04 AM Petr Mladek <pmladek@suse.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On Jun 7, 2024, at 17:07, Miroslav Benes <mbenes@suse.cz> wrote:
> 
> 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.
> 

Hi, Miroslav

Can ftrace able to trace the function which I added into kernel by livepatching?

Regard
Wardenjohn

Re: [PATCH] livepatch: introduce klp_func called interface

[Miroslav Benes]

Hi,

On Thu, 20 Jun 2024, zhang warden wrote:

> 
> 
> > On Jun 7, 2024, at 17:07, Miroslav Benes <mbenes@suse.cz> wrote:
> > 
> > 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.
> > 
> 
> Hi, Miroslav
> 
> Can ftrace able to trace the function which I added into kernel by livepatching?

yes, it should also work as is. I just generally recommend to use a 
different name (prefixed for example) for the new replacement function to 
avoid issues if you do not already do it.

Miroslav

Re: [PATCH] livepatch: introduce klp_func called interface

[Joe Lawrence]

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 <mbenes@suse.cz> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On Jun 1, 2024, at 03:16, Joe Lawrence <joe.lawrence@redhat.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Joe Lawrence]

On Tue, Jun 04, 2024 at 04:14:51PM +0800, zhang warden wrote:
> 
> 
> > On Jun 1, 2024, at 03:16, Joe Lawrence <joe.lawrence@redhat.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

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

Re: [PATCH] livepatch: introduce klp_func called interface

[Joe Lawrence]

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

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On Jun 6, 2024, at 23:01, Joe Lawrence <joe.lawrence@redhat.com> 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

Re: [PATCH] livepatch: introduce klp_func called interface

[Markus Elfring]

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/20240519074343.5833-1-zhangwarden@gmail.com/

Regards,
Markus

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

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 <Markus.Elfring@web.de> 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/20240519074343.5833-1-zhangwarden@gmail.com/
> 
> Regards,
> Markus

[PATCH] livepatch: introduce klp_func called interface

[Wardenjohn]

Livepatch module usually used to modify kernel functions.
If the patched function have bug, it may cause serious result
such as kernel crash.

This commit introduce a read only interface of livepatch
sysfs interface. If a livepatch function is called, this
sysfs interface "called" of the patched function will
set to be 1.

/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.
---
 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..bc055b56dbe5 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 = 0;
 	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

Re: [PATCH] livepatch: introduce klp_func called interface

[Markus Elfring]

…
> This commit introduce a read only interface of livepatch

Please improve the changelog with an imperative wording.


…
> 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.

* I suggest to take preferred line lengths better into account
  also for such a change description.

* Please provide the tag “Signed-off-by”.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n398

Regards,
Markus

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

OK, I will optimize my patch’s changelog in my next patch.

> On May 20, 2024, at 02:05, Markus Elfring <Markus.Elfring@web.de> wrote:
> 
>  I suggest to take preferred line lengths better into account
>  also for such a change description.

Re: [PATCH] livepatch: introduce klp_func called interface

[Dan Carpenter]

On Sun, May 19, 2024 at 03:43:43PM +0800, 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 commit introduce a read only interface of livepatch
> sysfs interface. If a livepatch function is called, this
> sysfs interface "called" of the patched function will
> set to be 1.
> 
> /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.
> ---

Always run your patches through checkpatch.

So this patch is so that testers can see if a function has been called?
Can you not get the same information from gcov or ftrace?

There are style issues with the patch, but it's not so important until
the design is agreed on.

regards,
dan carpenter

Re: [PATCH] livepatch: introduce klp_func called interface

[zhang warden]

> On May 23, 2024, at 22:22, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> Always run your patches through checkpatch.
> 
> So this patch is so that testers can see if a function has been called?
> Can you not get the same information from gcov or ftrace?
> 
> There are style issues with the patch, but it's not so important until
> the design is agreed on.
> 
> regards,
> dan carpenter

Hi, Dan.

This patch have format issues as Markus said. A newer version of this patch is sent which is checked by ./scripts/checkpatch.pl

Thanks for your suggestions.

Regards,
Wardenjohn