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
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
Re: [PATCH 1/1] livepatch: Rename KLP_* to KLP_TRANSITION_*
On Tue, 7 May 2024, zhangwar...@gmail.com wrote: > From: Wardenjohn > > The original macros of KLP_* is about the state of the transition. > Rename macros of KLP_* to KLP_TRANSITION_* to fix the confusing > description of klp transition state. > > Signed-off-by: Wardenjohn Acked-by: Miroslav Benes M
Re: [PATCH v1 0/5] livepatch: klp-convert tool - Minimal version
Hi, > > Summary of changes in this minimal version > > > > > > - rebase for v6.5 > > - cleaned-up SoB chains (suggested by pmladek) > > - klp-convert: remove the symbol map auto-resolving solution > > - klp-convert: add macro for flagging variables inside a LP src to be > > resolved by this tool > > - klp-convert: code simplification > > Do we have anything that blocks klp-convert-mini to be merged, or something > that > needs to be fixed? there is feedback from Petr and I agree with him that a selftest would be appropriate. Lukas, are you planning to send v2 with everything addressed? Miroslav
Re: [ANNOUNCE and CfP] Live Patching MC at LPC 2023
Hello, On Fri, 11 Aug 2023, Miroslav Benes wrote: > Hi, > > On Wed, 14 Jun 2023, Miroslav Benes wrote: > > > Hello, > > > > the Live Patching Microconference for Linux Plumbers Conference 2023 has > > been accepted. > > > > It is possible to submit topic proposals and abstracts for the > > microconference through Indico system linked at [1]. I welcome you to do > > so. Submissions should then be published at [2]. > > > > The rest still remains to be sorted out. Joe and I will share information > > as it is available. > > > > The registration will open soon [3]. > > > > See you in Richmond! > > > > Miroslav > > > > [1] https://lpc.events/event/17/abstracts/ > > [2] https://lpc.events/event/17/contributions/1405/ > > [3] > > https://lpc.events/blog/current/index.php/2023/06/14/registration-for-lpc-2023-is-almost-here/ > > Live Patching MC CfP blog post was published at > https://lpc.events/blog/current/index.php/2023/08/09/live-patching-mc-cfp/ > > Let me use the opportunity to also encourage you to submit topics for the > MC as mentioned above. ...and this is the last call. If you still want to submit a topic for the MC, please do so as soon as possible. Thank you, Miroslav
Re: the qemu-nbd process automatically exit with the commit 43347d56c 'livepatch: send a fake signal to all blocking tasks'
On Wed, 14 Apr 2021, Josef Bacik wrote:
> On 4/14/21 11:21 AM, xiaojun.zhao...@gmail.com wrote:
> > On Wed, 14 Apr 2021 13:27:43 +0200 (CEST)
> > Miroslav Benes wrote:
> >
> >> Hi,
> >>
> >> On Wed, 14 Apr 2021, xiaojun.zhao...@gmail.com wrote:
> >>
> >>> I found the qemu-nbd process(started with qemu-nbd -t -c /dev/nbd0
> >>> nbd.qcow2) will automatically exit when I patched for functions of
> >>> the nbd with livepatch.
> >>>
> >>> The nbd relative source:
> >>> static int nbd_start_device_ioctl(struct nbd_device *nbd, struct
> >>> block_device *bdev)
> >>> { struct nbd_config *config =
> >>> nbd->config; int
> >>> ret;
> >>> ret =
> >>> nbd_start_device(nbd); if
> >>> (ret) return
> >>> ret;
> >>> if
> >>> (max_part) bdev->bd_invalidated =
> >>> 1;
> >>> mutex_unlock(>config_lock); ret =
> >>> wait_event_interruptible(config->recv_wq,
> >>> atomic_read(>recv_threads) == 0); if
> >>> (ret)
> >>> sock_shutdown(nbd);
> >>> flush_workqueue(nbd->recv_workq);
> >>> mutex_lock(>config_lock);
> >>> nbd_bdev_reset(bdev);
> >>> /* user requested, ignore socket errors
> >>> */ if (test_bit(NBD_RT_DISCONNECT_REQUESTED,
> >>> >runtime_flags)) ret =
> >>> 0; if (test_bit(NBD_RT_TIMEDOUT,
> >>> >runtime_flags)) ret =
> >>> -ETIMEDOUT; return
> >>> ret; }
> >>
> >> So my understanding is that ndb spawns a number
> >> (config->recv_threads) of workqueue jobs and then waits for them to
> >> finish. It waits interruptedly. Now, any signal would make
> >> wait_event_interruptible() to return -ERESTARTSYS. Livepatch fake
> >> signal is no exception there. The error is then propagated back to
> >> the userspace. Unless a user requested a disconnection or there is
> >> timeout set. How does the userspace then reacts to it? Is
> >> _interruptible there because the userspace sends a signal in case of
> >> NBD_RT_DISCONNECT_REQUESTED set? How does the userspace handles
> >> ordinary signals? This all sounds a bit strange, but I may be missing
> >> something easily.
> >>
> >>> When the nbd waits for atomic_read(>recv_threads) == 0, the
> >>> klp will send a fake signal to it then the qemu-nbd process exits.
> >>> And the signal of sysfs to control this action was removed in the
> >>> commit 10b3d52790e 'livepatch: Remove signal sysfs attribute'. Are
> >>> there other ways to control this action? How?
> >>
> >> No, there is no way currently. We send a fake signal automatically.
> >>
> >> Regards
> >> Miroslav
> > It occurs IO error of the nbd device when I use livepatch of the
> > nbd, and I guess that any livepatch on other kernel source maybe cause
> > the IO error. Well, now I decide to workaround for this problem by
> > adding a livepatch for the klp to disable a automatic fake signal.
> >
>
> Would wait_event_killable() fix this problem? I'm not sure any client
> implementations depend on being able to send other signals to the client
> process, so it should be safe from that standpoint. Not sure if the livepatch
> thing would still get an error at that point tho. Thanks,
wait_event_killable() means that you would sleep uninterruptedly (still
reacting to fatal signals), so the fake signal from livepatch would not be
sent at all. set_notify_signal() handles TASK_INTERRUPTIBLE tasks. No
disruption for the userspace and it would fix this problem.
There is a catch on the livepatch side of things. If there is a live patch
for nbd_start_device_ioctl(), the transition process would get stuck until
the task leaves the function (all workqueue jobs are processed). I gather
it is unlikely to be it indefinite, so we can live with that, I think.
Miroslav
Re: the qemu-nbd process automatically exit with the commit 43347d56c 'livepatch: send a fake signal to all blocking tasks'
Hi,
On Wed, 14 Apr 2021, xiaojun.zhao...@gmail.com wrote:
> I found the qemu-nbd process(started with qemu-nbd -t -c /dev/nbd0
> nbd.qcow2) will automatically exit when I patched for functions of
> the nbd with livepatch.
>
> The nbd relative source:
> static int nbd_start_device_ioctl(struct nbd_device *nbd, struct block_device
> *bdev)
> {
>
> struct nbd_config *config = nbd->config;
>
> int ret;
>
>
>
> ret = nbd_start_device(nbd);
>
> if (ret)
>
> return ret;
>
>
>
> if (max_part)
>
> bdev->bd_invalidated = 1;
>
> mutex_unlock(>config_lock);
>
> ret = wait_event_interruptible(config->recv_wq,
>
> atomic_read(>recv_threads)
> == 0);
> if (ret)
>
> sock_shutdown(nbd);
>
> flush_workqueue(nbd->recv_workq);
>
>
>
> mutex_lock(>config_lock);
>
> nbd_bdev_reset(bdev);
>
> /* user requested, ignore socket errors */
>
> if (test_bit(NBD_RT_DISCONNECT_REQUESTED, >runtime_flags))
>
> ret = 0;
>
> if (test_bit(NBD_RT_TIMEDOUT, >runtime_flags))
>
> ret = -ETIMEDOUT;
>
> return ret;
>
> }
So my understanding is that ndb spawns a number (config->recv_threads) of
workqueue jobs and then waits for them to finish. It waits interruptedly.
Now, any signal would make wait_event_interruptible() to return
-ERESTARTSYS. Livepatch fake signal is no exception there. The error is
then propagated back to the userspace. Unless a user requested a
disconnection or there is timeout set. How does the userspace then reacts
to it? Is _interruptible there because the userspace sends a signal in
case of NBD_RT_DISCONNECT_REQUESTED set? How does the userspace handles
ordinary signals? This all sounds a bit strange, but I may be missing
something easily.
> When the nbd waits for atomic_read(>recv_threads) == 0, the klp
> will send a fake signal to it then the qemu-nbd process exits. And the
> signal of sysfs to control this action was removed in the commit
> 10b3d52790e 'livepatch: Remove signal sysfs attribute'. Are there other
> ways to control this action? How?
No, there is no way currently. We send a fake signal automatically.
Regards
Miroslav
Re: [PATCH 1/2] zram: fix crashes due to use of cpu hotplug multistate
Hi, > > Driver developers will simply have to open code these protections. In > > light of what I see on LTP / fuzzing, I suspect the use case will grow > > and we'll have to revisit this in the future. But for now, sure, we can > > just open code the required protections everywhere to not crash on module > > removal. > > LTP and fuzzing too do not remove modules. So I do not understand the > root problem here, that's just something that does not happen on a real > system. If I am not mistaken, the issue that Luis tries to solve here was indeed found by running LTP. > On Sat, Apr 03, 2021 at 08:13:23AM +0200, Greg KH wrote: > > On Fri, Apr 02, 2021 at 06:30:16PM +, Luis Chamberlain wrote: > > > On Fri, Apr 02, 2021 at 09:54:12AM +0200, Greg KH wrote: > > > > No, please no. Module removal is a "best effort", > > > > > > Not for live patching. I am not sure if I am missing any other valid > > > use case? > > > > live patching removes modules? We have so many code paths that are > > "best effort" when it comes to module unloading, trying to resolve this > > one is a valiant try, but not realistic. > > Miroslav, your input / help here would be valuable. I did the > generalization work because you said it would be worthy for you too... Yes, we have the option to revert and remove the existing live patch from the system. I am not sure how (if) it is used in practice. At least at SUSE we do not support the option. But we are only one of the many downstream users. So yes, there is the option. Miroslav
Re: [PATCH v3 00/16] x86,objtool: Optimize !RETPOLINE
On Fri, 26 Mar 2021, Peter Zijlstra wrote: > Hi, another week, another update :-) > > Respin of the !RETPOLINE optimization patches. > > Boris, the first 3 should probably go into tip/x86/core, it's an ungodly > tangle > since it relies on the insn decoder patches in tip/x86/core, the NOP patches > in > tip/x86/cpu and the alternative patches in tip/x86/alternatives. > > Just to make life easy I'd suggest merging everything in x86/core and > forgetting about the other topic branches (that's what I ended up doing > locally). > > The remaining 13 patches depend on the first 3 as well as on the work in > tip/objtool/core, just to make life more interesting still ;-) > > All except the last 4 patches should be fairly uncontroversial (I hope...). > > There's a fair number of new patches and another few have been completely > rewritten, but it all seems to work nicely. Reviewed-by: Miroslav Benes for the objtool changes. All looks much better in this version. I have only one minor thing. There are only two call sites of elf_add_string(). The one in elf_create_section() passes shstrtab, the other one in elf_create_undef_symbol() NULL. elf_add_string() then retrieves it itself. I think it would be nicer to just call find_section_by_name() in elf_create_undef_symbol(), pass it down and make it consistent. Might be a matter of taste. Miroslav
[PATCH v2] livepatch: Replace the fake signal sending with TIF_NOTIFY_SIGNAL infrastructure
Livepatch sends a fake signal to all remaining blocking tasks of a
running transition after a set period of time. It uses TIF_SIGPENDING
flag for the purpose. Commit 12db8b690010 ("entry: Add support for
TIF_NOTIFY_SIGNAL") added a generic infrastructure to achieve the same.
Replace our bespoke solution with the generic one.
Reviewed-by: Jens Axboe
Reviewed-by: Petr Mladek
Signed-off-by: Miroslav Benes
---
v2:
- #include from kernel/signal.c removed [Petr]
kernel/livepatch/transition.c | 5 ++---
kernel/signal.c | 4 +---
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include "core.h"
#include "patch.h"
#include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
* Send fake signal to all non-kthread tasks which are
* still not migrated.
*/
- spin_lock_irq(>sighand->siglock);
- signal_wake_up(task, 0);
- spin_unlock_irq(>sighand->siglock);
+ set_notify_signal(task);
}
}
read_unlock(_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index f2a1b898da29..604290a8ca89 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -43,7 +43,6 @@
#include
#include
#include
-#include
#include
#include
@@ -181,8 +180,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
void recalc_sigpending(void)
{
- if (!recalc_sigpending_tsk(current) && !freezing(current) &&
- !klp_patch_pending(current))
+ if (!recalc_sigpending_tsk(current) && !freezing(current))
clear_thread_flag(TIF_SIGPENDING);
}
--
2.30.2
[PATCH] livepatch: Replace the fake signal sending with TIF_NOTIFY_SIGNAL infrastructure
Livepatch sends a fake signal to all remaining blocking tasks of a
running transition after a set period of time. It uses TIF_SIGPENDING
flag for the purpose. Commit 12db8b690010 ("entry: Add support for
TIF_NOTIFY_SIGNAL") added a generic infrastructure to achieve the same.
Replace our bespoke solution with the generic one.
Signed-off-by: Miroslav Benes
---
Tested on x86_64, s390x and ppc64le archs.
kernel/livepatch/transition.c | 5 ++---
kernel/signal.c | 3 +--
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include "core.h"
#include "patch.h"
#include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
* Send fake signal to all non-kthread tasks which are
* still not migrated.
*/
- spin_lock_irq(>sighand->siglock);
- signal_wake_up(task, 0);
- spin_unlock_irq(>sighand->siglock);
+ set_notify_signal(task);
}
}
read_unlock(_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index f2a1b898da29..e52cb82aaecd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
void recalc_sigpending(void)
{
- if (!recalc_sigpending_tsk(current) && !freezing(current) &&
- !klp_patch_pending(current))
+ if (!recalc_sigpending_tsk(current) && !freezing(current))
clear_thread_flag(TIF_SIGPENDING);
}
--
2.30.2
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
On Thu, 25 Mar 2021, Jens Axboe wrote: > On 3/25/21 3:30 AM, Miroslav Benes wrote: > >> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is > >> a > >> silly question, but... > >> > >> If the livepatch code could use fake_signal_wake_up(), we could consolidate > >> the pattern in klp_send_signals() with the one in freeze_task(). Then > >> there > >> would only one place for wake up / fake signal logic. > >> > >> I don't fully understand the differences in the freeze_task() version, so I > >> only pose this as a question and not v2 request. > > > > The plan was to remove our live patching fake signal completely and use > > the new infrastructure Jens proposed in the past. > > That would be great, I've actually been waiting for that to show up! Sorry about that. I failed to notice that the infrastructure was merged already. I'll send it soonish. > I would greatly prefer this approach if you deem it suitable for 5.12, > if not we'll still need the temporary work-around for live patching. I noticed there is 20210326003928.978750-1-ax...@kernel.dk now, so I suppose we should wait for that to land in mainline and simply do nothing about PF_IO_WORKER for live patching. Miroslav
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a
> silly question, but...
>
> If the livepatch code could use fake_signal_wake_up(), we could consolidate
> the pattern in klp_send_signals() with the one in freeze_task(). Then there
> would only one place for wake up / fake signal logic.
>
> I don't fully understand the differences in the freeze_task() version, so I
> only pose this as a question and not v2 request.
The plan was to remove our live patching fake signal completely and use
the new infrastructure Jens proposed in the past.
Something like
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include "core.h"
#include "patch.h"
#include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
* Send fake signal to all non-kthread tasks which are
* still not migrated.
*/
- spin_lock_irq(>sighand->siglock);
- signal_wake_up(task, 0);
- spin_unlock_irq(>sighand->siglock);
+ set_notify_signal(task);
}
}
read_unlock(_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index a15c584a0455..b7cf4eda8611 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
void recalc_sigpending(void)
{
- if (!recalc_sigpending_tsk(current) && !freezing(current) &&
- !klp_patch_pending(current))
+ if (!recalc_sigpending_tsk(current) && !freezing(current))
clear_thread_flag(TIF_SIGPENDING);
}
Let me verify it still works and there are all the needed pieces merged
for all the architectures we support (x86_64, ppc64le and s390x). I'll
send a proper patch then.
Miroslav
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
On Thu, 25 Mar 2021, Dong Kai wrote:
> commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like
> PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads
> by making the freezer not send them fake signals.
>
> Here live patching consistency model call klp_send_signals to wake up
> all tasks by send fake signal to all non-kthread which only check the
> PF_KTHREAD flag, so it still send signal to io threads which may lead to
> freezeing issue of io threads.
I suppose this could happen, but it will also affect the live patching
transition if the io threads do not react to signals.
Are you able to reproduce it easily? I mean, is there a testcase I could
use to take a closer look?
> Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
> within klp_send_signal function.
Yes, this sounds reasonable.
Miroslav
> Signed-off-by: Dong Kai
> ---
> note:
> the io threads freeze issue links:
> [1] https://lore.kernel.org/io-uring/yegnip43%2f6kfn...@kevinlocke.name/
> [2]
> https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb5...@kernel.dk/
>
> kernel/livepatch/transition.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index f6310f848f34..0e1c35c8f4b4 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -358,7 +358,7 @@ static void klp_send_signals(void)
>* Meanwhile the task could migrate itself and the action
>* would be meaningless. It is not serious though.
>*/
> - if (task->flags & PF_KTHREAD) {
> + if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) {
> /*
>* Wake up a kthread which sleeps interruptedly and
>* still has not been migrated.
Re: [PATCH] docs: livepatch: Fix a typo
On Thu, 25 Mar 2021, Bhaskar Chowdhury wrote: > On 10:05 Thu 25 Mar 2021, Miroslav Benes wrote: > >Hi, > > > >On Thu, 25 Mar 2021, Bhaskar Chowdhury wrote: > > > >> > >> s/varibles/variables/ > >> > >> Signed-off-by: Bhaskar Chowdhury > >> --- > >> Documentation/livepatch/shadow-vars.rst | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/Documentation/livepatch/shadow-vars.rst > >> b/Documentation/livepatch/shadow-vars.rst > >> index c05715aeafa4..8464866d18ba 100644 > >> --- a/Documentation/livepatch/shadow-vars.rst > >> +++ b/Documentation/livepatch/shadow-vars.rst > >> @@ -165,7 +165,7 @@ In-flight parent objects > >> > >> Sometimes it may not be convenient or possible to allocate shadow > >> variables alongside their parent objects. Or a livepatch fix may > >> -require shadow varibles to only a subset of parent object instances. In > >> +require shadow variables to only a subset of parent object instances. In > >> these cases, the klp_shadow_get_or_alloc() call can be used to attach > >> shadow variables to parents already in-flight. > > > >you sent the same fix a couple of weeks ago and Jon applied it. > > > Ah..difficult to rememberthanks for reminding ..it seems I need to keep > track ...which I don't do at this moment ...so the patch get duplicated .. Well, you definitely should. > So.do you have any better policy to keep track??? I do not send a large amount of typo fixes, so it is quite easy to keep track of everything in my case. So please, just find something that suits you. Miroslav
Re: [PATCH] docs: livepatch: Fix a typo
Hi, On Thu, 25 Mar 2021, Bhaskar Chowdhury wrote: > > s/varibles/variables/ > > Signed-off-by: Bhaskar Chowdhury > --- > Documentation/livepatch/shadow-vars.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/livepatch/shadow-vars.rst > b/Documentation/livepatch/shadow-vars.rst > index c05715aeafa4..8464866d18ba 100644 > --- a/Documentation/livepatch/shadow-vars.rst > +++ b/Documentation/livepatch/shadow-vars.rst > @@ -165,7 +165,7 @@ In-flight parent objects > > Sometimes it may not be convenient or possible to allocate shadow > variables alongside their parent objects. Or a livepatch fix may > -require shadow varibles to only a subset of parent object instances. In > +require shadow variables to only a subset of parent object instances. In > these cases, the klp_shadow_get_or_alloc() call can be used to attach > shadow variables to parents already in-flight. you sent the same fix a couple of weeks ago and Jon applied it. Miroslav
Re: [PATCH v2 06/14] objtool: Fix static_call list generation
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1045,6 +1045,12 @@ static int add_call_destinations(struct
> } else
> insn->call_dest = reloc->sym;
>
> + if (insn->call_dest && insn->call_dest->static_call_tramp) {
> + list_add_tail(>static_call_node,
> + >static_call_list);
> + }
> +
> +
A nit, but too many new lines here.
Miroslav
Re: [PATCH 6/9] objtool: Add elf_create_undef_symbol()
On Wed, 17 Mar 2021, Peter Zijlstra wrote:
> On Wed, Mar 17, 2021 at 02:52:23PM +0100, Miroslav Benes wrote:
>
> > > + if (!elf_symbol_add(elf, sym, SHN_XINDEX)) {
> > > + WARN("elf_symbol_add");
> > > + return NULL;
> > > + }
> >
> > SHN_XINDEX means that the extended section index is used. Above you seem
> > to use it in the opposite sense too (assigning to shndx when shndx_data is
> > NULL). While it makes the code easier to handle, it is a bit confusing
> > (and maybe I am just confused now). Could you add a comment about that,
> > please? elf_symbol_add() seems like a good place.
>
> Yes, that was a horrible thing to do :/ And you understood it right.
>
> Looking at it again, I'm not sure it is actually correct tho; shouldn't
> elf_create_undef_symbol() also look at gelf_getsymshndx() of symtab ?
Probably yes.
> What toolchain generates these extended sections and how? That is, how
> do I test this crud..
Sami might know.
Miroslav
Re: [PATCH 6/9] objtool: Add elf_create_undef_symbol()
[ also correcting my e-mail address ]
On Fri, 12 Mar 2021, Peter Zijlstra wrote:
Just a remark regarding SHN_XINDEX...
> +static bool elf_symbol_add(struct elf *elf, struct symbol *sym, Elf32_Word
> shndx)
> +{
> + struct list_head *entry;
> + struct rb_node *pnode;
> +
> + sym->type = GELF_ST_TYPE(sym->sym.st_info);
> + sym->bind = GELF_ST_BIND(sym->sym.st_info);
> +
> + if ((sym->sym.st_shndx > SHN_UNDEF &&
> + sym->sym.st_shndx < SHN_LORESERVE) ||
> + (shndx != SHN_XINDEX && sym->sym.st_shndx == SHN_XINDEX)) {
> + if (sym->sym.st_shndx != SHN_XINDEX)
> + shndx = sym->sym.st_shndx;
> +
> + sym->sec = find_section_by_index(elf, shndx);
> + if (!sym->sec) {
> + WARN("couldn't find section for symbol %s",
> + sym->name);
> + return false;
> + }
...
> @@ -366,47 +414,11 @@ static int read_symbols(struct elf *elf)
> goto err;
> }
>
> - sym->type = GELF_ST_TYPE(sym->sym.st_info);
> - sym->bind = GELF_ST_BIND(sym->sym.st_info);
> + if (!shndx_data)
> + shndx = SHN_XINDEX;
>
> - if ((sym->sym.st_shndx > SHN_UNDEF &&
> - sym->sym.st_shndx < SHN_LORESERVE) ||
> - (shndx_data && sym->sym.st_shndx == SHN_XINDEX)) {
> - if (sym->sym.st_shndx != SHN_XINDEX)
> - shndx = sym->sym.st_shndx;
> -
> - sym->sec = find_section_by_index(elf, shndx);
> - if (!sym->sec) {
> - WARN("couldn't find section for symbol %s",
> - sym->name);
> - goto err;
...
> + if (!elf_symbol_add(elf, sym, SHN_XINDEX)) {
> + WARN("elf_symbol_add");
> + return NULL;
> + }
SHN_XINDEX means that the extended section index is used. Above you seem
to use it in the opposite sense too (assigning to shndx when shndx_data is
NULL). While it makes the code easier to handle, it is a bit confusing
(and maybe I am just confused now). Could you add a comment about that,
please? elf_symbol_add() seems like a good place.
Miroslav
Re: [PATCH -tip 3/5] kprobes: treewide: Remove trampoline_address from kretprobe_trampoline_handler()
Hi Masami,
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -205,15 +205,23 @@ extern void arch_prepare_kretprobe(struct
> kretprobe_instance *ri,
> struct pt_regs *regs);
> extern int arch_trampoline_kprobe(struct kprobe *p);
>
> +void kretprobe_trampoline(void);
> +/*
> + * Since some architecture uses structured function pointer,
> + * use arch_deref_entry_point() to get real function address.
s/arch_deref_entry_point/dereference_function_descriptor/ ?
> + */
> +static nokprobe_inline void *kretprobe_trampoline_addr(void)
> +{
> + return dereference_function_descriptor(kretprobe_trampoline);
> +}
> +
Would it make sense to use this in s390 and powerpc reliable unwinders?
Both
arch/s390/kernel/stacktrace.c:arch_stack_walk_reliable()
arch/powerpc/kernel/stacktrace.c:__save_stack_trace_tsk_reliable()
have
if (state.ip == (unsigned long)kretprobe_trampoline)
return -EINVAL;
which you wanted to hide previously if I am not mistaken.
Miroslav
Re: [PATCH RFC] MIPS: livepatch: Add LIVEPATCH basic code
Hi, I cannot really comment on mips arch specifics but few words from the live patching perspective. On Mon, 1 Mar 2021, Jinyang He wrote: > Add the basic code of livepatch. livepatch is temporarily unavailable. > Two core functions are missing, one is DYNAMIC_FTRACE_WITH_REGS, and > another is save_stack_trace_tsk_reliable(). > `Huang Pei ` is doing for ftrace. He will use > `-fpatchable-function-entry` to achieve more complete ftrace. DYNAMIC_FTRACE_WITH_ARGS has been introduced recently, so you might also look at that. As far as the live patching is concerned, DYNAMIC_FTRACE_WITH_ARGS is sufficient. > save_stack_trace_tsk_reliable() currently has difficulties. This function > may be improved in the future, but that seems to be a long time away. > This is also the reason for delivering this RFC. Hope to get any help. You may want to look at Documentation/livepatch/reliable-stacktrace.rst which nicely describes the requirements for the reliable stacktraces. Regards Miroslav
Re: [PATCH 0/2] x86/unwind/orc: Handle missing ORC data better
On Fri, 5 Feb 2021, Josh Poimboeuf wrote: > A couple of patches for improving the ORC unwinder's handling of missing > ORC data. > > Josh Poimboeuf (2): > x86/unwind/orc: Disable KASAN checking in the ORC unwinder, part 2 > x86/unwind/orc: Silence warnings caused by missing ORC data > > arch/x86/kernel/unwind_orc.c | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) I apologize for a really late reply. It seems it has not been merged yet, so Reviewed-by: Miroslav Benes M
Re: [PATCH] module: potential uninitialized return in module_kallsyms_on_each_symbol()
On Wed, 10 Feb 2021, Dan Carpenter wrote:
> Smatch complains that:
>
> kernel/module.c:4472 module_kallsyms_on_each_symbol()
> error: uninitialized symbol 'ret'.
>
> This warning looks like it could be correct if the list is
> empty.
>
> Fixes: 013c1667cf78 ("kallsyms: refactor {,module_}kallsyms_on_each_symbol")
> Signed-off-by: Dan Carpenter
Reviewed-by: Miroslav Benes
M
Re: [PATCH v2 0/3] objtool: Support the stack swizzle
On Tue, 9 Feb 2021, Peter Zijlstra wrote: > Hi! > > Implement objtool support for the x86_64 stack swizzle pattern. > > This means we can use the minial stack swizzle: > > mov %rsp, (%[tos]) > mov %[tos], %rsp > ... > pop %rsp > > from inline asm, with arbitrary stack setup. The ORC data for the Top-of-Stack > will use the SP_INDIRECT CFA base. In order for this to work, SP_INDIRECT > needs > to first dereference and then add the offset to find the next frame. > > Therefore we need to change SP_INDIRECT (which is currently unused) to mean: > (%rsp) + offset. > > Changes since v1 include: > > - removed the !callee saved reg restriction by using the vals[] array >over the regs[] array. > > - per the above, removed the patches creating the regs[] scratch space. > > - more comments. > > - rebased to tip/objtool/core I haven't tested it, but it all looks good to me. Reviewed-by: Miroslav Benes M
Re: [GIT PULL] x86/urgent for v5.11-rc7
On Tue, 9 Feb 2021, Steven Rostedt wrote:
> On Tue, 9 Feb 2021 09:32:34 +0100 (CET)
> Miroslav Benes wrote:
>
> > powerpc has this
> >
> > static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
> >
> > {
> >
> > /*
> >
> > * Live patch works only with -mprofile-kernel on PPC. In this
> > case,
> > * the ftrace location is always within the first 16 bytes.
> >
> > */
> >
> > return ftrace_location_range(faddr, faddr + 16);
> >
> > }
> >
> >
> > > > I suppose the trivial fix is to see if it points to endbr64 and if so,
> > > > increment the addr by the length of that.
> > >
> > > I thought of that too. But one thing that may be possible, is to use
> > > kallsym. I believe you can get the range of a function (start and end of
> > > the function) from kallsyms. Then ask ftrace for the addr in that range
> > > (there should only be one).
> >
> > And we can do this if a hard-coded value live above is not welcome. If I
> > remember correctly, we used to have exactly this in the old versions of
> > kGraft. We walked through all ftrace records, called
> > kallsyms_lookup_size_offset() on every record's ip and if the offset+ip
> > matched faddr (in this case), we returned the ip.
>
> Either way is fine. Question is, should we just wait till CET is
> implemented for the kernel before making any of these changes? Just knowing
> that we have a solution to handle it may be good enough for now.
I'd prefer it to be a part of CET enablement patch set.
Miroslav
Re: [GIT PULL] x86/urgent for v5.11-rc7
On Mon, 8 Feb 2021, Steven Rostedt wrote:
> On Mon, 8 Feb 2021 16:47:05 +0100
> Peter Zijlstra wrote:
>
> > > /*
> > > * Convert a function address into the appropriate ftrace location.
> > > *
> > > * Usually this is just the address of the function, but on some
> > > architectures
> > > * it's more complicated so allow them to provide a custom behaviour.
> > > */
> > > #ifndef klp_get_ftrace_location
> > > static unsigned long klp_get_ftrace_location(unsigned long faddr)
> > > {
> > > return faddr;
> > > }
> > > #endif
powerpc has this
static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
{
/*
* Live patch works only with -mprofile-kernel on PPC. In this case,
* the ftrace location is always within the first 16 bytes.
*/
return ftrace_location_range(faddr, faddr + 16);
}
> > I suppose the trivial fix is to see if it points to endbr64 and if so,
> > increment the addr by the length of that.
>
> I thought of that too. But one thing that may be possible, is to use
> kallsym. I believe you can get the range of a function (start and end of
> the function) from kallsyms. Then ask ftrace for the addr in that range
> (there should only be one).
And we can do this if a hard-coded value live above is not welcome. If I
remember correctly, we used to have exactly this in the old versions of
kGraft. We walked through all ftrace records, called
kallsyms_lookup_size_offset() on every record's ip and if the offset+ip
matched faddr (in this case), we returned the ip.
Miroslav
Re: [PATCH 10/13] module: pass struct find_symbol_args to find_symbol
On Wed, 3 Feb 2021, Christoph Hellwig wrote: > FYI, this is the updated version: > > --- > >From 664ca3378deac7530fe8fc15fe73d583ddf2 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig > Date: Wed, 20 Jan 2021 14:58:27 +0100 > Subject: module: pass struct find_symbol_args to find_symbol > > Simplify the calling convention by passing the find_symbol_args structure > to find_symbol instead of initializing it inside the function. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 08/13] module: remove each_symbol_in_section
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > each_symbol_in_section just contains a trivial loop over its arguments. > Just open code the loop in the two callers. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 09/13] module: merge each_symbol_section into find_symbol
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > each_symbol_section is only called by find_symbol, so merge the two > functions. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 10/13] module: pass struct find_symbol_args to find_symbol
> void *__symbol_get(const char *symbol)
> {
> - struct module *owner;
> - const struct kernel_symbol *sym;
> + struct find_symbol_arg fsa = {
> + .name = symbol,
> + .gplok = true,
> + .warn = true,
> + };
>
> preempt_disable();
> - sym = find_symbol(symbol, , NULL, NULL, true, true);
> - if (sym && strong_try_module_get(owner))
> - sym = NULL;
> + if (!find_symbol() || !strong_try_module_get(fsa.owner)) {
I think this should be in fact
if (!find_symbol() || strong_try_module_get(fsa.owner)) {
to get the logic right (note the missing !). We want to return NULL if
strong_try_module_get() does not succeed for a found symbol.
> + preempt_enable();
> + return NULL;
> + }
> preempt_enable();
> -
> - return sym ? (void *)kernel_symbol_value(sym) : NULL;
> + return (void *)kernel_symbol_value(fsa.sym);
> }
> EXPORT_SYMBOL_GPL(__symbol_get);
Miroslav
Re: [PATCH 13/13] module: remove EXPORT_UNUSED_SYMBOL*
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > EXPORT_UNUSED_SYMBOL* is not actually used anywhere. Remove the > unused functionality as we generally just remove unused code anyway. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 12/13] module: remove EXPORT_SYMBOL_GPL_FUTURE
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > As far as I can tell this has never been used at all, and certainly > not any time recently. Right, I've always wondered about this one. > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 11/13] module: move struct symsearch to module.c
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > struct symsearch is only used inside of module.h, so move the definition > out of module.h. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 07/13] module: mark module_mutex static
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > Except for two lockdep asserts module_mutex is only used in module.c. > Remove the two asserts given that the functions they are in are not > exported and just called from the module code, and mark module_mutex > static. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 06/13] kallsyms: only build {,module_}kallsyms_on_each_symbol when required
On Tue, 2 Feb 2021, Christoph Hellwig wrote: > kallsyms_on_each_symbol and module_kallsyms_on_each_symbol are only used > by the livepatching code, so don't build them if livepatching is not > enabled. > > Signed-off-by: Christoph Hellwig Reviewed-by: Miroslav Benes M
Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
On Mon, 1 Feb 2021, Christoph Hellwig wrote:
> On Mon, Feb 01, 2021 at 02:37:12PM +0100, Miroslav Benes wrote:
> > > > This change is not needed. (objname == NULL) means that we are
> > > > interested only in symbols in "vmlinux".
> > > >
> > > > module_kallsyms_on_each_symbol(klp_find_callback, )
> > > > will always fail when objname == NULL.
> > >
> > > I just tried to keep the old behavior. I can respin it with your
> > > recommended change noting the change in behavior, though.
> >
> > Yes, please. It would be cleaner that way.
>
> Let me know if this works for you:
>
> ---
> >From 18af41e88d088cfb8680d1669fcae2bc2ede5328 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig
> Date: Wed, 20 Jan 2021 16:23:16 +0100
> Subject: kallsyms: refactor {,module_}kallsyms_on_each_symbol
>
> Require an explicit call to module_kallsyms_on_each_symbol to look
> for symbols in modules instead of the call from kallsyms_on_each_symbol,
> and acquire module_mutex inside of module_kallsyms_on_each_symbol instead
> of leaving that up to the caller. Note that this slightly changes the
> behavior for the livepatch code in that the symbols from vmlinux are not
> iterated anymore if objname is set, but that actually is the desired
> behavior in this case.
>
> Signed-off-by: Christoph Hellwig
Acked-by: Miroslav Benes
Thanks Christoph
M
Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
One more thing...
> @@ -4379,8 +4379,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *,
> const char *,
> unsigned int i;
> int ret;
>
> - module_assert_mutex();
> -
> + mutex_lock(_mutex);
> list_for_each_entry(mod, , list) {
> /* We hold module_mutex: no need for rcu_dereference_sched */
> struct mod_kallsyms *kallsyms = mod->kallsyms;
This was the last user of module_assert_mutex(), which can be removed now.
Miroslav
Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
On Mon, 1 Feb 2021, Christoph Hellwig wrote: > On Fri, Jan 29, 2021 at 10:43:36AM +0100, Petr Mladek wrote: > > > --- a/kernel/livepatch/core.c > > > +++ b/kernel/livepatch/core.c > > > @@ -164,12 +164,8 @@ static int klp_find_object_symbol(const char > > > *objname, const char *name, > > > .pos = sympos, > > > }; > > > > > > - mutex_lock(_mutex); > > > - if (objname) > > > + if (objname || !kallsyms_on_each_symbol(klp_find_callback, )) > > > module_kallsyms_on_each_symbol(klp_find_callback, ); > > > - else > > > - kallsyms_on_each_symbol(klp_find_callback, ); > > > - mutex_unlock(_mutex); > > > > This change is not needed. (objname == NULL) means that we are > > interested only in symbols in "vmlinux". > > > > module_kallsyms_on_each_symbol(klp_find_callback, ) > > will always fail when objname == NULL. > > I just tried to keep the old behavior. I can respin it with your > recommended change noting the change in behavior, though. Yes, please. It would be cleaner that way. Miroslav
Re: [PATCH 04/13] module: use RCU to synchronize find_module
On Mon, 1 Feb 2021, Jessica Yu wrote: > +++ Miroslav Benes [29/01/21 16:29 +0100]: > >On Thu, 28 Jan 2021, Christoph Hellwig wrote: > > > >> Allow for a RCU-sched critical section around find_module, following > >> the lower level find_module_all helper, and switch the two callers > >> outside of module.c to use such a RCU-sched critical section instead > >> of module_mutex. > > > >That's a nice idea. > > > >> @@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object > >> *obj) > >> if (!klp_is_module(obj)) > >>return; > >> > >> - mutex_lock(_mutex); > >> + rcu_read_lock_sched(); > >> /* > >>* We do not want to block removal of patched modules and therefore > >>* we do not take a reference here. The patches are removed by > >> @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object > >> *obj) > >> if (mod && mod->klp_alive) > > > >RCU always baffles me a bit, so I'll ask. Don't we need > >rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I > >wonder. > > Same here :-) I had to double check the RCU documentation. For our > modules list case I believe the rcu list API should take care of that > for us. Worth noting is this snippet from Documentation/RCU/whatisRCU.txt: > >rcu_dereference() is typically used indirectly, via the _rcu >list-manipulation primitives, such as list_for_each_entry_rcu() Ok, thanks to both for checking and explanation. Ack to the patch then. Miroslav
Re: [PATCH] stacktrace: Move documentation for arch_stack_walk_reliable() to header
On Mon, 18 Jan 2021, Mark Brown wrote:
> Currently arch_stack_wallk_reliable() is documented with an identical
> comment in both x86 and S/390 implementations which is a bit redundant.
> Move this to the header and convert to kerneldoc while we're at it.
Makes sense.
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Borislav Petkov
> Cc: "H. Peter Anvin"
> Cc: Heiko Carstens
> Cc: Vasily Gorbik
> Cc: Christian Borntraeger
> Cc: Josh Poimboeuf
> Cc: Jiri Kosina
> Cc: Miroslav Benes
> Cc: Petr Mladek
> Cc: Joe Lawrence
> Cc: x...@kernel.org
> Cc: linux-s...@vger.kernel.org
> Cc: live-patch...@vger.kernel.org
> Signed-off-by: Mark Brown
Reviewed-by: Miroslav Benes
> diff --git a/arch/s390/kernel/stacktrace.c b/arch/s390/kernel/stacktrace.c
> index 7f1266c24f6b..101477b3e263 100644
> --- a/arch/s390/kernel/stacktrace.c
> +++ b/arch/s390/kernel/stacktrace.c
> @@ -24,12 +24,6 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry,
> void *cookie,
> }
> }
>
> -/*
> - * This function returns an error if it detects any unreliable features of
> the
> - * stack. Otherwise it guarantees that the stack trace is reliable.
> - *
> - * If the task is not 'current', the caller *must* ensure the task is
> inactive.
> - */
> int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
>void *cookie, struct task_struct *task)
> {
Just a note. arch/powerpc/kernel/stacktrace.c has the same for
__save_stack_trace_tsk_reliable(), but it would not be nice to pollute
include/linux/stacktrace.h with that in my opinion. It is an old
infrastructure after all.
Miroslav
Re: [PATCH v6 0/2] Documentation: livepatch: Document reliable stacktrace and minor cleanup
Hi, On Wed, 20 Jan 2021, Mark Brown wrote: > This series adds a document, mainly written by Mark Rutland, which makes > explicit the requirements for implementing reliable stacktrace in order > to aid architectures adding this feature. It also updates the other > livepatching documents to use automatically generated tables of contents > following review comments on Mark's document. > > v6: > - Remove a duplicated "points". > v5: > - Tweaks to the commit message for the new document. > - Convert new and existing documents to autogenerated tables of >contents. > v4: > - Renumber table of contents > v3: > - Incorporated objtool section from Mark. > - Deleted confusing notes about using annotations. > > Mark Brown (1): > Documentation: livepatch: Convert to automatically generated contents > > Mark Rutland (1): > Documentation: livepatch: document reliable stacktrace > > Documentation/livepatch/index.rst | 1 + > Documentation/livepatch/livepatch.rst | 15 +- > Documentation/livepatch/module-elf-format.rst | 10 +- > .../livepatch/reliable-stacktrace.rst | 309 ++ > 4 files changed, 313 insertions(+), 22 deletions(-) > create mode 100644 Documentation/livepatch/reliable-stacktrace.rst sorry for the late reply (slowly crawling through my email backlog). Thanks a lot for putting this together! FWIW (so it is at least archived in the thread) Acked-by: Miroslav Benes M
Re: [PATCH] objtool: Don't fail the kernel build on fatal errors
On Thu, 14 Jan 2021, Josh Poimboeuf wrote:
> This is basically a revert of commit 644592d32837 ("objtool: Fail the
> kernel build on fatal errors").
>
> That change turned out to be more trouble than it's worth. Failing the
> build is an extreme measure which sometimes gets too much attention and
> blocks CI build testing.
>
> These fatal-type warnings aren't yet as rare as we'd hope, due to the
> ever-increasing matrix of supported toolchains/plugins and their
> fast-changing nature as of late.
>
> Also, there are more people (and bots) looking for objtool warnings than
> ever before, so such warnings not likely to be ignored for long.
>
> Suggested-by: Nick Desaulniers
> Signed-off-by: Josh Poimboeuf
Reviewed-by: Miroslav Benes
M
Re: [PATCH] objtool: Don't fail on missing symbol table
On Thu, 14 Jan 2021, Josh Poimboeuf wrote:
> Thanks to a recent binutils change which doesn't generate unused
> symbols, it's now possible for thunk_64.o be completely empty with
> CONFIG_PREEMPTION: no text, no data, no symbols.
"without CONFIG_PREEMPTION", or did I misunderstand?
> We could edit the Makefile to only build that file when
> CONFIG_PREEMPTION is enabled, but that will likely create confusion
> if/when the thunks end up getting used by some other code again.
>
> Just ignore it and move on.
>
> Reported-by: Nathan Chancellor
> Signed-off-by: Josh Poimboeuf
Reviewed-by: Miroslav Benes
with the note below.
> ---
> tools/objtool/elf.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index be89c741ba9a..2b0f4f52f7b5 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -380,8 +380,11 @@ static int read_symbols(struct elf *elf)
>
> symtab = find_section_by_name(elf, ".symtab");
> if (!symtab) {
> - WARN("missing symbol table");
> - return -1;
> + /*
> + * A missing symbol table is actually possible if it's an empty
> + * .o file. This can happen for thunk_64.o.
> + */
> + return 0;
> }
We rely on .symtab presence elsewhere in the code. See
elf_create_{rel,rela}_reloc_section(). However, there should never be a
problem. If there is a need to create a new reloc section (either for a
static call site, or ORC), there should always be a symbol to create it
for (or because of it).
Miroslav
Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
> That comment is indeed now obsolete. I can squash something like so: > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 81d56fdef1c3..ce67437aaf3f 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -958,21 +958,8 @@ static int add_call_destinations(struct objtool_file > *file) > } > > /* > - * The .alternatives section requires some extra special care, over and above > - * what other special sections require: > - * > - * 1. Because alternatives are patched in-place, we need to insert a fake > jump > - *instruction at the end so that validate_branch() skips all the original > - *replaced instructions when validating the new instruction path. > - * > - * 2. An added wrinkle is that the new instruction length might be zero. In > - *that case the old instructions are replaced with noops. We simulate > that > - *by creating a fake jump as the only new instruction. > - * > - * 3. In some cases, the alternative section includes an instruction which > - *conditionally jumps to the _end_ of the entry. We have to modify these > - *jumps' destinations to point back to .text rather than the end of the > - *entry in .altinstr_replacement. > + * The .alternatives section requires some extra special care over and above > + * other special sections because alternatives are patched in place. > */ > static int handle_group_alt(struct objtool_file *file, > struct special_alt *special_alt, Looks good to me. Miroslav
Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
On Tue, 22 Dec 2020, Josh Poimboeuf wrote: > BTW, another benefit of these changes is that, thanks to some related > cleanups (new fake nops and alt_group struct) objtool can finally be rid > of fake jumps, which were a constant source of headaches. \o/ You may also want to remove/edit the comment right before handle_group_alt() now that fake jumps are gone. Anyway, I walked through the patch (set) and I think it should work fine (but I am not confident enough to give it Reviewed-by. My head spins :)). I even like the change. Also, 1/3 is a benefit on its own, so if nothing else, it could go in. Regards Miroslav
Re: [PATCH] objtool: Fix seg fault with Clang non-section symbols
On Mon, 14 Dec 2020, Josh Poimboeuf wrote:
> The Clang assembler likes to strip section symbols, which means objtool
> can't reference some text code by its section. This confuses objtool
> greatly, causing it to seg fault.
>
> The fix is similar to what was done before, for ORC reloc generation:
>
> e81e07244325 ("objtool: Support Clang non-section symbols in ORC
> generation")
>
> Factor out that code into a common helper and use it for static call
> reloc generation as well.
>
> Reported-by: Arnd Bergmann
> Reviewed-by: Nick Desaulniers
> Link: https://github.com/ClangBuiltLinux/linux/issues/1207
> Signed-off-by: Josh Poimboeuf
Reviewed-by: Miroslav Benes
M
Re: [PATCH 3/3 v6] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index e00fe88146e0..7c9474d52060 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -54,6 +54,9 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
> return NULL;
> return >regs;
> }
> +
> +#define ftrace_instruction_pointer_set(fregs, ip)\
> + do { (fregs)->regs.ip = (_ip); } while (0)
> #endif
>
> #ifdef CONFIG_DYNAMIC_FTRACE
> diff --git a/arch/x86/include/asm/livepatch.h
> b/arch/x86/include/asm/livepatch.h
> index 1fde1ab6559e..59a08d5c6f1d 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -12,9 +12,9 @@
> #include
> #include
>
> -static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long
> ip)
> {
> - regs->ip = ip;
> + ftrace_regs_set_ip(fregs, ip);
You forgot to update the call site :)
Miroslav
Re: [PATCH 07/11 v3] livepatch: Trigger WARNING if livepatch function fails due to recursion
On Thu, 5 Nov 2020, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > If for some reason a function is called that triggers the recursion > detection of live patching, trigger a warning. By not executing the live > patch code, it is possible that the old unpatched function will be called > placing the system into an unknown state. > > Link: https://lore.kernel.org/r/20201029145709.GD16774@alley > > Cc: Josh Poimboeuf > Cc: Jiri Kosina > Cc: Joe Lawrence > Cc: live-patch...@vger.kernel.org > Suggested-by: Miroslav Benes > Reviewed-by: Petr Mladek > Signed-off-by: Steven Rostedt (VMware) Acked-by: Miroslav Benes > --- > Changes since v2: > > - Blame Miroslav instead of Petr ;-) Thanks. Fortunately, if printk is broken in WARN_ON_ONCE(), I can always blame Petr again :) M
Re: [PATCH 06/11 v3] livepatch/ftrace: Add recursion protection to the ftrace callback
On Thu, 5 Nov 2020, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > If a ftrace callback does not supply its own recursion protection and > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will > make a helper trampoline to do so before calling the callback instead of > just calling the callback directly. > > The default for ftrace_ops is going to change. It will expect that handlers > provide their own recursion protection, unless its ftrace_ops states > otherwise. > > Link: https://lkml.kernel.org/r/20201028115613.291169...@goodmis.org > > Cc: Masami Hiramatsu > Cc: Andrew Morton > Cc: Josh Poimboeuf > Cc: Jiri Kosina > Cc: Miroslav Benes > Cc: Joe Lawrence > Cc: live-patch...@vger.kernel.org > Reviewed-by: Petr Mladek > Signed-off-by: Steven Rostedt (VMware) Acked-by: Miroslav Benes M
Re: [PATCH v2 0/3] module: refactor module_sig_check()
On Sat, 31 Oct 2020, Sergey Shtylyov wrote: > Here are 3 patches against the 'modules-next' branch of Jessica Yu's > 'linux.git' repo. > I'm doing some refactoring in module_sig_check()... > > [1/3] module: merge repetitive strings in module_sig_check() > [2/3] module: avoid *goto*s in module_sig_check() > [3/3] module: only handle errors with the *switch* statement in > module_sig_check() Reviewed-by: Miroslav Benes M
Re: [RFC][PATCH 3/3 v3] livepatching: Use the default ftrace_ops instead of REGS when ARGS is available
(live-patching ML CCed, keeping the complete email for reference)
Hi,
a nit concerning the subject. We use just "livepatch:" as a prefix.
On Wed, 28 Oct 2020, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)"
>
> When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS is available, the ftrace call
> will be able to set the ip of the calling function. This will improve the
> performance of live kernel patching where it does not need all the regs to
> be stored just to change the instruction pointer.
>
> If all archs that support live kernel patching also support
> HAVE_DYNAMIC_FTRACE_WITH_ARGS, then the architecture specific function
> klp_arch_set_pc() could be made generic.
I think this is a nice idea, which could easily lead to removing
FTRACE_WITH_REGS altogether. I'm really looking forward to that because
every consolidation step is welcome.
My only remark is for the config. LIVEPATCH now depends on
DYNAMIC_FTRACE_WITH_REGS which is not completely true after this change,
so we should probably make it depend on DYNAMIC_FTRACE_WITH_REGS ||
DYNAMIC_FTRACE_WITH_ARGS, just to reflect the situation better.
Anyway, it passed my tests too and the patch looks good to me, so
Acked-by: Miroslav Benes
M
> Signed-off-by: Steven Rostedt (VMware)
> ---
> arch/powerpc/include/asm/livepatch.h | 4 +++-
> arch/s390/include/asm/livepatch.h| 5 -
> arch/x86/include/asm/ftrace.h| 3 +++
> arch/x86/include/asm/livepatch.h | 4 ++--
> arch/x86/kernel/ftrace_64.S | 4
> include/linux/ftrace.h | 7 +++
> kernel/livepatch/patch.c | 9 +
> 7 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/livepatch.h
> b/arch/powerpc/include/asm/livepatch.h
> index 4a3d5d25fed5..ae25e6e72997 100644
> --- a/arch/powerpc/include/asm/livepatch.h
> +++ b/arch/powerpc/include/asm/livepatch.h
> @@ -12,8 +12,10 @@
> #include
>
> #ifdef CONFIG_LIVEPATCH
> -static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long
> ip)
> {
> + struct pt_regs *regs = ftrace_get_regs(fregs);
> +
> regs->nip = ip;
> }
>
> diff --git a/arch/s390/include/asm/livepatch.h
> b/arch/s390/include/asm/livepatch.h
> index 818612b784cd..d578a8c76676 100644
> --- a/arch/s390/include/asm/livepatch.h
> +++ b/arch/s390/include/asm/livepatch.h
> @@ -11,10 +11,13 @@
> #ifndef ASM_LIVEPATCH_H
> #define ASM_LIVEPATCH_H
>
> +#include
> #include
>
> -static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long
> ip)
> {
> + struct pt_regs *regs = ftrace_get_regs(fregs);
> +
> regs->psw.addr = ip;
> }
>
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index 6b175c2468e6..7042e80941e5 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -62,6 +62,9 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
> return NULL;
> return >regs;
> }
> +
> +#define ftrace_regs_set_ip(fregs, _ip) \
> + do { (fregs)->regs.ip = (_ip); } while (0)
> #endif
>
> #endif /* CONFIG_DYNAMIC_FTRACE */
> diff --git a/arch/x86/include/asm/livepatch.h
> b/arch/x86/include/asm/livepatch.h
> index 1fde1ab6559e..59a08d5c6f1d 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -12,9 +12,9 @@
> #include
> #include
>
> -static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long
> ip)
> {
> - regs->ip = ip;
> + ftrace_regs_set_ip(fregs, ip);
> }
>
> #endif /* _ASM_X86_LIVEPATCH_H */
> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index c4177bd00cd2..d00806dd8eed 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -157,6 +157,10 @@ SYM_INNER_LABEL(ftrace_caller_op_ptr, SYM_L_GLOBAL)
> SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
> call ftrace_stub
>
> + /* Handlers can change the RIP */
> + movq RIP(%rsp), %rax
> + movq %rax, MCOUNT_REG_SIZE(%rsp)
> +
> restore_mcount_regs
>
> /*
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 588ea7023a7a..b4eb962e2be9 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -97,6 +97,13 @@ struct ftrace_regs {
> };
> #define arch_ftrace_get_regs(fregs) (&(freg
Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback
> > > > + bit = ftrace_test_recursion_trylock(); > > > > + if (bit < 0) > > > > + return; > > > > > > This means that the original function will be called in case of > > > recursion. > > > That's probably fair, but I'm wondering if we should at least WARN about > > > it. > > > > Yeah, the early return might break the consistency model and > > unexpected things might happen. We should be aware of it. > > Please use: > > > > if (WARN_ON_ONCE(bit < 0)) > > return; > > > > WARN_ON_ONCE() might be part of the recursion. But it should happen > > only once. IMHO, it is worth the risk. > > > > Otherwise it looks good. > > Perhaps we can add that as a separate patch, because this patch doesn't add > any real functionality change. It only moves the recursion testing from the > helper function (which ftrace wraps all callbacks that do not have the > RECURSION flags set, including this one) down to your callback. > > In keeping with one patch to do one thing principle, the added of > WARN_ON_ONCE() should be a separate patch, as that will change the > functionality. > > If that WARN_ON_ONCE() breaks things, I'd like it to be bisected to another > patch other than this one. Works for me. Miroslav
Re: [PATCH 1/9] ftrace: Move the recursion testing into global headers
Hi,
> +static __always_inline int trace_get_context_bit(void)
> +{
> + int bit;
> +
> + if (in_interrupt()) {
> + if (in_nmi())
> + bit = 0;
> +
> + else if (in_irq())
> + bit = 1;
> + else
> + bit = 2;
> + } else
> + bit = 3;
> +
> + return bit;
> +}
> +
> +static __always_inline int trace_test_and_set_recursion(int start, int max)
> +{
> + unsigned int val = current->trace_recursion;
> + int bit;
> +
> + /* A previous recursion check was made */
> + if ((val & TRACE_CONTEXT_MASK) > max)
> + return 0;
> +
> + bit = trace_get_context_bit() + start;
> + if (unlikely(val & (1 << bit)))
> + return -1;
> +
> + val |= 1 << bit;
> + current->trace_recursion = val;
> + barrier();
> +
> + return bit;
> +}
how does this work in case of NMI? trace_get_context_bit() returns 0 (it
does not change later in the patch set). "start" in
trace_test_and_set_recursion() is 0 zero too as used later in the patch
set by ftrace_test_recursion_trylock(). So trace_test_and_set_recursion()
returns 0. That is perfectly sane, but then...
> +static __always_inline void trace_clear_recursion(int bit)
> +{
> + unsigned int val = current->trace_recursion;
> +
> + if (!bit)
> + return;
... the bit is not cleared here.
> + bit = 1 << bit;
> + val &= ~bit;
> +
> + barrier();
> + current->trace_recursion = val;
> +}
Thanks
Miroslav
Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback
On Thu, 29 Oct 2020, Petr Mladek wrote: > On Thu 2020-10-29 14:51:06, Miroslav Benes wrote: > > On Wed, 28 Oct 2020, Steven Rostedt wrote: > > > Hm, I've always thought that we did not need any kind of recursion > > protection for our callback. It is marked as notrace and it does not call > > anything traceable. In fact, it does not call anything. I even have a note > > in my todo list to mark the callback as RECURSION_SAFE :) > > Well, it calls WARN_ON_ONCE() ;-) Oh my, I learned to ignore these. Of course there is printk hidden everywhere. > > At the same time, it probably does not hurt and the patch is still better > > than what we have now without RECURSION_SAFE if I understand the patch set > > correctly. > > And better be on the safe side. Agreed. > > > Cc: Josh Poimboeuf > > > Cc: Jiri Kosina > > > Cc: Miroslav Benes > > > Cc: Petr Mladek > > > Cc: Joe Lawrence > > > Cc: live-patch...@vger.kernel.org > > > Signed-off-by: Steven Rostedt (VMware) > > > --- > > > kernel/livepatch/patch.c | 5 + > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c > > > index b552cf2d85f8..6c0164d24bbd 100644 > > > --- a/kernel/livepatch/patch.c > > > +++ b/kernel/livepatch/patch.c > > > @@ -45,9 +45,13 @@ static void notrace klp_ftrace_handler(unsigned long > > > ip, > > > struct klp_ops *ops; > > > struct klp_func *func; > > > int patch_state; > > > + int bit; > > > > > > ops = container_of(fops, struct klp_ops, fops); > > > > > > + bit = ftrace_test_recursion_trylock(); > > > + if (bit < 0) > > > + return; > > > > This means that the original function will be called in case of recursion. > > That's probably fair, but I'm wondering if we should at least WARN about > > it. > > Yeah, the early return might break the consistency model and > unexpected things might happen. We should be aware of it. > Please use: > > if (WARN_ON_ONCE(bit < 0)) > return; > > WARN_ON_ONCE() might be part of the recursion. But it should happen > only once. IMHO, it is worth the risk. Agreed. Miroslav
Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback
On Wed, 28 Oct 2020, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > If a ftrace callback does not supply its own recursion protection and > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will > make a helper trampoline to do so before calling the callback instead of > just calling the callback directly. > > The default for ftrace_ops is going to assume recursion protection unless > otherwise specified. Hm, I've always thought that we did not need any kind of recursion protection for our callback. It is marked as notrace and it does not call anything traceable. In fact, it does not call anything. I even have a note in my todo list to mark the callback as RECURSION_SAFE :) At the same time, it probably does not hurt and the patch is still better than what we have now without RECURSION_SAFE if I understand the patch set correctly. > Cc: Josh Poimboeuf > Cc: Jiri Kosina > Cc: Miroslav Benes > Cc: Petr Mladek > Cc: Joe Lawrence > Cc: live-patch...@vger.kernel.org > Signed-off-by: Steven Rostedt (VMware) > --- > kernel/livepatch/patch.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c > index b552cf2d85f8..6c0164d24bbd 100644 > --- a/kernel/livepatch/patch.c > +++ b/kernel/livepatch/patch.c > @@ -45,9 +45,13 @@ static void notrace klp_ftrace_handler(unsigned long ip, > struct klp_ops *ops; > struct klp_func *func; > int patch_state; > + int bit; > > ops = container_of(fops, struct klp_ops, fops); > > + bit = ftrace_test_recursion_trylock(); > + if (bit < 0) > + return; This means that the original function will be called in case of recursion. That's probably fair, but I'm wondering if we should at least WARN about it. Thanks Miroslav
Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load
On Wed, 28 Oct 2020, Jessica Yu wrote: > +++ Miroslav Benes [27/10/20 15:03 +0100]: > >If a module fails to load due to an error in prepare_coming_module(), > >the following error handling in load_module() runs with > >MODULE_STATE_COMING in module's state. Fix it by correctly setting > >MODULE_STATE_GOING under "bug_cleanup" label. > > > >Signed-off-by: Miroslav Benes > >--- > > kernel/module.c | 1 + > > 1 file changed, 1 insertion(+) > > > >diff --git a/kernel/module.c b/kernel/module.c > >index a4fa44a652a7..b34235082394 100644 > >--- a/kernel/module.c > >+++ b/kernel/module.c > >@@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const > >char __user *uargs, > > MODULE_STATE_GOING, mod); > > klp_module_going(mod); > > bug_cleanup: > >+mod->state = MODULE_STATE_GOING; > > /* module_bug_cleanup needs module_mutex protection */ > > mutex_lock(_mutex); > > module_bug_cleanup(mod); > > Thanks for the fix! Hmm, I am wondering if we also need to set the > module to GOING if it happens to fail while it is still UNFORMED. > > Currently, when a module is UNFORMED and encounters an error during > load_module(), it stays UNFORMED until it finally goes away. That > sounds fine, but try_module_get() technically permits you to get a > module while it's UNFORMED (but not if it's GOING). Theoretically > someone could increase the refcount of an unformed module that has > encountered an error condition and is in the process of going away. Right. > This shouldn't happen if we properly set the module to GOING whenever > it encounters an error during load_module(). That's correct. > But - I cannot think of a scenario where someone could call > try_module_get() on an unformed module, since find_module() etc. do > not return unformed modules, so they shouldn't be visible outside of > the module loader. So in practice, I think we're probably safe here.. Hopefully yes. I haven't found anything that would contradict it. I think it is even safer to leave UNFORMED there. free_module() explicitly sets UNFORMED state too while going through the similar process. ftrace_release_mod() is the only inconsistency there. It is called with UNFORMED in load_module() if going through ddebug_cleanup label directly, and with GOING in both do_init_module() before free_module() is called and delete_module syscall. But it probably does not care. Miroslav
Re: [PATCH] Documentation: livepatch: document reliable stacktrace
Hi, On Fri, 23 Oct 2020, Mark Rutland wrote: > Add documentation for reliable stacktrace. This is intended to describe > the semantics and to be an aid for implementing architecture support for > HAVE_RELIABLE_STACKTRACE. thanks a lot for doing the work! > Unwinding is a subtle area, and architectures vary greatly in both > implementation and the set of concerns that affect them, so I've tried > to avoid making this too specific to any given architecture. I've used > examples from both x86_64 and arm64 to explain corner cases in more > detail, but I've tried to keep the descriptions sufficient for those who > are unfamiliar with the particular architecture. Yes, I think it is a good approach. We can always add more details later, but it would probably cause more confusion for those unfamiliar. > I've tried to give rationale for all the recommendations/requirements, > since that makes it easier to spot nearby issues, or when a check > happens to catch a few things at once. I believe what I have written is > sound, but as some of this was reverse-engineered I may have missed > things worth noting. > > I've made a few assumptions about preferred behaviour, notably: > > * If you can reliably unwind through exceptions, you should (as x86_64 > does). Yes, it does. I think (and Josh will correct me if I am wrong here), that even at the beginning the intention was to improve the reliability of unwinding in general. Both x86_64 and s390x are the case. _reliable() interface only takes an advantage of that. As you pointed out in the document, unwinding through exceptions is not necessary. It can be reported as unreliable and we can deal with that later. But it is always better to do it if possible. powerpc is an exception to the approach, because it implements its _reliable() API from the scratch. > * It's fine to omit ftrace_return_to_handler and other return > trampolines so long as these are not subject to patching and the > original return address is reported. Most architectures do this for > ftrace_return_handler, but not other return trampolines. Yes. Patching a trampoline is not something I can imagine, so that should not be a problem. But one never knows and we may run into a problem here easily. I don't remember if we even audited all the trampolines. And new ones are introduced all the time. > * For cases where link register unreliability could result in duplicate > entries in the trace or an inverted trace, I've assumed this should be > treated as unreliable. This specific case shouldn't matter to > livepatching, but I assume that that we want a reliable trace to have > the correct order. Agreed. Thanks Miroslav
[PATCH] module: set MODULE_STATE_GOING state when a module fails to load
If a module fails to load due to an error in prepare_coming_module(), the following error handling in load_module() runs with MODULE_STATE_COMING in module's state. Fix it by correctly setting MODULE_STATE_GOING under "bug_cleanup" label. Signed-off-by: Miroslav Benes --- kernel/module.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/module.c b/kernel/module.c index a4fa44a652a7..b34235082394 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const char __user *uargs, MODULE_STATE_GOING, mod); klp_module_going(mod); bug_cleanup: + mod->state = MODULE_STATE_GOING; /* module_bug_cleanup needs module_mutex protection */ mutex_lock(_mutex); module_bug_cleanup(mod); -- 2.29.0
Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL
On Sat, 10 Oct 2020, Jens Axboe wrote:
> On 10/9/20 9:21 AM, Jens Axboe wrote:
> > On 10/9/20 2:01 AM, Miroslav Benes wrote:
> >> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
> >>
> >>> On 10/05, Jens Axboe wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
> >>>> from real signals and signal delivery.
> >>>
> >>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
> >>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
> >>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
> >>>
> >>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
> >>> set_notify_signal() rather than signal_wake_up().
> >>
> >> Yes, that was my impression from the patch set too, when I accidentally
> >> noticed it.
> >>
> >> Jens, could you CC our live patching ML when you submit v4, please? It
> >> would be a nice cleanup.
> >
> > Definitely, though it'd be v5 at this point. But we really need to get
> > all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
> > a whole slew of cleanups that'll fall out naturally:
> >
> > - Removal of JOBCTL_TASK_WORK
> > - Removal of special path for TWA_SIGNAL in task_work
> > - TIF_PATCH_PENDING can be converted and then removed
> > - try_to_freeze() cleanup that Oleg mentioned
> >
> > And probably more I'm not thinking of right now :-)
>
> Here's the current series, I took a stab at converting all archs to
> support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
> of them were straight forward, but I need someone to fixup powerpc,
> verify arm and s390.
>
> But it's a decent start I think, and means that we can drop various
> bits as is done at the end of the series. I could swap things around
> a bit and avoid having the intermediate step, but I envision that
> getting this in all archs will take a bit longer than just signing off
> on the generic/x86 bits. So probably best to keep the series as it is
> for now, and work on getting the arch bits verified/fixed/tested.
>
> https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work
Thanks, Jens.
Crude diff for live patching on top of the series is below. Tested only on
x86_64, but it passes the tests without an issue.
Miroslav
---
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include "core.h"
#include "patch.h"
#include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
* Send fake signal to all non-kthread tasks which are
* still not migrated.
*/
- spin_lock_irq(>sighand->siglock);
- signal_wake_up(task, 0);
- spin_unlock_irq(>sighand->siglock);
+ set_notify_signal(task);
}
}
read_unlock(_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index a15c584a0455..b7cf4eda8611 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
void recalc_sigpending(void)
{
- if (!recalc_sigpending_tsk(current) && !freezing(current) &&
- !klp_patch_pending(current))
+ if (!recalc_sigpending_tsk(current) && !freezing(current))
clear_thread_flag(TIF_SIGPENDING);
}
Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL
On Thu, 8 Oct 2020, Oleg Nesterov wrote: > On 10/05, Jens Axboe wrote: > > > > Hi, > > > > The goal is this patch series is to decouple TWA_SIGNAL based task_work > > from real signals and signal delivery. > > I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move > try_to_freeze() from get_signal() to tracehook_notify_signal(), kill > fake_signal_wake_up(), and remove freezing() from recalc_sigpending(). > > Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use > set_notify_signal() rather than signal_wake_up(). Yes, that was my impression from the patch set too, when I accidentally noticed it. Jens, could you CC our live patching ML when you submit v4, please? It would be a nice cleanup. Thanks Miroslav
Re: [PATCH v4 04/29] objtool: Add a pass for generating __mcount_loc
On Fri, 2 Oct 2020, Josh Poimboeuf wrote: > On Thu, Oct 01, 2020 at 03:36:12PM +0200, Peter Zijlstra wrote: > > On Thu, Oct 01, 2020 at 03:17:07PM +0200, Miroslav Benes wrote: > > > > > I also wonder about making 'mcount' command separate from 'check'. > > > Similar > > > to what is 'orc' now. But that could be done later. > > > > I'm not convinced more commands make sense. That only begets us the > > problem of having to run multiple commands. > > Agreed, it gets hairy when we need to combine things. I think "orc" as > a separate subcommand was a mistake. > > We should change to something like > > objtool run [--check] [--orc] [--mcount] > objtool dump [--orc] [--mcount] Yes, that makes sense. Miroslav
Re: [PATCH v4 04/29] objtool: Add a pass for generating __mcount_loc
Hi Sami, On Tue, 29 Sep 2020, Sami Tolvanen wrote: > From: Peter Zijlstra > > Add the --mcount option for generating __mcount_loc sections > needed for dynamic ftrace. Using this pass requires the kernel to > be compiled with -mfentry and CC_USING_NOP_MCOUNT to be defined > in Makefile. > > Link: > https://lore.kernel.org/lkml/20200625200235.gq4...@hirez.programming.kicks-ass.net/ > Signed-off-by: Peter Zijlstra > [Sami: rebased to mainline, dropped config changes, fixed to actually use >--mcount, and wrote a commit message.] > Signed-off-by: Sami Tolvanen > Reviewed-by: Kees Cook I am sorry to reply on v4. Should have been sooner. Julien has been sending patches to make objtool's check functionality arch-agnostic as much as possible. So it seems to me that the patch should be based on the effort I also wonder about making 'mcount' command separate from 'check'. Similar to what is 'orc' now. But that could be done later. See tip-tree/objtool/core for both. Thanks Miroslav
Re: [PATCH v5 00/10] Function Granular KASLR
Hi Kristen, On Wed, 23 Sep 2020, Kristen Carlson Accardi wrote: > Function Granular Kernel Address Space Layout Randomization (fgkaslr) > - > > This patch set is an implementation of finer grained kernel address space > randomization. It rearranges your kernel code at load time > on a per-function level granularity, with only around a second added to > boot time. I ran live patching kernel selftests on the patch set and everything passed fine. However, we also use not-yet-upstream set of tests at SUSE for testing live patching [1] and one of them, klp_tc_12.sh, is failing. You should be able to run the set on upstream as is. The test uninterruptedly sleeps in a kretprobed function called by a patched one. The current master without fgkaslr patch set reports the stack of the sleeping task as unreliable and live patching fails. The situation is different with fgkaslr (even with nofgkaslr on the command line). The stack is returned as reliable. It looks something like [<0>] __schedule+0x465/0xa40 [<0>] schedule+0x55/0xd0 [<0>] orig_do_sleep+0xb1/0x110 [klp_test_support_mod] [<0>] swap_pages+0x7f/0x7f where the last entry is not reliable. I've seen kretprobe_trampoline+0x0/0x4a and some other symbols there too. Since the patched function (orig_sleep_uninterruptible_set) is not on the stack, live patching succeeds, which is not intended. With kprobe setting removed, all works as expected. So I wonder if there is still some issue with ORC somewhere as you mentioned in v4 thread. I'll investigate more next week, but wanted to report early. Regards Miroslav [1] https://github.com/lpechacek/qa_test_klp
Re: [PATCH v5 10/10] livepatch: only match unique symbols when using fgkaslr
Hi,
On Wed, 23 Sep 2020, Kristen Carlson Accardi wrote:
> If any type of function granular randomization is enabled, the sympos
> algorithm will fail, as it will be impossible to resolve symbols when
> there are duplicates using the previous symbol position.
>
> Override the value of sympos to always be zero if fgkaslr is enabled for
> either the core kernel or modules, forcing the algorithm
> to require that only unique symbols are allowed to be patched.
>
> Signed-off-by: Kristen Carlson Accardi
> ---
> kernel/livepatch/core.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index f76fdb925532..da08e40f2da2 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -170,6 +170,17 @@ static int klp_find_object_symbol(const char *objname,
> const char *name,
> kallsyms_on_each_symbol(klp_find_callback, );
> mutex_unlock(_mutex);
>
> + /*
> + * If any type of function granular randomization is enabled, it
> + * will be impossible to resolve symbols when there are duplicates
> + * using the previous symbol position (i.e. sympos != 0). Override
> + * the value of sympos to always be zero in this case. This will
> + * force the algorithm to require that only unique symbols are
> + * allowed to be patched.
> + */
> + if (IS_ENABLED(CONFIG_FG_KASLR) || IS_ENABLED(CONFIG_MODULE_FG_KASLR))
> + sympos = 0;
This should work, but I wonder if we should make it more explicit. With
the change the user will get the error with "unresolvable ambiguity for
symbol..." if they specify sympos and the symbol is not unique. It could
confuse them.
So, how about it making it something like
if (IS_ENABLED(CONFIG_FG_KASLR) || IS_ENABLED(CONFIG_MODULE_FG_KASLR))
if (sympos) {
pr_err("fgkaslr is enabled, specifying sympos for symbol '%s'
in object '%s' does not work.\n",
name, objname);
*addr = 0;
return -EINVAL;
}
? (there could be goto to the error out at the end of the function).
In that case, if sympos is not specified, the user will get the message
which matches the reality. If the user specifies it, they will get the
error in case of fgkaslr.
Thanks for dealing with it
Miroslav
Re: [PATCH v3 0/3] arm64: Convert to ARCH_STACKWALK
On Mon, 14 Sep 2020, Mark Brown wrote: > This series updates the arm64 stacktrace code to use the newer and much > simpler arch_stack_walk() interface, the main benefit being a single > entry point to the arch code with no need for the arch code to worry > about skipping frames. Along the way I noticed that the reliable > parameter to the arch_stack_walk() callback appears to be redundant > so there's also a patch here removing that from the existing code to > simplify the interface. > > This is preparatory work for implementing reliable stack trace for > arm64. > > v3: > - Rebase onto v5.9-rc3. > - Fix handling of task == current. > - Flip the sense of the walk_stackframe() callback. > v2: > - Rebase onto v5.9-rc1. > > Mark Brown (3): > stacktrace: Remove reliable argument from arch_stack_walk() callback > arm64: stacktrace: Make stack walk callback consistent with generic > code > arm64: stacktrace: Convert to ARCH_STACKWALK The patches look good to me. Reviewed-by: Miroslav Benes M
Re: [PATCH v3 00/10] Make check implementation arch agnostic
On Fri, 4 Sep 2020, Julien Thierry wrote: > Hi, > > The current implementation of the check subcommand has various x86 bits > here and there. In order to prepare objtool to provide check for other > architectures, add some abstraction over the x86 specific bits, relying > on objtool arch specific code to provide some necessary operations. > > This is part of the effort to implement check for arm64, initiated [1] > by Raphael. The series is based on top of the separation of check & orc > subcommands series[2]. > > I've push both series base on top of tip/objtool/core at [3]. > > - The first two patches make it simpler for new arches to provide their > list of kernel headers, without worrying about modifications in the x86 > headers. > - Patch 3 Moves arch specific macros to more suitable location > - Patches 4 and 5 add abstraction to handle alternatives > - Patch 6 adds abstraction to handle jump table > - Patches 7-10 makes unwind hint definitions shared across architectures > > Changes since v2 [4]: > - Rebased on v5.9-rc1 > - Under tools/objtool/arch/x86/, rename arch_special.c to special.c > - Rename include/linux/frame.h to inclide/linux/objtool.h > - Share unwind hint types across architectures > > [1] https://lkml.org/lkml/2019/8/16/400 > [2] https://lkml.org/lkml/2020/6/4/675 > [3] https://github.com/julien-thierry/linux/tree/arch-independent-check > [4] https://lkml.org/lkml/2020/7/30/424 Hi, Josh merged the patch set already, but FWIW Reviewed-by: Miroslav Benes for the new changes (patches 7, 9 and 10). Miroslav
Re: [PATCH] objtool: support symtab_shndx during dump
On Thu, 3 Sep 2020, Josh Poimboeuf wrote:
> On Wed, Aug 12, 2020 at 10:57:11AM -0700, Kristen Carlson Accardi wrote:
>
> > if (GELF_ST_TYPE(sym.st_info) == STT_SECTION) {
> > - scn = elf_getscn(elf, sym.st_shndx);
> > + if ((sym.st_shndx > SHN_UNDEF &&
> > +sym.st_shndx < SHN_LORESERVE) ||
> > + (xsymtab && sym.st_shndx == SHN_XINDEX)) {
> > + if (sym.st_shndx != SHN_XINDEX)
> > + shndx = sym.st_shndx;
>
> The sym.st_shndx checks are redundant, if 'sym.st_shndx == SHN_XINDEX'
> then 'sym.st_shndx != SHN_XINDEX' can't be true.
It is probably a copy-paste from read_symbols() in elf.c, where the logic
is different.
> Actually I think this can be even further simplified to something like
>
> if (!shndx)
> shndx = sym.st_shndx;
This relies on the fact that gelf_getsymshndx() always initializes shndx,
no? I think it would be better to initialize it in orc_dump() too. Safer
and easier to read. It applies to Kristen's patch as well. I missed that.
Miroslav
Re: [PATCH] objtool: support symtab_shndx during dump
On Wed, 12 Aug 2020, Kristen Carlson Accardi wrote: > When getting the symbol index number, make sure to use the > extended symbol table information in order to support symbol > index's greater than 64K. > > Signed-off-by: Kristen Carlson Accardi Reviewed-by: Miroslav Benes M
Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc
Hi,
first, I'm sorry for the late reply. Thanks, Josh, for the reminder.
CCing Nicolai. Nicolai, could you take a look at the proposed
documentation too, please? You have more up-to-date experience.
On Tue, 21 Jul 2020, Joe Lawrence wrote:
> +Examples
> +
> +
> +Interprocedural optimization (IPA)
> +--
> +
> +Function inlining is probably the most common compiler optimization that
> +affects livepatching. In a simple example, inlining transforms the original
> +code::
> +
> + foo() { ... [ foo implementation ] ... }
> +
> + bar() { ... foo() ... }
> +
> +to::
> +
> + bar() { ... [ foo implementation ] ... }
> +
> +Inlining is comparable to macro expansion, however the compiler may inline
> +cases which it determines worthwhile (while preserving original call/return
> +semantics in others) or even partially inline pieces of functions (see cold
> +functions in GCC function suffixes section below).
> +
> +To safely livepatch ``foo()`` from the previous example, all of its callers
> +need to be taken into consideration. For those callers that the compiler had
> +inlined ``foo()``, a livepatch should include a new version of the calling
> +function such that it:
> +
> + 1. Calls a new, patched version of the inlined function, or
> + 2. Provides an updated version of the caller that contains its own inlined
> + and updated version of the inlined function
I'm afraid the above could cause a confusion...
"1. Calls a new, patched version of the inlined function, or". The
function is not inlined in this case. Would it be more understandable to
use function names?
1. Calls a new, patched version of function foo(), or
2. Provides an updated version of bar() that contains its own inlined and
updated version of foo() (as seen in the example above).
Not to say that it is again a call of the compiler to decide that, so one
usually prepares an updated version of foo() and updated version of bar()
calling to it. Updated foo() has to be there for non-inlined cases anyway.
> +
> +Other interesting IPA examples include:
> +
> +- *IPA-SRA*: removal of unused parameters, replace parameters passed by
> + referenced by parameters passed by value. This optimization basically
s/referenced/reference/
> + violates ABI.
> +
> + .. note::
> + GCC changes the name of function. See GCC function suffixes
> + section below.
> +
> +- *IPA-CP*: find values passed to functions are constants and then optimizes
> + accordingly Several clones of a function are possible if a set is limited.
"...accordingly. Several..."
[...]
> + void cdev_put(struct cdev *p)
> + {
> + if (p) {
> + struct module *owner = p->owner;
> + kobject_put(>kobj);
> + module_put(owner);
> + }
> + }
git am complained here about whitespace damage.
[...]
> +kgraft-analysis-tool
> +
> +
> +With the -fdump-ipa-clones flag, GCC will dump IPA clones that were created
> +by all inter-procedural optimizations in ``.000i.ipa-clones`` files.
> +
> +kgraft-analysis-tool pretty-prints those IPA cloning decisions. The full
> +list of affected functions provides additional updates that the source-based
> +livepatch author may need to consider. For example, for the function
> +``scatterwalk_unmap()``:
> +
> +::
> +
> + $ ./kgraft-ipa-analysis.py --symbol=scatterwalk_unmap
> aesni-intel_glue.i.000i.ipa-clones
> + Function: scatterwalk_unmap/2930 (include/crypto/scatterwalk.h:81:60)
> +isra: scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> + inlining to: helper_rfc4106_decrypt/3007
> (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> + inlining to: helper_rfc4106_decrypt/3007
> (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> + inlining to: helper_rfc4106_encrypt/3006
> (arch/x86/crypto/aesni-intel_glue.c:939:12)
> +
> +Affected functions: 3
> + scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> + helper_rfc4106_decrypt/3007
> (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> + helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)
The example for the github is not up-to-date. The tool now expects
file_list with *.ipa-clones files and the output is a bit different for
the recent kernel.
$ echo arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones |
kgraft-ipa-analysis.py --symbol=scatterwalk_unmap /dev/stdin
Parsing file (1/1): arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones
Function: scatterwalk_unmap/3935 (./include/crypto/scatterwalk.h:59:20)
[REMOVED] [object file: arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones]
isra: scatterwalk_unmap.isra.8/4117 (./include/crypto/scatterwalk.h:59:20)
[REMOVED]
inlining to: gcmaes_crypt_by_sg/4019
(arch/x86/crypto/aesni-intel_glue.c:682:12) [REMOVED] [edges: 4]
constprop: gcmaes_crypt_by_sg.constprop.13/4182
Re: refactoring livepatch documentation was Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc
[side note: So not only that my INBOX is a mess after the summer. I also lost some emails apparently. I'm really sorry about that. ] CCing Nicolai too. > Hi Petr, Josh, > > The compiler optimization pitfall document can wait for refactored livepatch > documentation if that puts it into better context, particularly for newbies. > I don't mind either way. FWIW, I don't profess to be an authoritative source > its content -- we've dealt some of these issues in kpatch, so it was > interesting to see how they affect livepatches that don't rely on binary > comparison. > > > Toward the larger goal, I've changed the thread subject to talk about how we > may rearrange and supplement our current documentation. This is a first pass > at a possible refactoring... > > > 1. Provide a better index page to connect the other files/docs, like > https://www.kernel.org/doc/html/latest/core-api/index.html but obviously not > that extensive. Right now we have only a Table of Contents tree without any > commentary. > > 2. Rearrange and refactor sections: > > livepatch.rst > Keep just about everything > Add a history section to explain ksplice, kgraft, kpatch for the > uninitiated? > Add a section on source based vs. binary diff livepatch creation, > this may be worth its own top-level section > > Livepatch API > Basic API > Callbacks > Shadow variables > Cumulative patches > System state > > KLP Relocations > Right now this is a bit academic AFAIK kpatch is the only tool > currently making use of them. So maybe this document becomes a > more general purpose doc explaining how to reference unexported > symbols? (ie, how does kgraft currently do it, particularly > w/kallsyms going unexported?) Yes, we rely on kallsyms_lookup_name() pretty much right now and once we hit the problem with the next kernel version upgrade, we'll have to fix it. > Eventually this could contain klp-convert howto if it ever gets > merged. > > Compiler considerations > TBD > > I suppose this doesn't create a "Livepatching creation for dummies" guide, but > my feeling is that there are so many potential (hidden) pitfalls that such > guide would be dangerous. It does not create the guide, but it looks like a good basis. I agree with Josh here. It might be difficult at the beginning, but the outcome could be great even for a newbie and I think we should aim for that. > If someone were to ask me today how to start building a livepatch, I would > probably point them at the samples to demonstrate the basic concept and API, > but then implore them to read through the documentation to understand how > quickly complicated it can become. True. We discuss the need to properly document our internal process every once in a while and there is always something more important to deal with, but it is high time to finally start with that. Miroslav
Re: [PATCH 2/2] samples/livepatch: Add README.rst disclaimer
On Tue, 21 Jul 2020, Joe Lawrence wrote: > The livepatch samples aren't very careful with respect to compiler > IPA-optimization of target kernel functions. > > Add a quick disclaimer and pointer to the compiler-considerations.rst > file to warn readers. > > Suggested-by: Josh Poimboeuf > Signed-off-by: Joe Lawrence Acked-by: Miroslav Benes M
Re: [PATCH v2 3/3] arm64: stacktrace: Convert to ARCH_STACKWALK
Hi, it could be a silly question, but better to ask... > + if (regs) > + start_backtrace(, regs->regs[29], regs->pc); > + else > + start_backtrace(, thread_saved_fp(task), > + thread_saved_pc(task)); Would this also work for task == current? Given that the original code had > - start_backtrace(, > - (unsigned long)__builtin_frame_address(0), > - (unsigned long)__save_stack_trace); for the case, which seems correct (but I don't know much about arm64 arch in the kernel). Otherwise, I did not spot anything suspicious or wrong. Regards Miroslav
Re: [PATCH v2 2/3] arm64: stacktrace: Make stack walk callback consistent with generic code
On Wed, 19 Aug 2020, Mark Brown wrote: > As with the generic arch_stack_walk() code the arm64 stack walk code takes > a callback that is called per stack frame. Currently the arm64 code always > passes a struct stackframe to the callback and the generic code just passes > the pc, however none of the users ever reference anything in the struct > other than the pc value. The arm64 code also uses a return type of int > while the generic code uses a return type of bool though in both cases the > return value is a boolean value. > > In order to reduce code duplication when arm64 is converted to use > arch_stack_walk() change the signature of the arm64 specific callback to > match that of the generic code. > > Signed-off-by: Mark Brown Reviewed-by: Miroslav Benes M
Re: [PATCH v2 1/3] stacktrace: Remove reliable argument from arch_stack_walk() callback
On Wed, 19 Aug 2020, Mark Brown wrote: > Currently the callback passed to arch_stack_walk() has an argument called > reliable passed to it to indicate if the stack entry is reliable, a comment > says that this is used by some printk() consumers. However in the current > kernel none of the arch_stack_walk() implementations ever set this flag to > true and the only callback implementation we have is in the generic > stacktrace code which ignores the flag. It therefore appears that this > flag is redundant so we can simplify and clarify things by removing it. > > Signed-off-by: Mark Brown Reviewed-by: Miroslav Benes M
Re: [PATCH] ftrace: Free the trampoline when ftrace_startup() fails
I used Masami's address which did not work. Sorry about that. Should be ok
now.
On Mon, 31 Aug 2020, Miroslav Benes wrote:
> Commit fc0ea795f53c ("ftrace: Add symbols for ftrace trampolines")
> missed to remove ops from new ftrace_ops_trampoline_list in
> ftrace_startup() if ftrace_hash_ipmodify_enable() fails there. It may
> lead to BUG if such ops come from a module which may be removed.
>
> Moreover, the trampoline itself is not freed in this case.
>
> Fix it by calling ftrace_trampoline_free() during the rollback.
>
> Fixes: fc0ea795f53c ("ftrace: Add symbols for ftrace trampolines")
> Signed-off-by: Miroslav Benes
> ---
>
> It would be fair to add
>
> Fixes: f8b8be8a310a ("ftrace, kprobes: Support IPMODIFY flag to find IP
> modify conflict")
>
> too. The situation was a bit more complicated back then though since
> RCU-tasks support was missing.
>
> kernel/trace/ftrace.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 275441254bb5..656d7cb5a78c 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2862,6 +2862,8 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
> __unregister_ftrace_function(ops);
> ftrace_start_up--;
> ops->flags &= ~FTRACE_OPS_FL_ENABLED;
> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
> + ftrace_trampoline_free(ops);
> return ret;
> }
>
> --
> 2.28.0
>
[PATCH] ftrace: Free the trampoline when ftrace_startup() fails
Commit fc0ea795f53c ("ftrace: Add symbols for ftrace trampolines")
missed to remove ops from new ftrace_ops_trampoline_list in
ftrace_startup() if ftrace_hash_ipmodify_enable() fails there. It may
lead to BUG if such ops come from a module which may be removed.
Moreover, the trampoline itself is not freed in this case.
Fix it by calling ftrace_trampoline_free() during the rollback.
Fixes: fc0ea795f53c ("ftrace: Add symbols for ftrace trampolines")
Signed-off-by: Miroslav Benes
---
It would be fair to add
Fixes: f8b8be8a310a ("ftrace, kprobes: Support IPMODIFY flag to find IP modify
conflict")
too. The situation was a bit more complicated back then though since
RCU-tasks support was missing.
kernel/trace/ftrace.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 275441254bb5..656d7cb5a78c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2862,6 +2862,8 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
__unregister_ftrace_function(ops);
ftrace_start_up--;
ops->flags &= ~FTRACE_OPS_FL_ENABLED;
+ if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
+ ftrace_trampoline_free(ops);
return ret;
}
--
2.28.0
Re: [PATCH v4 00/10] Function Granular KASLR
Leaving Josh's proposals here for reference... > > I'm not sure how LTO does it, but a few more (half-brained) ideas > > that > > could work: > > > > 1) Add a field in kallsyms to keep track of a symbol's original > > offset > >before randomization/re-sorting. Livepatch could use that field > > to > >determine the original sympos. > > > > 2) In fgkaslr code, go through all the sections and mark the ones > > which > >have duplicates (i.e. same name). Then when shuffling the > > sections, > >skip a shuffle if it involves a duplicate section. That way all > > the > >duplicates would retain their original sympos. > > > > 3) Livepatch could uniquely identify symbols by some feature other > > than > >sympos. For example: > > > >Symbol/function size - obviously this would only work if > > duplicately > >named symbols have different sizes. > > > >Checksum - as part of a separate feature we're also looking at > > giving > >each function its own checksum, calculated based on its > > instruction > >opcodes. Though calculating checksums at runtime could be > >complicated by IP-relative addressing. > > > > I'm thinking #1 or #2 wouldn't be too bad. #3 might be harder. > > > > Hi there! I was trying to find a super easy way to address this, so I > thought the best thing would be if there were a compiler or linker > switch to just eliminate any duplicate symbols at compile time for > vmlinux. I filed this question on the binutils bugzilla looking to see > if there were existing flags that might do this, but H.J. Lu went ahead > and created a new one "-z unique", that seems to do what we would need > it to do. > > https://sourceware.org/bugzilla/show_bug.cgi?id=26391 > > When I use this option, it renames any duplicate symbols with an > extension - for example duplicatefunc.1 or duplicatefunc.2. You could > either match on the full unique name of the specific binary you are > trying to patch, or you match the base name and use the extension to > determine original position. Do you think this solution would work? Yes, I think so (thanks, Joe, for testing!). It looks cleaner to me than the options above, but it may just be a matter of taste. Anyway, I'd go with full name matching, because -z unique-symbol would allow us to remove sympos altogether, which is appealing. > If > so, I can modify livepatch to refuse to patch on duplicated symbols if > CONFIG_FG_KASLR and when this option is merged into the tool chain I > can add it to KBUILD_LDFLAGS when CONFIG_FG_KASLR and livepatching > should work in all cases. Ok. Josh, Petr, would this work for you too? Thanks Miroslav
Re: [PATCH v4 4/4] objtool: check: Use orc definition only when needed
On Tue, 25 Aug 2020, Julien Thierry wrote: > Implementation of ORC requires some definitions that are currently > provided by the target architecture headers. Do not depend on these > definitions when the orc subcommand is not implemented. > > This avoid requiring arches with no orc implementation to provide dummy > orc definitions. > > Signed-off-by: Julien Thierry This is definitely simpler than what v3 had. Reviewed-by: Miroslav Benes M
Re: [PATCH v4 2/4] objtool: Move orc outside of check
On Tue, 25 Aug 2020, Julien Thierry wrote: > Now that the objtool_file can be obtained outside of the check function, > orc generation builtin no longer requires check to explicitly call its > orc related functions. > > Signed-off-by: Julien Thierry Reviewed-by: Miroslav Benes M
[PATCH] selftests/livepatch: Do not check order when using "comm" for dmesg checking
check_result() uses "comm" to check expected results of selftests output
in dmesg. Everything works fine if timestamps in dmesg are unique. If
not, like in this example
[ 86.844422] test_klp_callbacks_demo: pre_unpatch_callback:
test_klp_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 86.844422] livepatch: 'test_klp_callbacks_demo': starting unpatching
transition
, "comm" fails with "comm: file 2 is not in sorted order". Suppress the
order checking with --nocheck-order option.
Signed-off-by: Miroslav Benes
---
The strange thing is, I can reproduce the issue easily and reliably on
older codestreams (4.12) but not on current upstream in my testing
environment. I think the change makes sense regardless though.
tools/testing/selftests/livepatch/functions.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/livepatch/functions.sh
b/tools/testing/selftests/livepatch/functions.sh
index 1aba83c87ad3..846c7ed71556 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -278,7 +278,7 @@ function check_result {
# help differentiate repeated testing runs. Remove them with a
# post-comparison sed filter.
- result=$(dmesg | comm -13 "$SAVED_DMESG" - | \
+ result=$(dmesg | comm --nocheck-order -13 "$SAVED_DMESG" - | \
grep -e 'livepatch:' -e 'test_klp' | \
grep -v '\(tainting\|taints\) kernel' | \
sed 's/^\[[ 0-9.]*\] //')
--
2.28.0
Ftrace regression in v5.9-rc1 due to commit fc0ea795f53c ("ftrace: Add symbols for ftrace trampolines")
Hi,
during v5.9-rc1 testing I ran into an issue (BUG dump at the end of the
email). I suspected commit fc0ea795f53c ("ftrace: Add symbols for ftrace
trampolines") (which git bisect later confirmed) and a missing call to
ftrace_remove_trampoline_from_kallsyms() somewhere. And indeed there is an
unhandled place in ftrace_startup(). __register_ftrace_function() creates
a trampoline and adds its ops to new ftrace_ops_trampoline_list. If
ftrace_hash_ipmodify_enable() fails, the ops is not removed from the list.
Moreover, the trampoline is not freed anywhere in this case if I am not
missing something, which is not a problem of the mentioned patch.
If that's the case, something like
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 275441254bb5..656d7cb5a78c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2862,6 +2862,8 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
__unregister_ftrace_function(ops);
ftrace_start_up--;
ops->flags &= ~FTRACE_OPS_FL_ENABLED;
+ if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
+ ftrace_trampoline_free(ops);
return ret;
}
could do the trick. At least it fixes the issue for me, but I tend to get
lost in ftrace code, so it might not be a good approach generally.
If no one sees a problem anywhere, I'll prepare a proper patch and will
run some more testing.
Regards
Miroslav
---
BUG: unable to handle page fault for address: fff0
#PF: supervisor read access in kernel mode
#PF: error_code(0x) - not-present page
PGD 2216067 P4D 2216067 PUD 2218067 PMD 0
Oops: [#1] SMP PTI
CPU: 1 PID: 2430 Comm: cat Tainted: G O K 5.9.0-rc2-dirty #44
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.13.0-48-gd9c812d-rebuilt.opensuse.org 04/01/2014
RIP: 0010:ftrace_mod_get_kallsym+0xd4/0x150
Code: 40 38 39 f8 0f 87 7b ff ff ff eb e1 0f 0b 4c 8b 0d 21 95 0d 01 b8 de ff
ff ff 49 81 f9 80 6d 29 82 4d 8d 81 60 fe ff ff 74 2e <49> 8b 80 90 01 00 00 48
85 c0 740
RSP: 0018:c9373e00 EFLAGS: 00010203
RAX: RBX: 888072952900 RCX: 888072952935
RDX: 888072952934 RSI: 888072952928 RDI:
RBP: 8880729529b5 R08: fe60 R09: 888037180fc0
R10: R11: ff6d59b3 R12: 8880729529f0
R13: 888072952928 R14: 888072952934 R15: 8880729529b5
FS: 7f543f339500() GS:88807d20() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: fff0 CR3: 7a09e000 CR4: 06e0
Call Trace:
update_iter+0x189/0x1e0
s_next+0x1f/0x30
seq_read+0x238/0x420
proc_reg_read+0x56/0x70
vfs_read+0xb7/0x1c0
ksys_read+0xa7/0xe0
do_syscall_64+0x33/0x80
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f543ee621a1
Code: fe ff ff 48 8d 3d 67 a1 09 00 48 83 ec 08 e8 e6 03 02 00 66 0f 1f 44 00
00 8b 05 0a d2 2c 00 48 63 ff 85 c0 75 13 31 c0 0f 05 <48> 3d 00 f0 ff ff 77 57
f3 c3 0f9
RSP: 002b:7ffd9b8691d8 EFLAGS: 0246 ORIG_RAX:
RAX: ffda RBX: 0002 RCX: 7f543ee621a1
RDX: 0002 RSI: 7f543f196000 RDI: 0003
RBP: 0002 R08: R09:
R10: 089f R11: 0246 R12: 7f543f196000
R13: 0003 R14: 0fe3 R15: 0002
Modules linked in: af_packet iscsi_ibft iscsi_boot_sysfs rfkill ppdev bochs_drm
drm_vram_helper drm_ttm_helper ttm drm_kms_helper joydev i2c_piix4 drm pcspkr
parport_]
CR2: fff0
---[ end trace 2ad43a0bbf68c2e2 ]---
RIP: 0010:ftrace_mod_get_kallsym+0xd4/0x150
Code: 40 38 39 f8 0f 87 7b ff ff ff eb e1 0f 0b 4c 8b 0d 21 95 0d 01 b8 de ff
ff ff 49 81 f9 80 6d 29 82 4d 8d 81 60 fe ff ff 74 2e <49> 8b 80 90 01 00 00 48
85 c0 740
RSP: 0018:c9373e00 EFLAGS: 00010203
RAX: RBX: 888072952900 RCX: 888072952935
RDX: 888072952934 RSI: 888072952928 RDI:
RBP: 8880729529b5 R08: fe60 R09: 888037180fc0
R10: R11: ff6d59b3 R12: 8880729529f0
R13: 888072952928 R14: 888072952934 R15: 8880729529b5
FS: 7f543f339500() GS:88807d20() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: fff0 CR3: 7a09e000 CR4: 06e0
note: cat[2430] exited with preempt_count 1
BUG: sleeping function called from invalid context at
./include/linux/percpu-rwsem.h:49
in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2430, name: cat
INFO: lockdep is turned off.
irq event stamp: 530598
hardirqs last enabled at (530597): []
syscall_enter_from_user_mode+0x20/0x250
hardirqs last disabled at (530598): []
irqentry_enter+0x1d/0x50
softirqs last enabled at (522266): []
__do_softirq+0x343/0x463
softirqs last disabled at (522259): []
Re: [PATCH v2 0/9] Make check implementation arch agnostic
> I've push both series base on top of tip/objtool/core at [3]. > > - The first two patches make it simpler for new arches to provide their > list of kernel headers, without worrying about modifications in the x86 > headers. > - Patch 3 Moves arch specific macros to more suitable location > - Patches 4 and 5 add abstraction to handle alternatives > - Patch 6 adds abstraction to handle jump table > - Patches 7-9 abstracts the use of unwind hints, so some definitions > can be shared across architectures while keeping arch specific > semantics > > Changes since v1 [4]: > - Rebased on recent tip/objtool/core > - Split the unwind hint rework into multiple patches as suggested by > Miroslav For remaining patches 7-9 Reviewed-by: Miroslav Benes M
Re: [PATCH v3 2/4] objtool: Move orc outside of check
On Thu, 30 Jul 2020, Julien Thierry wrote: > > > On 7/30/20 3:15 PM, Josh Poimboeuf wrote: > > On Thu, Jul 30, 2020 at 02:29:20PM +0100, Julien Thierry wrote: > >> > >> > >> On 7/30/20 2:22 PM, pet...@infradead.org wrote: > >>> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote: > > > On 7/30/20 10:57 AM, pet...@infradead.org wrote: > > On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote: > >> + if (file->elf->changed) > >> + return elf_write(file->elf); > >> + else > >> + return 0; > >> } > > > > I think we can do without that else :-) > > > > I did wonder and was not 100% confident about it, but the orc gen will > always change the file, correct? > >>> > >>> Not if it already has orc, iirc. > >>> > >>> But what I was trying to say is that: > >>> > >>> if (file->elf->changed) > >>> return elf_write(file->elf) > >>> > >>> return 0; > >>> > >>> is identical code and, IMO, easier to read. > >>> > >> > >> Much easier yes, I'll change it. > > > > But I think file->elf->changed can be assumed at this point anyway, so > > it could just be an unconditional > > > > return elf_write(file->elf); > > > > I'll triple check whether that's the case and remove the if if possible. I think it is the case. And even if not, it would only cause a pointless call to elf_update() in the end and that should not do any harm anyway if I am not mistaken. However, I think there is a problem with the rebase on top of the current code. The patch moves elf_write() call to orc_gen.c which was ok before Peterz introduced elf_write_insn() et al. We need to keep elf_write() in check.c for this case too. Miroslav
Re: [PATCH v4 00/10] Function Granular KASLR
Let me CC live-patching ML, because from a quick glance this is something which could impact live patching code. At least it invalidates assumptions which "sympos" is based on. Miroslav On Fri, 17 Jul 2020, Kristen Carlson Accardi wrote: > Function Granular Kernel Address Space Layout Randomization (fgkaslr) > - > > This patch set is an implementation of finer grained kernel address space > randomization. It rearranges your kernel code at load time > on a per-function level granularity, with only around a second added to > boot time. [...] > Background > -- > KASLR was merged into the kernel with the objective of increasing the > difficulty of code reuse attacks. Code reuse attacks reused existing code > snippets to get around existing memory protections. They exploit software bugs > which expose addresses of useful code snippets to control the flow of > execution for their own nefarious purposes. KASLR moves the entire kernel > code text as a unit at boot time in order to make addresses less predictable. > The order of the code within the segment is unchanged - only the base address > is shifted. There are a few shortcomings to this algorithm. > > 1. Low Entropy - there are only so many locations the kernel can fit in. This >means an attacker could guess without too much trouble. > 2. Knowledge of a single address can reveal the offset of the base address, >exposing all other locations for a published/known kernel image. > 3. Info leaks abound. > > Finer grained ASLR has been proposed as a way to make ASLR more resistant > to info leaks. It is not a new concept at all, and there are many variations > possible. Function reordering is an implementation of finer grained ASLR > which randomizes the layout of an address space on a function level > granularity. We use the term "fgkaslr" in this document to refer to the > technique of function reordering when used with KASLR, as well as finer > grained > KASLR in general. > > Proposed Improvement > > This patch set proposes adding function reordering on top of the existing > KASLR base address randomization. The over-arching objective is incremental > improvement over what we already have. It is designed to work in combination > with the existing solution. The implementation is really pretty simple, and > there are 2 main area where changes occur: > > * Build time > > GCC has had an option to place functions into individual .text sections for > many years now. This option can be used to implement function reordering at > load time. The final compiled vmlinux retains all the section headers, which > can be used to help find the address ranges of each function. Using this > information and an expanded table of relocation addresses, individual text > sections can be suffled immediately after decompression. Some data tables > inside the kernel that have assumptions about order require re-sorting > after being updated when applying relocations. In order to modify these > tables, > a few key symbols are excluded from the objcopy symbol stripping process for > use after shuffling the text segments. > > Some highlights from the build time changes to look for: > > The top level kernel Makefile was modified to add the gcc flag if it > is supported. Currently, I am applying this flag to everything it is > possible to randomize. Anything that is written in C and not present in a > special input section is randomized. The final binary segment 0 retains a > consolidated .text section, as well as all the individual .text.* sections. > Future work could turn off this flags for selected files or even entire > subsystems, although obviously at the cost of security. > > The relocs tool is updated to add relative relocations. This information > previously wasn't included because it wasn't necessary when moving the > entire .text segment as a unit. > > A new file was created to contain a list of symbols that objcopy should > keep. We use those symbols at load time as described below. > > * Load time > > The boot kernel was modified to parse the vmlinux elf file after > decompression to check for our interesting symbols that we kept, and to > look for any .text.* sections to randomize. The consolidated .text section > is skipped and not moved. The sections are shuffled randomly, and copied > into memory following the .text section in a new random order. The existing > code which updated relocation addresses was modified to account for > not just a fixed delta from the load address, but the offset that the function > section was moved to. This requires inspection of each address to see if > it was impacted by a randomization. We use a bsearch to make this less > horrible on performance. Any tables that need to be modified with new > addresses or resorted are updated using the symbol addresses parsed from the > elf symbol table. > > In order to hide our new
Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"
On Sun, 19 Jul 2020, Joe Lawrence wrote:
> On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
> > Use of the new -flive-patching flag was introduced with the following
> > commit:
> >
> >43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is
> >enabled")
> >
> > This flag has several drawbacks:
> >
> > [ ... snip ... ]
> >
> > - While there *is* a distro which relies on this flag for their distro
> >livepatch module builds, there's not a publicly documented way to
> >create safe livepatch modules with it. Its use seems to be based on
> >tribal knowledge. It serves no benefit to those who don't know how to
> >use it.
> >
> >(In fact, I believe the current livepatch documentation and samples
> >are misleading and dangerous, and should be corrected. Or at least
> >amended with a disclaimer. But I don't feel qualified to make such
> >changes.)
>
> FWIW, I'm not exactly qualified to document source-based creation either,
> however I have written a few of the samples and obviously the kselftest
> modules.
>
> The samples should certainly include a disclaimer (ie, they are only for API
> demonstration purposes!) and eventually it would be great if the kselftest
> modules could guarantee their safety as well. I don't know quite yet how we
> can automate that, but perhaps some kind of post-build sanity check could
> verify that they are in fact patching what they intend to patch.
That's a good idea. We should have something like that. I don't know how
to make it nice. Just horrible post-build hacks that would check that
modules were compiled as expected
> As for a more general, long-form warning about optimizations, I grabbed
> Miroslav's LPC slides from a few years back and poked around at some
> IPA-optimized disassembly... Here are my notes that attempt to capture some
> common cases:
>
> http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
>
> It's not complete and I lost steam about 80% of the way through today.
> :) But if it looks useful enough to add to Documentation/livepatch, we
> can work on it on-list and try to steer folks into using the automated
> kpatch-build, objtool (eventually) or a source-based safety checklist.
It looks really useful. Could you prepare a patch and submit it, please?
We could discuss it there.
> The
> source-based steps have been posted on-list a few times, but I think it only
> needs to be formalized in a doc.
Yes, I think they were. We discussed it with Nicolai to (better) document
our workflow. It is currently based on klp-ccp
(https://github.com/SUSE/klp-ccp), but we need a proper documentation how
to prepare a live patch starting with an ordinary patch.
Thanks
Miroslav
Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"
On Fri, 17 Jul 2020, Josh Poimboeuf wrote:
> Use of the new -flive-patching flag was introduced with the following
> commit:
>
> 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is
> enabled")
>
> This flag has several drawbacks:
>
> - It disables some optimizations, so it can have a negative effect on
> performance.
>
> - According to the GCC documentation it's not compatible with LTO, which
> will become a compatibility issue as LTO support gets upstreamed in
> the kernel.
>
> - It was intended to be used for source-based patch generation tooling,
> as opposed to binary-based patch generation tooling (e.g.,
> kpatch-build). It probably should have at least been behind a
> separate config option so as not to negatively affect other livepatch
> users.
>
> - Clang doesn't have the flag, so as far as I can tell, this method of
> generating patches is incompatible with Clang, which like LTO is
> becoming more mainstream.
>
> - It breaks GCC's implicit noreturn detection for local functions. This
> is the cause of several "unreachable instruction" objtool warnings.
>
> - The broken noreturn detection is an obvious GCC regression, but we
> haven't yet gotten GCC developers to acknowledge that, which doesn't
> inspire confidence in their willingness to keep the feature working as
> optimizations are added or changed going forward.
>
> - While there *is* a distro which relies on this flag for their distro
> livepatch module builds, there's not a publicly documented way to
> create safe livepatch modules with it. Its use seems to be based on
> tribal knowledge. It serves no benefit to those who don't know how to
> use it.
>
> (In fact, I believe the current livepatch documentation and samples
> are misleading and dangerous, and should be corrected. Or at least
> amended with a disclaimer. But I don't feel qualified to make such
> changes.)
>
> Also, we have an idea for using objtool to detect function changes,
> which could potentially obsolete the need for this flag anyway.
>
> At this point the flag has no benefits for upstream which would
> counteract the above drawbacks. Revert it until it becomes more ready.
>
> This reverts commit 43bd3a95c98e1a86b8b55d97f745c224ecff02b9.
>
> Fixes: 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is
> enabled")
> Reported-by: Randy Dunlap
> Signed-off-by: Josh Poimboeuf
Acked-by: Miroslav Benes
M
Re: [PATCH 3/3] arm64: stacktrace: Convert to ARCH_STACKWALK
On Thu, 16 Jul 2020, Mark Brown wrote:
> On Thu, Jul 16, 2020 at 01:56:13PM +0200, Miroslav Benes wrote:
> > On Wed, 15 Jul 2020, Mark Brown wrote:
>
> > > -void save_stack_trace(struct stack_trace *trace)
> > > -{
> > > - __save_stack_trace(current, trace, 0);
> > > + walk_stackframe(task, , consume_entry, cookie);
> > > }
>
> > just an idea for further improvement (and it might be a matter of taste).
>
> Yeah, there's some more stuff that can be done - the reason I'm looking
> at this code is to do reliable stack trace which is going to require at
> least some changes to the actual unwinder, this just seemed like a
> useful block moving things forwards in itself and I particularly wanted
> feedback on patch 1.
Understood. Reliable stack traces would be an important step for live
patching on arm64, so I am looking forward to seeing that.
Thanks
Miroslav
Re: [PATCH 1/3] stacktrace: Remove reliable argument from arch_stack_walk() callback
On Wed, 15 Jul 2020, Mark Brown wrote: > Currently the callback passed to arch_stack_walk() has an argument called > reliable passed to it to indicate if the stack entry is reliable, a comment > says that this is used by some printk() consumers. However in the current > kernel none of the arch_stack_walk() implementations ever set this flag to > true and the only callback implementation we have is in the generic > stacktrace code which ignores the flag. It therefore appears that this > flag is redundant so we can simplify and clarify things by removing it. Correct. I dug around and it seems it was the case even when it was introduced. So I wonder about the comment. I don't remember the details, maybe Thomas or someone else does. But the patch looks correct. Miroslav
Re: [PATCH 3/3] arm64: stacktrace: Convert to ARCH_STACKWALK
Hi,
On Wed, 15 Jul 2020, Mark Brown wrote:
> Historically architectures have had duplicated code in their stack trace
> implementations for filtering what gets traced. In order to avoid this
> duplication some generic code has been provided using a new interface
> arch_stack_walk(), enabled by selecting ARCH_STACKWALK in Kconfig, which
> factors all this out into the generic stack trace code. Convert arm64
> to use this common infrastructure.
>
> Signed-off-by: Mark Brown
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/stacktrace.c | 79 --
> 2 files changed, 9 insertions(+), 71 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5d4f02b3dfe9..6ed4b6c6df95 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -29,6 +29,7 @@ config ARM64
> select ARCH_HAS_SETUP_DMA_OPS
> select ARCH_HAS_SET_DIRECT_MAP
> select ARCH_HAS_SET_MEMORY
> + select ARCH_STACKWALK
> select ARCH_HAS_STRICT_KERNEL_RWX
> select ARCH_HAS_STRICT_MODULE_RWX
> select ARCH_HAS_SYNC_DMA_FOR_DEVICE
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 743cf11fbfca..a33fba048954 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -133,82 +133,19 @@ void notrace walk_stackframe(struct task_struct *tsk,
> struct stackframe *frame,
> NOKPROBE_SYMBOL(walk_stackframe);
>
> #ifdef CONFIG_STACKTRACE
> -struct stack_trace_data {
> - struct stack_trace *trace;
> - unsigned int no_sched_functions;
> - unsigned int skip;
> -};
>
> -static bool save_trace(void *d, unsigned long addr)
> +void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> + struct task_struct *task, struct pt_regs *regs)
> {
> - struct stack_trace_data *data = d;
> - struct stack_trace *trace = data->trace;
> -
> - if (data->no_sched_functions && in_sched_functions(addr))
> - return false;
> - if (data->skip) {
> - data->skip--;
> - return false;
> - }
> -
> - trace->entries[trace->nr_entries++] = addr;
> -
> - return trace->nr_entries >= trace->max_entries;
> -}
> -
> -void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
> -{
> - struct stack_trace_data data;
> - struct stackframe frame;
> -
> - data.trace = trace;
> - data.skip = trace->skip;
> - data.no_sched_functions = 0;
> -
> - start_backtrace(, regs->regs[29], regs->pc);
> - walk_stackframe(current, , save_trace, );
> -}
> -EXPORT_SYMBOL_GPL(save_stack_trace_regs);
> -
> -static noinline void __save_stack_trace(struct task_struct *tsk,
> - struct stack_trace *trace, unsigned int nosched)
> -{
> - struct stack_trace_data data;
> struct stackframe frame;
>
> - if (!try_get_task_stack(tsk))
> - return;
> + if (regs)
> + start_backtrace(, regs->regs[29], regs->pc);
> + else
> + start_backtrace(, thread_saved_fp(task),
> + thread_saved_pc(task));
>
> - data.trace = trace;
> - data.skip = trace->skip;
> - data.no_sched_functions = nosched;
> -
> - if (tsk != current) {
> - start_backtrace(, thread_saved_fp(tsk),
> - thread_saved_pc(tsk));
> - } else {
> - /* We don't want this function nor the caller */
> - data.skip += 2;
> - start_backtrace(,
> - (unsigned long)__builtin_frame_address(0),
> - (unsigned long)__save_stack_trace);
> - }
> -
> - walk_stackframe(tsk, , save_trace, );
> -
> - put_task_stack(tsk);
> -}
> -
> -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
> -{
> - __save_stack_trace(tsk, trace, 1);
> -}
> -EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
> -
> -void save_stack_trace(struct stack_trace *trace)
> -{
> - __save_stack_trace(current, trace, 0);
> + walk_stackframe(task, , consume_entry, cookie);
> }
just an idea for further improvement (and it might be a matter of taste).
Wouldn't it be slightly better to do one more step and define "struct
unwind_state" instead of "struct stackframe" and also some iterator for
the unwinding and use that right in new arch_stack_walk() instead of
walk_stackframe()? I mean, take the unbounded loop, "inline" it to
arch_stack_walk() and replace the loop with the iterator. The body of the
iterator would call to unwind_frame() and consume_entry() and that's it.
It would make arm64 implementation very similar to x86 and s390 and thus
easier to follow when one switches between architectures all the time.
Tangential to this patch, but another idea for improvement is in
unwind_frame(). If I am not missing something, everything in
CONFIG_FUNCTION_GRAPH_TRACER could be replaced by a simple call to
Re: linux-next: Tree for Jun 23 (objtool (2))
On Wed, 15 Jul 2020, Josh Poimboeuf wrote: > On Wed, Jul 15, 2020 at 03:41:55PM +0200, Petr Mladek wrote: > > On Wed 2020-07-15 14:10:54, Petr Mladek wrote: > > > On Wed 2020-07-15 13:11:14, Miroslav Benes wrote: > > > > Petr, would you agree to revert -flive-patching. > > > > > > Yes, I agree. > > > > Or better to say that I will not block it. > > > > I see that it causes maintenance burden. There are questions about > > reliability and performance impact. I do not have a magic solution > > in the pocket. > > > > Anyway, we need a solution to know what functions need to get livepatched. > > I do not have experience with comparing the assembly, so I do not know > > how it is complicated and reliable. > > Thanks Petr/Miroslav. I can do the revert patch. It doesn't have to be > a permanent revert. I'd certainly be willing to ACK it again in the > future if/when it becomes more ready. Ok. > Yannick has agreed to start looking at the objtool function diff > feature. It's purely theoretical at the moment, we'll see how it works > in practice. We already do something similar in kpatch-build -- it > differs from the objtool model, but it at least shows that something > similar is possible. Great. I'm looking forward to seeing that. Thanks Miroslav
Re: linux-next: Tree for Jun 23 (objtool (2))
On Tue, 14 Jul 2020, Josh Poimboeuf wrote: > On Tue, Jul 14, 2020 at 12:56:21PM +0200, Miroslav Benes wrote: > > On Thu, 2 Jul 2020, Josh Poimboeuf wrote: > > > > > On Tue, Jun 23, 2020 at 08:06:07AM -0700, Randy Dunlap wrote: > > > > On 6/22/20 11:28 PM, Stephen Rothwell wrote: > > > > > Hi all, > > > > > > > > > > Changes since 20200622: > > > > > > > > > > > > > on x86_64: > > > > > > > > arch/x86/kernel/cpu/mce/core.o: warning: objtool: mce_timed_out()+0x24: > > > > unreachable instruction > > > > kernel/exit.o: warning: objtool: __x64_sys_exit_group()+0x14: > > > > unreachable instruction > > > > > > > > Full randconfig file is attached. > > > > > > More livepatch... > > > > Correct. > > > > Both are known and I thought Josh had fixes queued somewhere for both, but > > my memory fails me quite often. See below. > > I did have fixes for some of them in a stash somewhere, but I never > finished them because I decided it's a GCC bug. Same here. > > However, I think it is time to decide how to approach this whole saga. It > > seems that there are not so many places in the kernel in need of > > __noreturn annotation in the end and as jikos argued at least some of > > those should be fixed regardless. > > I would agree that global functions like do_group_exit() deserve a > __noreturn annotation, though it should be in the header file. But > static functions shouldn't need it. Agreed. I'll post the patches for global functions eventually, but see below first. > > Josh, should I prepare proper patches and submit them to relevant > > maintainers to see where this path is going? > > If that's how you want to handle it, ok, but it doesn't seem right to > me, for the static functions at least. > > > It would be much better to fix it in GCC, but it has been like banging > > one's head against a wall so far. Josh, you wanted to create a bug > > for GCC in this respect in the past? Has that happened? > > I didn't open a bug, but I could, if you think that would help. I > haven't had a lot of success with GCC bugs in the past. Understood. > > If I remember correctly, we discussed briefly a possibility to cope with > > that in objtool, but no solution was presented. > > That would also feel like a GCC workaround and might impede objtool's > ability to find bugs like this one, and possibly more serious bugs. > > > Removing -flive-patching is also a possibility. I don't like it much, but > > we discussed it with Petr M. a couple of months ago and it might be a way > > too. > > -flive-patching has many problems which I outlined before. None of them > have been addressed. I still feel the same way, that it should be > reverted until it's ready. Otherwise it's a drain on upstream. > > Also, if the GCC developers won't acknowledge this bug then it doesn't > give me confidence in their ability to keep the feature working as > optimizations are added or changed. I must admit that I've started to share the sentiment recently. And it is probably the main reason for changing my mind about the whole thing. > I still think a potential alternative exists: objtool could be used as a > simple tree-wide object diff tool by generating a checksum for each > function. Then the patch can be applied and built to see exactly which > functions have changed, based on the changed checksums. In which case > this feature would no longer be needed anyway, would you agree? Yes. > I also think that could be a first step for converging our patch > creation processes. Yes again. Petr, would you agree to revert -flive-patching due to reasons above? Is there anything you want to add? Miroslav
Re: [PATCH v2] selftests/livepatch: adopt to newer sysctl error format
On Tue, 14 Jul 2020, Petr Mladek wrote: > With procfs v3.3.16, the sysctl command doesn't print the set key and > value on error. This change breaks livepatch selftest test-ftrace.sh, > that tests the interaction of sysctl ftrace_enabled: > > Make it work with all sysctl versions using '-q' option. > > Explicitly print the final status on success so that it can be verified > in the log. The error message is enough on failure. > > Reported-by: Kamalesh Babulal > Signed-off-by: Petr Mladek Acked-by: Miroslav Benes M
Re: linux-next: Tree for Jun 23 (objtool (2))
On Thu, 2 Jul 2020, Josh Poimboeuf wrote:
> On Tue, Jun 23, 2020 at 08:06:07AM -0700, Randy Dunlap wrote:
> > On 6/22/20 11:28 PM, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > Changes since 20200622:
> > >
> >
> > on x86_64:
> >
> > arch/x86/kernel/cpu/mce/core.o: warning: objtool: mce_timed_out()+0x24:
> > unreachable instruction
> > kernel/exit.o: warning: objtool: __x64_sys_exit_group()+0x14: unreachable
> > instruction
> >
> > Full randconfig file is attached.
>
> More livepatch...
Correct.
Both are known and I thought Josh had fixes queued somewhere for both, but
my memory fails me quite often. See below.
However, I think it is time to decide how to approach this whole saga. It
seems that there are not so many places in the kernel in need of
__noreturn annotation in the end and as jikos argued at least some of
those should be fixed regardless. Josh, should I prepare proper patches
and submit them to relevant maintainers to see where this path is going?
It would be much better to fix it in GCC, but it has been like banging
one's head against a wall so far. Josh, you wanted to create a bug
for GCC in this respect in the past? Has that happened?
If I remember correctly, we discussed briefly a possibility to cope with
that in objtool, but no solution was presented.
Removing -flive-patching is also a possibility. I don't like it much, but
we discussed it with Petr M. a couple of months ago and it might be a way
too.
Thanks
Miroslav
---
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 14e4b4d17ee5..469a71ecea3c 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -279,7 +279,7 @@ static int fake_panic;
static atomic_t mce_fake_panicked;
/* Panic in progress. Enable interrupts and wait for final IPI */
-static void wait_for_panic(void)
+static void __noreturn wait_for_panic(void)
{
long timeout = PANIC_TIMEOUT*USEC_PER_SEC;
diff --git a/kernel/exit.c b/kernel/exit.c
index 727150f28103..570649152e7f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -877,7 +877,7 @@ SYSCALL_DEFINE1(exit, int, error_code)
* as well as by sys_exit_group (below).
*/
void
-do_group_exit(int exit_code)
+__noreturn do_group_exit(int exit_code)
{
struct signal_struct *sig = current->signal;
Re: [RFC][PATCH v5 03/51] objtool: Make recordmcount into mcount subcmd
On Thu, 18 Jun 2020, Matt Helsley wrote:
> Rather than a standalone executable merge recordmcount as a sub command
> of objtool. This is a small step towards cleaning up recordmcount and
> eventually sharing ELF code with objtool.
>
> For the initial step all that's required is a bit of Makefile changes
> and invoking the former main() function from recordmcount.c because the
> subcommand code uses similar function arguments as main when dispatching.
>
> objtool ignores some object files that tracing does not, specifically
> those with OBJECT_FILES_NON_STANDARD Makefile variables. For this reason
> we keep the recordmcount_dep separate from the objtool_dep. When using
> objtool mcount we can also, like the other objtool invocations, just
> depend on the binary rather than the source the binary is built from.
>
> Subsequent patches will gradually convert recordmcount to use
> more and more of libelf/objtool's ELF accessor code. This will both
> clean up recordmcount to be more easily readable and remove
> recordmcount's crude accessor wrapping code.
I'll try to leave only relevant parts for a question below...
> sub_cmd_record_mcount = \
> if [ $(@) != "scripts/mod/empty.o" ]; then \
> - $(objtree)/tools/objtool/recordmcount $(RECORDMCOUNT_FLAGS)
> "$(@)"; \
> + $(objtree)/tools/objtool/objtool mcount record
> $(RECORDMCOUNT_FLAGS) "$(@)";\
> fi;
> +int cmd_mcount(int argc, const char **argv)
> +{
> + argc--; argv++;
> + if (argc <= 0)
> + usage_with_options(mcount_usage, mcount_options);
> +
> + if (!strncmp(argv[0], "record", 6)) {
> + argc = parse_options(argc, argv,
> + mcount_options, mcount_usage, 0);
> + if (argc < 1)
> + usage_with_options(mcount_usage, mcount_options);
> +
> + return record_mcount(argc, argv);
> + }
> +
> + usage_with_options(mcount_usage, mcount_options);
> +
> + return 0;
> +}
> -int main(int argc, char *argv[])
> +int record_mcount(int argc, const char **argv)
> {
> const char ftrace[] = "/ftrace.o";
> int ftrace_size = sizeof(ftrace) - 1;
> int n_error = 0; /* gcc-4.3.0 false positive complaint */
> - int c;
> int i;
>
> - while ((c = getopt(argc, argv, "w")) >= 0) {
> - switch (c) {
> - case 'w':
> - warn_on_notrace_sect = 1;
> - break;
> - default:
> - fprintf(stderr, "usage: recordmcount [-w] file.o...\n");
> - return 0;
> - }
> - }
> -
> - if ((argc - optind) < 1) {
> - fprintf(stderr, "usage: recordmcount [-w] file.o...\n");
> - return 0;
> - }
> -
> /* Process each file in turn, allowing deep failure. */
> - for (i = optind; i < argc; i++) {
> - char *file = argv[i];
> + for (i = 0; i < argc; i++) {
> + const char *file = argv[i];
> int len;
Do you expect that mcount subcmd would be called on more than one object
file at a time? I don't see a reason for that with all the Makefile
changes, but I may be missing something (Kbuild is a maze for me).
Because if not, I think it would be nice to make record_mcount() more
similar to what we have for check(). After Julien's changes
(20200608071203.4055-1-jthie...@redhat.com) at least. So that
record_mcount() could accept struct objtool_file and work directly on
that.
It would also impact several other patches in the series. For example,
is there a need for a private 'struct elf *lf' in mcount.c?
Thanks
Miroslav
Re: [RFC][PATCH v5 01/51] objtool: Factor out reasons to build objtool
Hi, this is a nice improvement. > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile > index 7770edcda3a0..aa0c6d3d2d46 100644 > --- a/tools/objtool/Makefile > +++ b/tools/objtool/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > include ../scripts/Makefile.include > include ../scripts/Makefile.arch > +include $(OUTPUT)/../../include/config/auto.conf > > # always use the host compiler > ifneq ($(LLVM),) > @@ -47,8 +48,8 @@ CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED) > > AWK = awk > > -SUBCMD_CHECK := n > -SUBCMD_ORC := n > +SUBCMD_CHECK := $(CONFIG_OBJTOOL_SUBCMD_CHECK) > +SUBCMD_ORC := $(CONFIG_OBJTOOL_SUBCMD_ORC) > > ifeq ($(SRCARCH),x86) > SUBCMD_CHECK := y I guess you can remove ifeq for x86 in this patch, right? You do it later in 3/51, but it seems to belong here. Miroslav
Re: [PATCH] objtool: Free memory in error case in special_get_alts
Hi,
On Fri, 12 Jun 2020, Tobias Klauser wrote:
> Avoid a memory leak in case get_alt_entry returns an error.
yes, this is not the only one, but I doubt we want to spend time on that.
The process is about to exit anyway.
Miroslav
> Signed-off-by: Tobias Klauser
> ---
> tools/objtool/special.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/objtool/special.c b/tools/objtool/special.c
> index e74e0189de22..f6f7dee1ba77 100644
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -188,8 +188,10 @@ int special_get_alts(struct elf *elf, struct list_head
> *alts)
> memset(alt, 0, sizeof(*alt));
>
> ret = get_alt_entry(elf, entry, sec, idx, alt);
> - if (ret)
> + if (ret) {
> + free(alt);
> return ret;
> + }
>
> list_add_tail(>list, alts);
> }
> --
> 2.27.0
>
Re: [PATCH 0/7] Make check implementation arch agnostic
On Mon, 8 Jun 2020, Julien Thierry wrote: > Hi, > > The current implementation of the check subcommand has various x86 bits > here and there. In order to prepare objtool to provide check for other > architectures, add some abstraction over the x86 specific bits, relying > on objtool arch specific code to provide some necessary operations. > > This is part of the effort to implement check for arm64, initiated [1] > by Raphael. The series is based on top of the separation of check & orc > subcommands series[2]. > > I've push both series base on top of tip/objtool/core at [3]. > > - The first two patches make it simpler for new arches to provide their > list of kernel headers, without worrying about modifications in the x86 > headers. > - Patch 3 Moves arch specific macros to more suitable location > - Patches 4 and 5 add abstraction to handle alternatives > - Patch 6 adds abstraction to handle jump table > - Patch 7 abstracts the use of unwind hints. Adding it as RFC as I'm sure > there's room for improvement. Reviewed-by: Miroslav Benes for patches 1-6. M
Re: [RFC PATCH 7/7] objtool: Make unwind_hints available for all architectures
Hi Julien, On Mon, 8 Jun 2020, Julien Thierry wrote: > Unwind hints are useful to give some information about the call frame > or stack states in non-standard code. > > Despite unwind hints being used in arch-independent code, the > unwind_hint structure type itself is define in x86 kernel headers. > > This is because what an unwind hint will describe is very architecture > specific, both regarding the state and the affected registers. > > To get to share this concept, expose the unwind_hint structure across > architecutres. However, the hint types remain defined by the > architecture code. Objtool then needs it's arch specific code to > "decode" the unwind hint into a cfi_state. I think it would be nice to split the patch. Something like. 1. current include/linux/frame.h mixes assembly and non-assembly definitions, so introduce ASSEMBLY ifdef first seems like a good idea to me. 2. move the relevant definitions to frame.h and add the file to sync-check 3. the rest of the patch Would it make sense? Otherwise, it looks good to me. Miroslav
Re: [PATCH v2 0/4] Remove dependency of check subcmd upon orc
On Mon, 8 Jun 2020, Julien Thierry wrote: > Hi, > > Matt Helsley's change[1] provided a base framework to opt-in/out > objtool subcommands at compile time. This makes it easier for > architectures to port objtool, one subcommand at a time. > > Orc generation relies on the check operation implementation. However, > the way this is done causes the check implementation to depend on the > implementation of orc generation functions to call if orc generation is > requested. This means that in order to implement check subcmd, orc > subcmd also need to be implemented. > > These patches aim at removing that dependency, having orc subcmd > being built on top of the check subcmd. > > Changes since v1 [2]: > - Remove redundant check in create_orc pointed out by Miroslav > > [1] https://www.spinics.net/lists/kernel/msg3510844.html > [2] https://lkml.org/lkml/2020/6/4/675 Reviewed-by: Miroslav Benes M
Re: [PATCH v2 1/4] selftests/livepatch: simplify test-klp-callbacks busy target tests
On Wed, 3 Jun 2020, Yannick Cote wrote: > From: Joe Lawrence > > The test-klp-callbacks script includes a few tests which rely on kernel > task timings that may not always execute as expected under system load. > These may generate out of sequence kernel log messages that result in > test failure. > > Instead of using sleep timing windows to orchestrate these tests, add a > block_transition module parameter to communicate the test purpose and > utilize flush_queue() to serialize the test module's task output. > > Suggested-by: Petr Mladek > Signed-off-by: Joe Lawrence Acked-by: Miroslav Benes M
