Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, Mar 09, 2016 at 05:59:40PM +1100, Balbir Singh wrote: > > Changelog v6: > 1. Experimental changes -- need loads of testing > Based on the assumption that very far TOC and LR values > indicate the call happened through a stub and the > stub return works differently from a local call which > uses klp_return_helper. Well, this is true, but the inverse is not. Less than 2GiB between LR and TOC is a necessary but not a sufficient condition for a local call. I see the problem with your code that it hardly detects calls from one module to another nearby. It will fail iff you mistake a global call to a module loaded earlier to be local while patching AND you access global data / 64-bit constants after that call before the TOC is sanitised again. Torsten ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, 9 Mar 2016, Balbir Singh wrote: > > The previous revision was nacked by Torsten, but compared to the alternatives > at hand I think we should test this approach. Ideally we want all the > complexity > of live-patching in the live-patching code and not in the patch. The other > option > is to accept v4 and document the limitation to patch writers of not patching > functions > 8 arguments or marking such functions as notrace or equivalent So I tried to read all the relevant emails and I must admit I am quite lost. Unfortunately I cannot help much with powerpc part as my knowledge is close to zero, but from live patching perspective there are only two sustainable solutions (in my opinion). First, make it work transparently for a patch writer. So no inline asm in patched functions. Second, make it impossible to patch such problematic functions and document it as a limitation. Well, this would be sad for sure. So I think we are on the same page. Hopefully. One or two nits follow. > 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; > + WARN_ON here in klp_disable_func() is reasonable, because we registered a stub for the function successfully, so something really wrong must happened... > ops = klp_find_ops(func->old_addr); > if (WARN_ON(!ops)) > return; > > if (list_is_singular(>func_stack)) { > WARN_ON(unregister_ftrace_function(>fops)); > - WARN_ON(ftrace_set_filter_ip(>fops, func->old_addr, 1, 0)); > + WARN_ON(ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0)); > > list_del_rcu(>stack_node); > list_del(>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; > + But here it might be too strong. I think simple if (!ftrace_loc) { pr_err("..."); return -EINVAL; } would be enough I guess. > 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(>func_stack); > list_add_rcu(>stack_node, >func_stack); > > - ret = ftrace_set_filter_ip(>fops, func->old_addr, 0, 0); > + ret = ftrace_set_filter_ip(>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(>fops, func->old_addr, 1, 0); > + ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0); > goto err; > } Thinking about it, we need ftrace_loc only in cases where we call ftrace_set_filter_ip() right? So we can move klp_get_ftrace_location() call to appropriate if branch both in klp_disable_func() and klp_enable_func(). Thanks, Miroslav ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On 09/03/16 20:19, Torsten Duwe wrote: > On Wed, Mar 09, 2016 at 05:59:40PM +1100, Balbir Singh wrote: >> The previous revision was nacked by Torsten, but compared to the alternatives > I nacked it because I was confident it couldn't work. Same goes > for this one, sorry. My good intention was to save us all some work. I don't doubt that. I added it to the changelog to keep the history. I've been working with the constraints we have to get a solution that does not put the burden on the patch writer. That is why this is marked experimental as it needs a lot of testing. I think we should mark livepatching on PPC as experimental to begin with >> @@ -1265,6 +1271,51 @@ ftrace_call: >> ld r0, LRSAVE(r1) >> mtlrr0 >> >> +#ifdef CONFIG_LIVEPATCH >> +beq+4f /* likely(old_NIP == new_NIP) */ >> +/* >> + * For a local call, restore this TOC after calling the patch function. > This is the key issue. > > Ftrace_caller can gather and save the current r2 contents, no problem; > but the point is, it needs to be restored *after* the replacement function. > I see 3 ways to accomplish this: > > 1st: make _every_ replacement function aware of this, and make it restore > the TOC manually just before each return statement. > Yes and I think -pg without -mprofile-kernel does a good job of doing it. In my patch I try to detect a call via stub and one without. The one with the stub will do the right thing (global calls). For local calls I have the store in CR+4 hook. > 2nd: provide a global hook to do the job, and use a stack frame to execute it. > > 3rd: have a global hook like solution 2, but let it have its own data > structure, I'd call it a "shadow stack", for the real return addresses. > See struct fgraph_cpu_data in kernel/trace/trace_functions_graph.c We thought of a shadow stack as well, but the copying can be expensive. I;ve not looked at trace_functions_graph.c in detail, will look > Using heuristics to determine whether the call was local or global > makes me feel highly uncomfortable; one day it will break and > nobody will remember why. It could break, but as with any code, the code is only as good as the test cases it passes :) We can document our design in detail > > Balbir, the problem with your patch is that it goes only half the way from > my solution 2 towards solution 1. When you call a helper function on return, > you need a place to store the real return address. > > I'll try to demonstrate a solution 1 as well, but you'll probably won't like > that either... Sure, look forward to it. I am keen on getting live-patching working. I think v4 with the documented limitation is fine - see Michael's email as well > Torsten > Thanks for looking into this, Balbir Singh. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed 2016-03-09 12:16:47, Torsten Duwe wrote: > On Wed, Mar 09, 2016 at 11:13:05AM +0100, Jiri Kosina wrote: > > On Wed, 9 Mar 2016, Torsten Duwe wrote: > > > was my first choice. Arguments on the stack? I thought we'll deal with > > > them > > > once we get there (e.g. _really_ need to patch a varargs function or one > > > with a silly signature). > > > > Well, the problem is, once such need arises, it's too late already. > > No, not if it's documented. > > > You need to be able to patch the kernels which are already out there, > > running on machines potentially for ages once all of a sudden there is a > > CVE for >8args / varargs function. > > Then you'd need a solution like I sent out yesterday, with a pre-prologue > caller that pops the extra frame, so the replacement can be more straight- > forward. Or you can just deal with the shifted offsets in the replacement. > > I'll try to demonstrate the alternative. That would then be required for > _all_ replacement functions. Or can the live patching framework differentiate > and tell ftrace_caller whether to place a stack frame or not? > > Miroslav? Petr? Can we have 2 sorts of replacement functions? I personally prefer to keep most functions without any special hack, especially when it is needed only for one architecture. If a hack is needed for "corner cases" and it is documented then, IMHO, we could live with it for some time. We test all patches anyway, so. But I could not speak for the LivePatching maintainers whose are Josh and Jiri. Best Regards, Petr ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, Mar 09, 2016 at 11:13:05AM +0100, Jiri Kosina wrote: > On Wed, 9 Mar 2016, Torsten Duwe wrote: > > was my first choice. Arguments on the stack? I thought we'll deal with them > > once we get there (e.g. _really_ need to patch a varargs function or one > > with a silly signature). > > Well, the problem is, once such need arises, it's too late already. No, not if it's documented. > You need to be able to patch the kernels which are already out there, > running on machines potentially for ages once all of a sudden there is a > CVE for >8args / varargs function. Then you'd need a solution like I sent out yesterday, with a pre-prologue caller that pops the extra frame, so the replacement can be more straight- forward. Or you can just deal with the shifted offsets in the replacement. I'll try to demonstrate the alternative. That would then be required for _all_ replacement functions. Or can the live patching framework differentiate and tell ftrace_caller whether to place a stack frame or not? Miroslav? Petr? Can we have 2 sorts of replacement functions? Torsten ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, 2016-03-09 at 11:03 +0100, Torsten Duwe wrote: > On Wed, Mar 09, 2016 at 10:44:23AM +0100, Petr Mladek wrote: > > find a solution that would work transparently. I mean that adding > > an extra hacks into selected functions in the patch might be quite > > error prone and problems hard to debug. I think that we all want this > > but I wanted to be sure :-) > > Full ACK. Again, the TOC restore needs to go _after_ the replacement function, > and the klp_return_helper works as transparently as possible, so this > was my first choice. Arguments on the stack? I thought we'll deal with them > once we get there (e.g. _really_ need to patch a varargs function or one > with a silly signature). I agree it's unlikely many people will want to patch varargs functions, or functions with stupid numbers of parameters. But at least with the current proposals, we have no way of preventing them from doing so. Which means the first sign they'll get that it doesn't work is when they've applied the patch and their production system goes down. And not even when they insert the patch, only when the patched function is called, possibly some time later. Now perhaps in reality most people are only applying livepatches from their distro, in which case the distro should have tested it. But I don't know for sure. Still I'm happy for the current solution to go in (klp_return_helper creating a minimal frame). I think we can probably come up with a fully robust solution. But not tonight, and not this week :) cheers ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, 9 Mar 2016, Torsten Duwe wrote: > > find a solution that would work transparently. I mean that adding > > an extra hacks into selected functions in the patch might be quite > > error prone and problems hard to debug. I think that we all want this > > but I wanted to be sure :-) > > Full ACK. Again, the TOC restore needs to go _after_ the replacement function, > and the klp_return_helper works as transparently as possible, so this > was my first choice. Arguments on the stack? I thought we'll deal with them > once we get there (e.g. _really_ need to patch a varargs function or one > with a silly signature). Well, the problem is, once such need arises, it's too late already. You need to be able to patch the kernels which are already out there, running on machines potentially for ages once all of a sudden there is a CVE for >8args / varargs function. At that time, only starting to put support in the kernel for that is waaay to late :) Thanks, -- Jiri Kosina SUSE Labs ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, Mar 09, 2016 at 10:44:23AM +0100, Petr Mladek wrote: > find a solution that would work transparently. I mean that adding > an extra hacks into selected functions in the patch might be quite > error prone and problems hard to debug. I think that we all want this > but I wanted to be sure :-) Full ACK. Again, the TOC restore needs to go _after_ the replacement function, and the klp_return_helper works as transparently as possible, so this was my first choice. Arguments on the stack? I thought we'll deal with them once we get there (e.g. _really_ need to patch a varargs function or one with a silly signature). Torsten ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed 2016-03-09 10:19:04, Torsten Duwe wrote: > On Wed, Mar 09, 2016 at 05:59:40PM +1100, Balbir Singh wrote: > > > > The previous revision was nacked by Torsten, but compared to the > > alternatives > > I nacked it because I was confident it couldn't work. Same goes > for this one, sorry. My good intention was to save us all some work. > > > @@ -1265,6 +1271,51 @@ ftrace_call: > > ld r0, LRSAVE(r1) > > mtlrr0 > > > > +#ifdef CONFIG_LIVEPATCH > > + beq+4f /* likely(old_NIP == new_NIP) */ > > + /* > > +* For a local call, restore this TOC after calling the patch function. > > This is the key issue. > > Ftrace_caller can gather and save the current r2 contents, no problem; > but the point is, it needs to be restored *after* the replacement function. > I see 3 ways to accomplish this: > > 1st: make _every_ replacement function aware of this, and make it restore > the TOC manually just before each return statement. > > 2nd: provide a global hook to do the job, and use a stack frame to execute it. > > 3rd: have a global hook like solution 2, but let it have its own data > structure, I'd call it a "shadow stack", for the real return addresses. > See struct fgraph_cpu_data in kernel/trace/trace_functions_graph.c > > Using heuristics to determine whether the call was local or global > makes me feel highly uncomfortable; one day it will break and > nobody will remember why. > > Balbir, the problem with your patch is that it goes only half the way from > my solution 2 towards solution 1. When you call a helper function on return, > you need a place to store the real return address. > > I'll try to demonstrate a solution 1 as well, but you'll probably won't like > that either... To be honest, I still do not have a good picture about all the problems in my head. Anyway, I would really appreciate if we could find a solution that would work transparently. I mean that adding an extra hacks into selected functions in the patch might be quite error prone and problems hard to debug. I think that we all want this but I wanted to be sure :-) BTW: I am getting close to send a patch with some basic livepatch documentation. It might be used to document "temporary" limitations. Best Regards, Petr ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH][v6][RFC] livepatch/ppc: Enable livepatching on powerpc
On Wed, Mar 09, 2016 at 05:59:40PM +1100, Balbir Singh wrote: > > The previous revision was nacked by Torsten, but compared to the alternatives I nacked it because I was confident it couldn't work. Same goes for this one, sorry. My good intention was to save us all some work. > @@ -1265,6 +1271,51 @@ ftrace_call: > ld r0, LRSAVE(r1) > mtlrr0 > > +#ifdef CONFIG_LIVEPATCH > + beq+4f /* likely(old_NIP == new_NIP) */ > + /* > + * For a local call, restore this TOC after calling the patch function. This is the key issue. Ftrace_caller can gather and save the current r2 contents, no problem; but the point is, it needs to be restored *after* the replacement function. I see 3 ways to accomplish this: 1st: make _every_ replacement function aware of this, and make it restore the TOC manually just before each return statement. 2nd: provide a global hook to do the job, and use a stack frame to execute it. 3rd: have a global hook like solution 2, but let it have its own data structure, I'd call it a "shadow stack", for the real return addresses. See struct fgraph_cpu_data in kernel/trace/trace_functions_graph.c Using heuristics to determine whether the call was local or global makes me feel highly uncomfortable; one day it will break and nobody will remember why. Balbir, the problem with your patch is that it goes only half the way from my solution 2 towards solution 1. When you call a helper function on return, you need a place to store the real return address. I'll try to demonstrate a solution 1 as well, but you'll probably won't like that either... Torsten ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev