Re: [PATCH] livepatch: introduce klp_func called interface
Hi, On Thu, 20 Jun 2024, zhang warden wrote: > > > > On Jun 7, 2024, at 17:07, Miroslav Benes 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
> On Jun 7, 2024, at 17:07, Miroslav Benes 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
> 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
Hi Miroslav,
On Fri, Jun 7, 2024 at 2:07 AM Miroslav Benes wrote:
>
> Hi,
>
> On Tue, 4 Jun 2024, Song Liu wrote:
>
> > On Tue, May 21, 2024 at 1:04 AM Petr Mladek 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
Hi,
On Tue, 4 Jun 2024, Song Liu wrote:
> On Tue, May 21, 2024 at 1:04 AM Petr Mladek 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
> On Jun 6, 2024, at 23:01, Joe Lawrence 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
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
On Tue, May 21, 2024 at 1:04 AM Petr Mladek 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
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
On Tue, Jun 04, 2024 at 04:14:51PM +0800, zhang warden wrote: > > > > On Jun 1, 2024, at 03:16, Joe Lawrence 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
> 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
> On May 31, 2024, at 22:06, Miroslav Benes 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
> On Jun 1, 2024, at 03:16, Joe Lawrence 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
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 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/livepatchcalled > > >> > > >> 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
> 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
> On May 31, 2024, at 15:21, Miroslav Benes 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
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
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
> On May 23, 2024, at 22:22, Dan Carpenter 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
Re: [PATCH] livepatch: introduce klp_func called interface
> On May 21, 2024, at 16:04, Petr Mladek 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
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/livepatchcalled > > 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
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 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/livepatchcalled > > >> > > >> 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
Hello, On Mon, 20 May 2024, zhang warden wrote: > > > > On May 20, 2024, at 14:46, Miroslav Benes 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/livepatchcalled > >> > >> 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
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 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-zhangwar...@gmail.com/ > > Regards, > Markus
Re: [PATCH] livepatch: introduce klp_func called interface
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-zhangwar...@gmail.com/ Regards, Markus
Re: [PATCH] livepatch: introduce klp_func called interface
> On May 20, 2024, at 14:46, Miroslav Benes 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/livepatchcalled >> >> 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
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/livepatchcalled > > 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
[PATCH] livepatch: introduce klp_func called interface
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/livepatchcalled
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
---
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
OK, I will optimize my patch’s changelog in my next patch. > On May 20, 2024, at 02:05, Markus Elfring 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
… > 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
[PATCH] livepatch: introduce klp_func called interface
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/livepatchcalled
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
