| Message ID | 1457023921-2051-1-git-send-email-pmladek@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 4 Mar 2016 03:52, "Petr Mladek" <pmladek@suse.com> wrote: > > From: Balbir Singh <bsingharora@gmail.com> > > Changelog v4: > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > and used it just to convert the function address. > 2. Synced klp_write_module_reloc() with s390(); made it > inline, no error message, return -ENOSYS > 3. Added an error message when including > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > 4. Update some comments. > Changelog v3: > 1. Moved -ENOSYS to -EINVAL in klp_write_module_reloc > 2. Moved klp_matchaddr to use ftrace_location_range > Changelog v2: > 1. Implement review comments by Michael > 2. The previous version compared _NIP from the > wrong location to check for whether we > are going to a patched location > > This applies on top of the patches posted by Michael > https://patchwork.ozlabs.org/patch/589791/ > > It enables livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > (See the reference [1] below) and adds logic for checking offset ranges > in livepatch with ftrace_location_range. > > I tested the sample in the livepatch > > Signed-off-by: Torsten Duwe <duwe@suse.de> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Petr Mladek <pmladek@suse.com> > Like the cleanup and the patch Balbir
* Petr Mladek <pmladek@suse.com> [2016-03-03 17:52:01]: > From: Balbir Singh <bsingharora@gmail.com> > > Changelog v4: > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > and used it just to convert the function address. > 2. Synced klp_write_module_reloc() with s390(); made it > inline, no error message, return -ENOSYS > 3. Added an error message when including > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > 4. Update some comments. > Changelog v3: > 1. Moved -ENOSYS to -EINVAL in klp_write_module_reloc > 2. Moved klp_matchaddr to use ftrace_location_range > Changelog v2: > 1. Implement review comments by Michael > 2. The previous version compared _NIP from the > wrong location to check for whether we > are going to a patched location > > This applies on top of the patches posted by Michael > https://patchwork.ozlabs.org/patch/589791/ > > It enables livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > (See the reference [1] below) and adds logic for checking offset ranges > in livepatch with ftrace_location_range. > > I tested the sample in the livepatch > > Signed-off-by: Torsten Duwe <duwe@suse.de> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Petr Mladek <pmladek@suse.com> Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> I tried it, with the sample livepatch module. Tested-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> Thanks, Kamalesh.
Hi Petr, On Thu, 2016-03-03 at 17:52 +0100, Petr Mladek wrote: > From: Balbir Singh <bsingharora@gmail.com> > > Changelog v4: > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > and used it just to convert the function address. > 2. Synced klp_write_module_reloc() with s390(); made it > inline, no error message, return -ENOSYS OK. > 3. Added an error message when including > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH I don't know why we want to do that, I don't see how it is helpful. It doesn't even do what it says: > +#ifdef CONFIG_LIVEPATCH ... > +#else /* CONFIG_LIVEPATCH */ > +#error Include linux/livepatch.h, not asm/livepatch.h > +#endif /* CONFIG_LIVEPATCH */ If I turn on CONFIG_LIVEPATCH then I can quite happily include asm/livepatch.h and not get an error. So the check doesn't do what the message suggests. If we *really* want to prevent people from including asm/livepatch.h then it needs to check for _LINUX_LIVEPATCH_H_. But there's no reason I can see why we *must* prevent people from including asm/livepatch.h. And on x86 & s390 it does: #else #error Live patching support is disabled; check CONFIG_LIVEPATCH #endif Which is also not helpful AFAICS, and just likely to break the build for no good reason. > This applies on top of the patches posted by Michael > https://patchwork.ozlabs.org/patch/589791/ > > It enables livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > (See the reference [1] below) and adds logic for checking offset ranges > in livepatch with ftrace_location_range. > > I tested the sample in the livepatch Thanks. cheers
Hi livepatch maintainers, On Thu, 2016-03-03 at 17:52 +0100, Petr Mladek wrote: > From: Balbir Singh <bsingharora@gmail.com> > > Changelog v4: > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > and used it just to convert the function address. > 2. Synced klp_write_module_reloc() with s390(); made it > inline, no error message, return -ENOSYS > 3. Added an error message when including > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > 4. Update some comments. > Changelog v3: > 1. Moved -ENOSYS to -EINVAL in klp_write_module_reloc > 2. Moved klp_matchaddr to use ftrace_location_range > Changelog v2: > 1. Implement review comments by Michael > 2. The previous version compared _NIP from the > wrong location to check for whether we > are going to a patched location > > This applies on top of the patches posted by Michael > https://patchwork.ozlabs.org/patch/589791/ > > It enables livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > (See the reference [1] below) and adds logic for checking offset ranges > in livepatch with ftrace_location_range. > > I tested the sample in the livepatch > > Signed-off-by: Torsten Duwe <duwe@suse.de> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > arch/powerpc/Kconfig | 3 +++ > arch/powerpc/include/asm/livepatch.h | 47 +++++++++++++++++++++++++++++++++ > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/entry_64.S | 50 ++++++++++++++++++++++++++++++++++++ > arch/powerpc/kernel/livepatch.c | 29 +++++++++++++++++++++ > include/linux/ftrace.h | 1 + > include/linux/livepatch.h | 2 ++ > kernel/livepatch/core.c | 28 +++++++++++++++++--- > kernel/trace/ftrace.c | 14 +++++++++- > 9 files changed, 171 insertions(+), 4 deletions(-) > create mode 100644 arch/powerpc/include/asm/livepatch.h > create mode 100644 arch/powerpc/kernel/livepatch.c How should we go about merging this? Obviously it depends heavily on the content of my series, which will go into powerpc#next, so it would make sense if this went there too. I don't see any changes in linux-next for livepatch, so merging it via powerpc would probably work fine and not cause any conflicts, unless there's some livepatch changes pending for 4.6 that aren't in linux-next yet? The other option is that I put my ftrace changes and this in a topic branch (based on v4.5-rc3), and then that can be merged into both powerpc#next and the livepatch tree. Also regardless of who takes it an Ack from Steve for the ftrace changes would be good. cheers
On Fri, 4 Mar 2016, Michael Ellerman wrote: > Obviously it depends heavily on the content of my series, which will go into > powerpc#next, so it would make sense if this went there too. > > I don't see any changes in linux-next for livepatch, so merging it via powerpc > would probably work fine and not cause any conflicts, unless there's some > livepatch changes pending for 4.6 that aren't in linux-next yet? > > The other option is that I put my ftrace changes and this in a topic branch > (based on v4.5-rc3), and then that can be merged into both powerpc#next and the > livepatch tree. This aligns with my usual workflow, so that'd be my preferred way of doing things; i.e. you put all the ftrace changes into a separate topic branch, and then - you pull that branch into powerpc#next - I pull that branch into livepatching tree - I apply the ppc livepatching support on top of that - I send a pull request to Linus only after powerpc#next gets merged to Linus' tree Sounds good? > Also regardless of who takes it an Ack from Steve for the ftrace changes > would be good. Absolutely. Thanks!
Hi, On Fri, 4 Mar 2016, Michael Ellerman wrote: > Hi Petr, > > On Thu, 2016-03-03 at 17:52 +0100, Petr Mladek wrote: > > > From: Balbir Singh <bsingharora@gmail.com> > > > > Changelog v4: > > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > > and used it just to convert the function address. > > 2. Synced klp_write_module_reloc() with s390(); made it > > inline, no error message, return -ENOSYS > > OK. > > > 3. Added an error message when including > > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > > I don't know why we want to do that, I don't see how it is helpful. It doesn't > even do what it says: > > > +#ifdef CONFIG_LIVEPATCH > ... > > +#else /* CONFIG_LIVEPATCH */ > > +#error Include linux/livepatch.h, not asm/livepatch.h > > +#endif /* CONFIG_LIVEPATCH */ > > If I turn on CONFIG_LIVEPATCH then I can quite happily include asm/livepatch.h > and not get an error. So the check doesn't do what the message suggests. Well, yes. I looked into the archives to find if there was a reason to even introduce it. It was not. It came up during a review process of the livepatching patch set somehow and we left it there. I only changed the error message to the mentioned one because we deemed it was better. > If we *really* want to prevent people from including asm/livepatch.h then it > needs to check for _LINUX_LIVEPATCH_H_. But there's no reason I can see why we > *must* prevent people from including asm/livepatch.h. > > And on x86 & s390 it does: > > #else > #error Live patching support is disabled; check CONFIG_LIVEPATCH > #endif > > Which is also not helpful AFAICS, and just likely to break the build for no > good reason. This is the old message. See 383bf44d1a8b ("livepatch: change the error message in asm/livepatch.h header files"). Anyway, it really does not mean much. I'll send a patch for s390 and x86 to remove it completely in a minute. Thanks, Miroslav
On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote: [...] > index ec7f8aada697..2d5333c228f1 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -1265,6 +1271,31 @@ ftrace_call: > ld r0, LRSAVE(r1) > mtlr r0 > > +#ifdef CONFIG_LIVEPATCH > + beq+ 4f /* likely(old_NIP == new_NIP) */ > + /* > + * For a local call, restore this TOC after calling the patch function. > + * For a global call, it does not matter what we restore here, > + * since the global caller does its own restore right afterwards, > + * anyway. Just insert a klp_return_helper frame in any case, > + * so a patch function can always count on the changed stack offsets. > + * The patch introduces a frame such that from the patched function > + * we return back to klp_return helper. For ABI compliance r12, > + * lr and LRSAVE(r1) contain the address of klp_return_helper. > + * We loaded ctr with the address of the patched function earlier > + */ > + stdu r1, -32(r1) /* open new mini stack frame */ > + std r2, 24(r1) /* save TOC now, unconditionally. */ > + bl 5f > +5: mflr r12 > + addi r12, r12, (klp_return_helper + 4 - .)@l > + std r12, LRSAVE(r1) > + mtlr r12 > + mfctr r12 /* allow for TOC calculation in newfunc */ > + bctr > +4: > +#endif > + > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > stdu r1, -112(r1) > .globl ftrace_graph_call > @@ -1281,6 +1312,25 @@ _GLOBAL(ftrace_graph_stub) > > _GLOBAL(ftrace_stub) > blr > +#ifdef CONFIG_LIVEPATCH > +/* Helper function for local calls that are becoming global > + * due to live patching. > + * We can't simply patch the NOP after the original call, > + * because, depending on the consistency model, some kernel > + * threads may still have called the original, local function > + * *without* saving their TOC in the respective stack frame slot, > + * so the decision is made per-thread during function return by > + * maybe inserting a klp_return_helper frame or not. > +*/ > +klp_return_helper: > + ld r2, 24(r1) /* restore TOC (saved by ftrace_caller) */ > + addi r1, r1, 32 /* destroy mini stack frame */ > + ld r0, LRSAVE(r1) /* get the real return address */ > + mtlr r0 > + blr > +#endif > + > + > #else > _GLOBAL_TOC(_mcount) > /* Taken from output of objdump from lib64/glibc */ We need a caveat here, at least in the comments, even better in some documentation, that the klp_return_helper shifts the stack layout. This is relevant for functions with more than 8 fixed integer arguments or for any varargs creator. As soon as the patch function is to replace an original with arguments on the stack, the extra stack frame needs to be accounted for. Where shall we put this warning? Torsten
On Fri 2016-03-04 13:42:47, Torsten Duwe wrote: > On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote: > [...] > > index ec7f8aada697..2d5333c228f1 100644 > > --- a/arch/powerpc/kernel/entry_64.S > > +++ b/arch/powerpc/kernel/entry_64.S > > @@ -1265,6 +1271,31 @@ ftrace_call: > > ld r0, LRSAVE(r1) > > mtlr r0 > > > > +#ifdef CONFIG_LIVEPATCH > > + beq+ 4f /* likely(old_NIP == new_NIP) */ > > + /* > > + * For a local call, restore this TOC after calling the patch function. > > + * For a global call, it does not matter what we restore here, > > + * since the global caller does its own restore right afterwards, > > + * anyway. Just insert a klp_return_helper frame in any case, > > + * so a patch function can always count on the changed stack offsets. > > + * The patch introduces a frame such that from the patched function > > + * we return back to klp_return helper. For ABI compliance r12, > > + * lr and LRSAVE(r1) contain the address of klp_return_helper. > > + * We loaded ctr with the address of the patched function earlier > > + */ > > + stdu r1, -32(r1) /* open new mini stack frame */ > > + std r2, 24(r1) /* save TOC now, unconditionally. */ > > + bl 5f > > +5: mflr r12 > > + addi r12, r12, (klp_return_helper + 4 - .)@l > > + std r12, LRSAVE(r1) > > + mtlr r12 > > + mfctr r12 /* allow for TOC calculation in newfunc */ > > + bctr > > +4: > > +#endif > > + > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > > stdu r1, -112(r1) > > .globl ftrace_graph_call > > @@ -1281,6 +1312,25 @@ _GLOBAL(ftrace_graph_stub) > > > > _GLOBAL(ftrace_stub) > > blr > > +#ifdef CONFIG_LIVEPATCH > > +/* Helper function for local calls that are becoming global > > + * due to live patching. > > + * We can't simply patch the NOP after the original call, > > + * because, depending on the consistency model, some kernel > > + * threads may still have called the original, local function > > + * *without* saving their TOC in the respective stack frame slot, > > + * so the decision is made per-thread during function return by > > + * maybe inserting a klp_return_helper frame or not. > > +*/ > > +klp_return_helper: > > + ld r2, 24(r1) /* restore TOC (saved by ftrace_caller) */ > > + addi r1, r1, 32 /* destroy mini stack frame */ > > + ld r0, LRSAVE(r1) /* get the real return address */ > > + mtlr r0 > > + blr > > +#endif > > + > > + > > #else > > _GLOBAL_TOC(_mcount) > > /* Taken from output of objdump from lib64/glibc */ > > We need a caveat here, at least in the comments, even better > in some documentation, that the klp_return_helper shifts the stack layout. > > This is relevant for functions with more than 8 fixed integer arguments > or for any varargs creator. As soon as the patch function is to replace > an original with arguments on the stack, the extra stack frame needs to > be accounted for. Do I understand it correctly that we could not patch functions that pass arguments on the stack with this implementation? If yes, how hard would be to get it working, please? At least, it would be great to catch this problem and handle it with grace. Otherwise, it might be hard to debug. > Where shall we put this warning? Sadly, we do not have any Documentation/livepatch/ yet/. I still hope that we could handle it somehow in the code. Best Regards, Petr
On Fri, Mar 04, 2016 at 02:01:37PM +0100, Petr Mladek wrote: > > Do I understand it correctly that we could not patch functions that > pass arguments on the stack with this implementation? If yes, how hard > would be to get it working, please? At least, it would be great to > catch this problem and handle it with grace. Otherwise, it might > be hard to debug. No, those functions only require special attention. I needed _any_ location to store the caller's TOC; and the stack is thread-safe and recursion-safe. The current caller's frame is already full so I had to create a new one. A patch function could e.g. grab that TOC value in a prologue and then pop that stack frame. Or it could add those 32 bytes to the assumed arguments' stack offsets. > > > Where shall we put this warning? > > Sadly, we do not have any Documentation/livepatch/ yet/. > I still hope that we could handle it somehow in the code. I really think some documentation would be good, a live patch howto for a start... Torsten
On Fri, Mar 04, 2016 at 07:16:57PM +0100, Torsten Duwe wrote: > On Fri, Mar 04, 2016 at 02:01:37PM +0100, Petr Mladek wrote: > > > > Do I understand it correctly that we could not patch functions that > > pass arguments on the stack with this implementation? If yes, how hard > > would be to get it working, please? At least, it would be great to > > catch this problem and handle it with grace. Otherwise, it might > > be hard to debug. > > No, those functions only require special attention. So far it's correct. It's been a while since I wrote that code. > I needed _any_ location to store the caller's TOC; > and the stack is thread-safe and recursion-safe. > The current caller's frame is already full so I had > to create a new one. Correction: the TOC can be stored in the caller's stack frame at the usual location. Only the restore instruction is a problem. > A patch function could e.g. grab that TOC value in a > prologue and then pop that stack frame. Or it could > add those 32 bytes to the assumed arguments' stack offsets. So one solution could be to call the patch function via a small trampoline or pre-prologue that just pops that frame, and have the patch function restore R2 manually at the end. Sorry for the confusion, Torsten
Hi Petr, On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote: > From: Balbir Singh <bsingharora@gmail.com> > > Changelog v4: > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > and used it just to convert the function address. > 2. Synced klp_write_module_reloc() with s390(); made it > inline, no error message, return -ENOSYS > 3. Added an error message when including > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > 4. Update some comments. > Changelog v3: > 1. Moved -ENOSYS to -EINVAL in klp_write_module_reloc > 2. Moved klp_matchaddr to use ftrace_location_range > Changelog v2: > 1. Implement review comments by Michael > 2. The previous version compared _NIP from the > wrong location to check for whether we > are going to a patched location > > This applies on top of the patches posted by Michael > https://patchwork.ozlabs.org/patch/589791/ > > It enables livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > (See the reference [1] below) and adds logic for checking offset ranges > in livepatch with ftrace_location_range. > > I tested the sample in the livepatch This commit message needs a rewrite. Most of the above information can be moved to the diffstat section so it doesn't end up in the git log. The log itself should be a straightforward description of the changes in the patch itself. > Signed-off-by: Torsten Duwe <duwe@suse.de> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > arch/powerpc/Kconfig | 3 +++ > arch/powerpc/include/asm/livepatch.h | 47 +++++++++++++++++++++++++++++++++ > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/entry_64.S | 50 ++++++++++++++++++++++++++++++++++++ > arch/powerpc/kernel/livepatch.c | 29 +++++++++++++++++++++ > include/linux/ftrace.h | 1 + > include/linux/livepatch.h | 2 ++ > kernel/livepatch/core.c | 28 +++++++++++++++++--- > kernel/trace/ftrace.c | 14 +++++++++- > 9 files changed, 171 insertions(+), 4 deletions(-) > create mode 100644 arch/powerpc/include/asm/livepatch.h > create mode 100644 arch/powerpc/kernel/livepatch.c [...] > diff --git a/arch/powerpc/kernel/livepatch.c b/arch/powerpc/kernel/livepatch.c > new file mode 100644 > index 000000000000..267ce0fdd89b > --- /dev/null > +++ b/arch/powerpc/kernel/livepatch.c > @@ -0,0 +1,29 @@ > +/* > + * livepatch.c - powerpc-specific Kernel Live Patching Core > + * > + * Copyright (C) 2015 SUSE > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > +#include <linux/ftrace.h> > +#include <asm/livepatch.h> > + > +/* > + * LivePatch works only with -mprofile-kernel on PPC. In this case, > + * the ftrace location is always within the first 16 bytes. > + */ No need for use CamelCase for LivePatch :-) > +unsigned long klp_get_ftrace_location(unsigned long faddr) > +{ > + return ftrace_location_range(faddr, faddr + 16); > +} > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index c2b340e23f62..fb13cd3e68f9 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -455,6 +455,7 @@ int ftrace_update_record(struct dyn_ftrace *rec, int enable); > int ftrace_test_record(struct dyn_ftrace *rec, int enable); > void ftrace_run_stop_machine(int command); > unsigned long ftrace_location(unsigned long ip); > +unsigned long ftrace_location_range(unsigned long start, unsigned long end); > unsigned long ftrace_get_addr_new(struct dyn_ftrace *rec); > unsigned long ftrace_get_addr_curr(struct dyn_ftrace *rec); > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index a8828652f794..25a267bcb01f 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -134,6 +134,8 @@ int klp_unregister_patch(struct klp_patch *); > int klp_enable_patch(struct klp_patch *); > int klp_disable_patch(struct klp_patch *); > > +unsigned long klp_get_ftrace_location(unsigned long faddr); > + > #endif /* CONFIG_LIVEPATCH */ > > #endif /* _LINUX_LIVEPATCH_H_ */ > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index bc2c85c064c1..9ad597faa57f 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -298,22 +298,39 @@ unlock: > rcu_read_unlock(); > } > > +/* > + * Convert a function address into the appropriate ftrace location. > + * > + * The given address is returned on most architectures. LivePatching > + * usually works only when the ftrace location is the first instruction > + * in the function. > + */ s/LivePatching/Live patching/ Otherwise the livepatch changes look good to me.
On 04/03/16 23:42, Torsten Duwe wrote: > On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote: > [...] >> index ec7f8aada697..2d5333c228f1 100644 >> --- a/arch/powerpc/kernel/entry_64.S >> +++ b/arch/powerpc/kernel/entry_64.S >> @@ -1265,6 +1271,31 @@ ftrace_call: >> ld r0, LRSAVE(r1) >> mtlr r0 >> >> +#ifdef CONFIG_LIVEPATCH >> + beq+ 4f /* likely(old_NIP == new_NIP) */ >> + /* >> + * For a local call, restore this TOC after calling the patch function. >> + * For a global call, it does not matter what we restore here, >> + * since the global caller does its own restore right afterwards, >> + * anyway. Just insert a klp_return_helper frame in any case, >> + * so a patch function can always count on the changed stack offsets. >> + * The patch introduces a frame such that from the patched function >> + * we return back to klp_return helper. For ABI compliance r12, >> + * lr and LRSAVE(r1) contain the address of klp_return_helper. >> + * We loaded ctr with the address of the patched function earlier >> + */ >> + stdu r1, -32(r1) /* open new mini stack frame */ >> + std r2, 24(r1) /* save TOC now, unconditionally. */ >> + bl 5f >> +5: mflr r12 >> + addi r12, r12, (klp_return_helper + 4 - .)@l >> + std r12, LRSAVE(r1) >> + mtlr r12 >> + mfctr r12 /* allow for TOC calculation in newfunc */ >> + bctr >> +4: >> +#endif >> + >> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >> stdu r1, -112(r1) >> .globl ftrace_graph_call >> @@ -1281,6 +1312,25 @@ _GLOBAL(ftrace_graph_stub) >> >> _GLOBAL(ftrace_stub) >> blr >> +#ifdef CONFIG_LIVEPATCH >> +/* Helper function for local calls that are becoming global >> + * due to live patching. >> + * We can't simply patch the NOP after the original call, >> + * because, depending on the consistency model, some kernel >> + * threads may still have called the original, local function >> + * *without* saving their TOC in the respective stack frame slot, >> + * so the decision is made per-thread during function return by >> + * maybe inserting a klp_return_helper frame or not. >> +*/ >> +klp_return_helper: >> + ld r2, 24(r1) /* restore TOC (saved by ftrace_caller) */ >> + addi r1, r1, 32 /* destroy mini stack frame */ >> + ld r0, LRSAVE(r1) /* get the real return address */ >> + mtlr r0 >> + blr >> +#endif >> + >> + >> #else >> _GLOBAL_TOC(_mcount) >> /* Taken from output of objdump from lib64/glibc */ > We need a caveat here, at least in the comments, even better > in some documentation, that the klp_return_helper shifts the stack layout. > > This is relevant for functions with more than 8 fixed integer arguments > or for any varargs creator. As soon as the patch function is to replace > an original with arguments on the stack, the extra stack frame needs to > be accounted for. > > Where shall we put this warning? Good catch! We should just document it in livepatch.c (I suppose). I wonder if we can reuse the previous stack frame -- the caller into ftrace_caller. I think our arch.trampoline does bunch of the work anyway, klp_return_helper would just need to restore the right set of values I hope I am thinking clearly on a Monday morning Balbir Singh
On Fri, 2016-03-04 at 10:31 +0100, Miroslav Benes wrote: > On Fri, 4 Mar 2016, Michael Ellerman wrote: > > On Thu, 2016-03-03 at 17:52 +0100, Petr Mladek wrote: > > > > > 3. Added an error message when including > > > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > > > > I don't know why we want to do that, I don't see how it is helpful. It doesn't > > even do what it says: > > > > > +#ifdef CONFIG_LIVEPATCH > > ... > > > +#else /* CONFIG_LIVEPATCH */ > > > +#error Include linux/livepatch.h, not asm/livepatch.h > > > +#endif /* CONFIG_LIVEPATCH */ > > > > If I turn on CONFIG_LIVEPATCH then I can quite happily include asm/livepatch.h > > and not get an error. So the check doesn't do what the message suggests. > > Well, yes. I looked into the archives to find if there was a reason to > even introduce it. It was not. It came up during a review process of the > livepatching patch set somehow and we left it there. I only changed the > error message to the mentioned one because we deemed it was better. Thanks for looking into it. > > And on x86 & s390 it does: > > > > #else > > #error Live patching support is disabled; check CONFIG_LIVEPATCH > > #endif > > This is the old message. See 383bf44d1a8b ("livepatch: change the error > message in asm/livepatch.h header files"). > > Anyway, it really does not mean much. I'll send a patch for s390 and x86 > to remove it completely in a minute. Thanks. I know it's not a big deal, but the kernel is complicated enough without extra code we don't really need :) cheers
On Fri, 2016-03-04 at 09:56 +0100, Jiri Kosina wrote: > On Fri, 4 Mar 2016, Michael Ellerman wrote: > > > Obviously it depends heavily on the content of my series, which will go into > > powerpc#next, so it would make sense if this went there too. > > > > I don't see any changes in linux-next for livepatch, so merging it via powerpc > > would probably work fine and not cause any conflicts, unless there's some > > livepatch changes pending for 4.6 that aren't in linux-next yet? > > > > The other option is that I put my ftrace changes and this in a topic branch > > (based on v4.5-rc3), and then that can be merged into both powerpc#next and the > > livepatch tree. > > This aligns with my usual workflow, so that'd be my preferred way of doing > things; i.e. you put all the ftrace changes into a separate topic branch, > and then > > - you pull that branch into powerpc#next > - I pull that branch into livepatching tree > - I apply the ppc livepatching support on top of that > - I send a pull request to Linus only after powerpc#next gets merged to > Linus' tree > > Sounds good? Yep, here it is: https://git.kernel.org/cgit/linux/kernel/git/powerpc/linux.git/log/?h=topic/mprofile-kernel aka: git fetch git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git topic/mprofile-kernel I haven't merged it into my next yet, but I will tomorrow unless you tell me there's something wrong with it. cheers
On Mon, 7 Mar 2016, Michael Ellerman wrote: > > This aligns with my usual workflow, so that'd be my preferred way of doing > > things; i.e. you put all the ftrace changes into a separate topic branch, > > and then > > > > - you pull that branch into powerpc#next > > - I pull that branch into livepatching tree > > - I apply the ppc livepatching support on top of that > > - I send a pull request to Linus only after powerpc#next gets merged to > > Linus' tree > > > > Sounds good? > > Yep, here it is: > > https://git.kernel.org/cgit/linux/kernel/git/powerpc/linux.git/log/?h=topic/mprofile-kernel > > aka: > > git fetch git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git topic/mprofile-kernel Excellent, thanks. > I haven't merged it into my next yet, but I will tomorrow unless you tell me > there's something wrong with it. There is one remaining issue which I think would be really nice to have(TM), and that's Steven's Ack for the whole thing :) For the livepatching part, I don't think we are quite there yet (so maybe it'll miss the upcoming merge window anyway). My primary worry there is what Torsten pointed out, i.e. functions with either varargs or more than 8 args needing special care. Also, I'd like to have this positively reviewed by at least one more livepatching maintainer (I am currently looking into it myself, but my understanding of powerpc arch is rather low, so the more eyes, the better). Thanks!
On Mon, 2016-03-07 at 23:52 +0100, Jiri Kosina wrote: > On Mon, 7 Mar 2016, Michael Ellerman wrote: > > > > This aligns with my usual workflow, so that'd be my preferred way of doing > > > things; i.e. you put all the ftrace changes into a separate topic branch, > > > and then > > > > > > - you pull that branch into powerpc#next > > > - I pull that branch into livepatching tree > > > - I apply the ppc livepatching support on top of that > > > - I send a pull request to Linus only after powerpc#next gets merged to > > > Linus' tree > > > > > > Sounds good? > > > > Yep, here it is: > > > > https://git.kernel.org/cgit/linux/kernel/git/powerpc/linux.git/log/?h=topic/mprofile-kernel > > > > aka: > > > > git fetch git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git topic/mprofile-kernel > > Excellent, thanks. > > > I haven't merged it into my next yet, but I will tomorrow unless you tell me > > there's something wrong with it. > > There is one remaining issue which I think would be really nice to > have(TM), and that's Steven's Ack for the whole thing :) Yeah. He's been on CC the whole time, but he's probably getting a bit sick of it all, as we're up to about version 15. So I figure if he really hated it he'd have said so by now :) - but an Ack would still be good. > For the livepatching part, I don't think we are quite there yet (so maybe > it'll miss the upcoming merge window anyway). > > My primary worry there is what Torsten pointed out, i.e. functions with > either varargs or more than 8 args needing special care. Yeah true. My preference would be to merge it, but mark LIVEPATCH as experimental on powerpc. I think having it in the tree would help it get more testing, and probably find other bugs too. But it's up to you guys. > Also, I'd like to have this positively reviewed by at least one more > livepatching maintainer (I am currently looking into it myself, but my > understanding of powerpc arch is rather low, so the more eyes, the > better). Sure. I can answer powerpc questions, though Torsten is probably the person who has the best understanding of (livepatching && powerpc). cheers
On Mon, Mar 07, 2016 at 11:52:31PM +0100, Jiri Kosina wrote: > On Mon, 7 Mar 2016, Michael Ellerman wrote: > > > > This aligns with my usual workflow, so that'd be my preferred way of doing > > > things; i.e. you put all the ftrace changes into a separate topic branch, > > > and then > > > > > > - you pull that branch into powerpc#next > > > - I pull that branch into livepatching tree > > > - I apply the ppc livepatching support on top of that > > > - I send a pull request to Linus only after powerpc#next gets merged to > > > Linus' tree > > > > > > Sounds good? > > > > Yep, here it is: > > > > https://git.kernel.org/cgit/linux/kernel/git/powerpc/linux.git/log/?h=topic/mprofile-kernel > > > > aka: > > > > git fetch git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git topic/mprofile-kernel > > Excellent, thanks. > > > I haven't merged it into my next yet, but I will tomorrow unless you tell me > > there's something wrong with it. > > There is one remaining issue which I think would be really nice to > have(TM), and that's Steven's Ack for the whole thing :) > > For the livepatching part, I don't think we are quite there yet (so maybe > it'll miss the upcoming merge window anyway). > > My primary worry there is what Torsten pointed out, i.e. functions with > either varargs or more than 8 args needing special care. > > Also, I'd like to have this positively reviewed by at least one more > livepatching maintainer (I am currently looking into it myself, but my > understanding of powerpc arch is rather low, so the more eyes, the > better). It's been a few years but I'll try to dust off my powerpc chops and give it a proper review.
On Tue, 08 Mar 2016 10:20:22 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote: > > > > There is one remaining issue which I think would be really nice to > > have(TM), and that's Steven's Ack for the whole thing :) > > Yeah. He's been on CC the whole time, but he's probably getting a bit sick of > it all, as we're up to about version 15. So I figure if he really hated it he'd > have said so by now :) - but an Ack would still be good. > I figured this is all powerpc work, and you can crash what you like :-) If you want, I can try to get some time tomorrow and take a quick look at the patches. But yeah, I haven't been paying too much attention to this. -- Steve
On Fri, Mar 04, 2016 at 08:22:22PM +0100, Torsten Duwe wrote: > On Fri, Mar 04, 2016 at 07:16:57PM +0100, Torsten Duwe wrote: > > On Fri, Mar 04, 2016 at 02:01:37PM +0100, Petr Mladek wrote: > > > > > > Do I understand it correctly that we could not patch functions that > > > pass arguments on the stack with this implementation? If yes, how hard > > > would be to get it working, please? At least, it would be great to > > > catch this problem and handle it with grace. Otherwise, it might > > > be hard to debug. > > > > No, those functions only require special attention. > > So far it's correct. It's been a while since I wrote that code. > > > I needed _any_ location to store the caller's TOC; > > and the stack is thread-safe and recursion-safe. > > The current caller's frame is already full so I had > > to create a new one. > > Correction: the TOC can be stored in the caller's stack frame at > the usual location. Only the restore instruction is a problem. Another correction :-( This is true only for local calls -*> Which become *global* calls due to live patching <*- For callers that made a global call to the patched function originally, they already _have_ stored their TOC value there, and the r2 they enter ftrace caller with is bogus. I see no way to determine which is the case, so my code preserves both: 24(r1) in the caller's frame is left untouched. R2, as it came, is saved in the mini stack frame, as well as the caller's return address (LR, shifted 1 frame). Remember, LR got modified to point to klp_return_helper. Removing this auxiliary stack frame causes even more problems than it solves. > So one solution could be to call the patch function via a small > trampoline or pre-prologue that just pops that frame, and have > the patch function restore R2 manually at the end. I'll try to demonstrate that. It's not so hard. And klp_return_helper will do the right thing for >90% of all function replacements automatically. > Sorry for the confusion, Once more. Torsten
On Thu, 3 Mar 2016 17:52:01 +0100 Petr Mladek <pmladek@suse.com> wrote: > From: Balbir Singh <bsingharora@gmail.com> > > Changelog v4: > 1. Renamed klp_matchaddr() to klp_get_ftrace_location() > and used it just to convert the function address. > 2. Synced klp_write_module_reloc() with s390(); made it > inline, no error message, return -ENOSYS > 3. Added an error message when including > powerpc/include/asm/livepatch.h without HAVE_LIVEPATCH > 4. Update some comments. > Changelog v3: > 1. Moved -ENOSYS to -EINVAL in klp_write_module_reloc > 2. Moved klp_matchaddr to use ftrace_location_range > Changelog v2: > 1. Implement review comments by Michael > 2. The previous version compared _NIP from the > wrong location to check for whether we > are going to a patched location > > This applies on top of the patches posted by Michael > https://patchwork.ozlabs.org/patch/589791/ > > It enables livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > (See the reference [1] below) and adds logic for checking offset ranges > in livepatch with ftrace_location_range. > > I tested the sample in the livepatch > > Signed-off-by: Torsten Duwe <duwe@suse.de> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > arch/powerpc/Kconfig | 3 +++ > arch/powerpc/include/asm/livepatch.h | 47 +++++++++++++++++++++++++++++++++ > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/entry_64.S | 50 ++++++++++++++++++++++++++++++++++++ > arch/powerpc/kernel/livepatch.c | 29 +++++++++++++++++++++ > include/linux/ftrace.h | 1 + > include/linux/livepatch.h | 2 ++ > kernel/livepatch/core.c | 28 +++++++++++++++++--- > kernel/trace/ftrace.c | 14 +++++++++- For the changes to the ftrace files, add my: Acked-by: Steven Rostedt <rostedt@goodmis.org> You can take the rest. -- Steve > 9 files changed, 171 insertions(+), 4 deletions(-) > create mode 100644 arch/powerpc/include/asm/livepatch.h > create mode 100644 arch/powerpc/kernel/livepatch.c > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index c2b340e23f62..fb13cd3e68f9 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -455,6 +455,7 @@ int ftrace_update_record(struct dyn_ftrace *rec, int enable); > int ftrace_test_record(struct dyn_ftrace *rec, int enable); > void ftrace_run_stop_machine(int command); > unsigned long ftrace_location(unsigned long ip); > +unsigned long ftrace_location_range(unsigned long start, unsigned long end); > unsigned long ftrace_get_addr_new(struct dyn_ftrace *rec); > unsigned long ftrace_get_addr_curr(struct dyn_ftrace *rec); > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 57a6eea84694..f4e6aae6ebe7 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -1533,7 +1533,19 @@ static int ftrace_cmp_recs(const void *a, const void *b) > return 0; > } > > -static unsigned long ftrace_location_range(unsigned long start, unsigned long end) > +/** > + * ftrace_location_range - return the first address of a traced location > + * if it touches the given ip range > + * @start: start of range to search. > + * @end: end of range to search (inclusive). @end points to the last byte > + * to check. > + * > + * Returns rec->ip if the related ftrace location is a least partly within > + * the given address range. That is, the first address of the instruction > + * that is either a NOP or call to the function tracer. It checks the ftrace > + * internal tables to determine if the address belongs or not. > + */ > +unsigned long ftrace_location_range(unsigned long start, unsigned long end) > { > struct ftrace_page *pg; > struct dyn_ftrace *rec;
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 792e169c1516..8278e5ef0dfb 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -159,6 +159,7 @@ config PPC select ARCH_HAS_DEVMEM_IS_ALLOWED select HAVE_ARCH_SECCOMP_FILTER select ARCH_HAS_UBSAN_SANITIZE_ALL + select HAVE_LIVEPATCH if HAVE_DYNAMIC_FTRACE_WITH_REGS config GENERIC_CSUM def_bool CPU_LITTLE_ENDIAN @@ -1109,3 +1110,5 @@ config PPC_LIB_RHEAP bool source "arch/powerpc/kvm/Kconfig" + +source "kernel/livepatch/Kconfig" diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h new file mode 100644 index 000000000000..b9856ceaa0cf --- /dev/null +++ b/arch/powerpc/include/asm/livepatch.h @@ -0,0 +1,47 @@ +/* + * livepatch.h - powerpc-specific Kernel Live Patching Core + * + * Copyright (C) 2015 SUSE + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ +#ifndef _ASM_POWERPC64_LIVEPATCH_H +#define _ASM_POWERPC64_LIVEPATCH_H + +#include <linux/module.h> + +#ifdef CONFIG_LIVEPATCH + +static inline int klp_check_compiler_support(void) +{ + return 0; +} + +static inline int klp_write_module_reloc(struct module *mod, unsigned long + type, unsigned long loc, unsigned long value) +{ + /* This requires infrastructure changes; we need the loadinfos. */ + return -ENOSYS; +} + +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) +{ + regs->nip = ip; +} + +#else /* CONFIG_LIVEPATCH */ +#error Include linux/livepatch.h, not asm/livepatch.h +#endif /* CONFIG_LIVEPATCH */ + +#endif /* _ASM_POWERPC64_LIVEPATCH_H */ diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index 2da380fcc34c..b767e140f040 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -119,6 +119,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o obj-$(CONFIG_TRACING) += trace_clock.o +obj-$(CONFIG_LIVEPATCH) += livepatch.o ifneq ($(CONFIG_PPC_INDIRECT_PIO),y) obj-y += iomap.o diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index ec7f8aada697..2d5333c228f1 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -1224,6 +1224,9 @@ _GLOBAL(ftrace_caller) addi r3,r3,function_trace_op@toc@l ld r5,0(r3) +#ifdef CONFIG_LIVEPATCH + mr r14,r7 /* remember old NIP */ +#endif /* Calculate ip from nip-4 into r3 for call below */ subi r3, r7, MCOUNT_INSN_SIZE @@ -1248,6 +1251,9 @@ ftrace_call: /* Load ctr with the possibly modified NIP */ ld r3, _NIP(r1) mtctr r3 +#ifdef CONFIG_LIVEPATCH + cmpd r14,r3 /* has NIP been altered? */ +#endif /* Restore gprs */ REST_8GPRS(0,r1) @@ -1265,6 +1271,31 @@ ftrace_call: ld r0, LRSAVE(r1) mtlr r0 +#ifdef CONFIG_LIVEPATCH + beq+ 4f /* likely(old_NIP == new_NIP) */ + /* + * For a local call, restore this TOC after calling the patch function. + * For a global call, it does not matter what we restore here, + * since the global caller does its own restore right afterwards, + * anyway. Just insert a klp_return_helper frame in any case, + * so a patch function can always count on the changed stack offsets. + * The patch introduces a frame such that from the patched function + * we return back to klp_return helper. For ABI compliance r12, + * lr and LRSAVE(r1) contain the address of klp_return_helper. + * We loaded ctr with the address of the patched function earlier + */ + stdu r1, -32(r1) /* open new mini stack frame */ + std r2, 24(r1) /* save TOC now, unconditionally. */ + bl 5f +5: mflr r12 + addi r12, r12, (klp_return_helper + 4 - .)@l + std r12, LRSAVE(r1) + mtlr r12 + mfctr r12 /* allow for TOC calculation in newfunc */ + bctr +4: +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER stdu r1, -112(r1) .globl ftrace_graph_call @@ -1281,6 +1312,25 @@ _GLOBAL(ftrace_graph_stub) _GLOBAL(ftrace_stub) blr +#ifdef CONFIG_LIVEPATCH +/* Helper function for local calls that are becoming global + * due to live patching. + * We can't simply patch the NOP after the original call, + * because, depending on the consistency model, some kernel + * threads may still have called the original, local function + * *without* saving their TOC in the respective stack frame slot, + * so the decision is made per-thread during function return by + * maybe inserting a klp_return_helper frame or not. +*/ +klp_return_helper: + ld r2, 24(r1) /* restore TOC (saved by ftrace_caller) */ + addi r1, r1, 32 /* destroy mini stack frame */ + ld r0, LRSAVE(r1) /* get the real return address */ + mtlr r0 + blr +#endif + + #else _GLOBAL_TOC(_mcount) /* Taken from output of objdump from lib64/glibc */ diff --git a/arch/powerpc/kernel/livepatch.c b/arch/powerpc/kernel/livepatch.c new file mode 100644 index 000000000000..267ce0fdd89b --- /dev/null +++ b/arch/powerpc/kernel/livepatch.c @@ -0,0 +1,29 @@ +/* + * livepatch.c - powerpc-specific Kernel Live Patching Core + * + * Copyright (C) 2015 SUSE + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ +#include <linux/ftrace.h> +#include <asm/livepatch.h> + +/* + * LivePatch works only with -mprofile-kernel on PPC. In this case, + * the ftrace location is always within the first 16 bytes. + */ +unsigned long klp_get_ftrace_location(unsigned long faddr) +{ + return ftrace_location_range(faddr, faddr + 16); +} diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index c2b340e23f62..fb13cd3e68f9 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -455,6 +455,7 @@ int ftrace_update_record(struct dyn_ftrace *rec, int enable); int ftrace_test_record(struct dyn_ftrace *rec, int enable); void ftrace_run_stop_machine(int command); unsigned long ftrace_location(unsigned long ip); +unsigned long ftrace_location_range(unsigned long start, unsigned long end); unsigned long ftrace_get_addr_new(struct dyn_ftrace *rec); unsigned long ftrace_get_addr_curr(struct dyn_ftrace *rec); diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index a8828652f794..25a267bcb01f 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -134,6 +134,8 @@ int klp_unregister_patch(struct klp_patch *); int klp_enable_patch(struct klp_patch *); int klp_disable_patch(struct klp_patch *); +unsigned long klp_get_ftrace_location(unsigned long faddr); + #endif /* CONFIG_LIVEPATCH */ #endif /* _LINUX_LIVEPATCH_H_ */ diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index bc2c85c064c1..9ad597faa57f 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -298,22 +298,39 @@ unlock: rcu_read_unlock(); } +/* + * Convert a function address into the appropriate ftrace location. + * + * The given address is returned on most architectures. LivePatching + * usually works only when the ftrace location is the first instruction + * in the function. + */ +unsigned long __weak klp_get_ftrace_location(unsigned long faddr) +{ + return faddr; +} + static void klp_disable_func(struct klp_func *func) { struct klp_ops *ops; + unsigned long ftrace_loc; if (WARN_ON(func->state != KLP_ENABLED)) return; if (WARN_ON(!func->old_addr)) return; + ftrace_loc = klp_get_ftrace_location(func->old_addr); + if (WARN_ON(!ftrace_loc)) + return; + ops = klp_find_ops(func->old_addr); if (WARN_ON(!ops)) return; if (list_is_singular(&ops->func_stack)) { WARN_ON(unregister_ftrace_function(&ops->fops)); - WARN_ON(ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0)); + WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0)); list_del_rcu(&func->stack_node); list_del(&ops->node); @@ -328,6 +345,7 @@ static void klp_disable_func(struct klp_func *func) static int klp_enable_func(struct klp_func *func) { struct klp_ops *ops; + unsigned long ftrace_loc; int ret; if (WARN_ON(!func->old_addr)) @@ -336,6 +354,10 @@ static int klp_enable_func(struct klp_func *func) if (WARN_ON(func->state != KLP_DISABLED)) return -EINVAL; + ftrace_loc = klp_get_ftrace_location(func->old_addr); + if (WARN_ON(!ftrace_loc)) + return -EINVAL; + ops = klp_find_ops(func->old_addr); if (!ops) { ops = kzalloc(sizeof(*ops), GFP_KERNEL); @@ -352,7 +374,7 @@ static int klp_enable_func(struct klp_func *func) INIT_LIST_HEAD(&ops->func_stack); list_add_rcu(&func->stack_node, &ops->func_stack); - ret = ftrace_set_filter_ip(&ops->fops, func->old_addr, 0, 0); + ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0); if (ret) { pr_err("failed to set ftrace filter for function '%s' (%d)\n", func->old_name, ret); @@ -363,7 +385,7 @@ static int klp_enable_func(struct klp_func *func) if (ret) { pr_err("failed to register ftrace handler for function '%s' (%d)\n", func->old_name, ret); - ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0); + ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0); goto err; } diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 57a6eea84694..f4e6aae6ebe7 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -1533,7 +1533,19 @@ static int ftrace_cmp_recs(const void *a, const void *b) return 0; } -static unsigned long ftrace_location_range(unsigned long start, unsigned long end) +/** + * ftrace_location_range - return the first address of a traced location + * if it touches the given ip range + * @start: start of range to search. + * @end: end of range to search (inclusive). @end points to the last byte + * to check. + * + * Returns rec->ip if the related ftrace location is a least partly within + * the given address range. That is, the first address of the instruction + * that is either a NOP or call to the function tracer. It checks the ftrace + * internal tables to determine if the address belongs or not. + */ +unsigned long ftrace_location_range(unsigned long start, unsigned long end) { struct ftrace_page *pg; struct dyn_ftrace *rec;