Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 19, 2015 at 11:11:53AM +0100, Vojtech Pavlik wrote: > On Thu, Feb 19, 2015 at 10:52:51AM +0100, Peter Zijlstra wrote: > > kthread_park() functionality seems to be exactly what you want. > > It might be exactly that, indeed. The requrement of not just cleaning > up, but also not using contents of local variables from before parking > would need to be documented. > > And kernel threads would need to start using it, too. I have been able > to find one instance where this functionality is actually used. Yeah, there's work to be done there. It was introduced for the cpu hotplug stuff, and some per-cpu threads use this through the smpboot infrastructure. More need to be converted. It would be relatively straight fwd to park threaded IRQs on irq-suspend like activity for example. > So it is > again a matter of a massive patch adding that, like with the approach of > converting kernel threads to workqueues. Yeah, but not nearly all kthreads can be converted to workqueues. And there is various problems with workqueues that make it undesirable for some even if possible. > By the way, if kthread_park() was implemented all through the kernel, > would we still need the freezer for kernel threads at all? Since parking > seems to be stronger than freezing, it could also be used for that > purpose. I think not; there might of course be horrible exceptions but in general parking should be good enough indeed. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 19, 2015 at 10:52:51AM +0100, Peter Zijlstra wrote: > On Wed, Feb 18, 2015 at 09:44:44PM +0100, Vojtech Pavlik wrote: > > For live patching it doesn't matter whether code is running, sleeping or > > frozen. > > > > What matters is whether there is state before patching that may not be > > valid after patching. > > > > For userspace tasks, the exit from a syscall is a perfect moment for > > switching to the "after" state, as all stacks, and thus all local > > variables are gone and no local state exists in the kernel for the > > thread. > > > > The freezer is a logical choice for kernel threads, however, given that > > kernel threads have no defined entry/exit point and execute within a > > single main function, local variables stay and thus local state persists > > from before to after freezing. > > > > Defining that no local state within a kernel thread may be relied upon > > after exiting from the freezer is certainly possible, and is already > > true for many kernel threads. > > > > It isn't a given property of the freezer itself, though. And isn't > > obvious for author of new kernel threads either. > > > > The ideal solution would be to convert the majority of kernel threads to > > workqueues, because then there is a defined entry/exit point over which > > state isn't transferred. That is a lot of work, though, and has other > > drawbacks, particularly in the realtime space. > > kthread_park() functionality seems to be exactly what you want. It might be exactly that, indeed. The requrement of not just cleaning up, but also not using contents of local variables from before parking would need to be documented. And kernel threads would need to start using it, too. I have been able to find one instance where this functionality is actually used. So it is again a matter of a massive patch adding that, like with the approach of converting kernel threads to workqueues. By the way, if kthread_park() was implemented all through the kernel, would we still need the freezer for kernel threads at all? Since parking seems to be stronger than freezing, it could also be used for that purpose. Vojtech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 18, 2015 at 09:44:44PM +0100, Vojtech Pavlik wrote: > For live patching it doesn't matter whether code is running, sleeping or > frozen. > > What matters is whether there is state before patching that may not be > valid after patching. > > For userspace tasks, the exit from a syscall is a perfect moment for > switching to the "after" state, as all stacks, and thus all local > variables are gone and no local state exists in the kernel for the > thread. > > The freezer is a logical choice for kernel threads, however, given that > kernel threads have no defined entry/exit point and execute within a > single main function, local variables stay and thus local state persists > from before to after freezing. > > Defining that no local state within a kernel thread may be relied upon > after exiting from the freezer is certainly possible, and is already > true for many kernel threads. > > It isn't a given property of the freezer itself, though. And isn't > obvious for author of new kernel threads either. > > The ideal solution would be to convert the majority of kernel threads to > workqueues, because then there is a defined entry/exit point over which > state isn't transferred. That is a lot of work, though, and has other > drawbacks, particularly in the realtime space. kthread_park() functionality seems to be exactly what you want. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 19, 2015 at 10:52:51AM +0100, Peter Zijlstra wrote: On Wed, Feb 18, 2015 at 09:44:44PM +0100, Vojtech Pavlik wrote: For live patching it doesn't matter whether code is running, sleeping or frozen. What matters is whether there is state before patching that may not be valid after patching. For userspace tasks, the exit from a syscall is a perfect moment for switching to the after state, as all stacks, and thus all local variables are gone and no local state exists in the kernel for the thread. The freezer is a logical choice for kernel threads, however, given that kernel threads have no defined entry/exit point and execute within a single main function, local variables stay and thus local state persists from before to after freezing. Defining that no local state within a kernel thread may be relied upon after exiting from the freezer is certainly possible, and is already true for many kernel threads. It isn't a given property of the freezer itself, though. And isn't obvious for author of new kernel threads either. The ideal solution would be to convert the majority of kernel threads to workqueues, because then there is a defined entry/exit point over which state isn't transferred. That is a lot of work, though, and has other drawbacks, particularly in the realtime space. kthread_park() functionality seems to be exactly what you want. It might be exactly that, indeed. The requrement of not just cleaning up, but also not using contents of local variables from before parking would need to be documented. And kernel threads would need to start using it, too. I have been able to find one instance where this functionality is actually used. So it is again a matter of a massive patch adding that, like with the approach of converting kernel threads to workqueues. By the way, if kthread_park() was implemented all through the kernel, would we still need the freezer for kernel threads at all? Since parking seems to be stronger than freezing, it could also be used for that purpose. Vojtech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 18, 2015 at 09:44:44PM +0100, Vojtech Pavlik wrote: For live patching it doesn't matter whether code is running, sleeping or frozen. What matters is whether there is state before patching that may not be valid after patching. For userspace tasks, the exit from a syscall is a perfect moment for switching to the after state, as all stacks, and thus all local variables are gone and no local state exists in the kernel for the thread. The freezer is a logical choice for kernel threads, however, given that kernel threads have no defined entry/exit point and execute within a single main function, local variables stay and thus local state persists from before to after freezing. Defining that no local state within a kernel thread may be relied upon after exiting from the freezer is certainly possible, and is already true for many kernel threads. It isn't a given property of the freezer itself, though. And isn't obvious for author of new kernel threads either. The ideal solution would be to convert the majority of kernel threads to workqueues, because then there is a defined entry/exit point over which state isn't transferred. That is a lot of work, though, and has other drawbacks, particularly in the realtime space. kthread_park() functionality seems to be exactly what you want. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 19, 2015 at 11:11:53AM +0100, Vojtech Pavlik wrote: On Thu, Feb 19, 2015 at 10:52:51AM +0100, Peter Zijlstra wrote: kthread_park() functionality seems to be exactly what you want. It might be exactly that, indeed. The requrement of not just cleaning up, but also not using contents of local variables from before parking would need to be documented. And kernel threads would need to start using it, too. I have been able to find one instance where this functionality is actually used. Yeah, there's work to be done there. It was introduced for the cpu hotplug stuff, and some per-cpu threads use this through the smpboot infrastructure. More need to be converted. It would be relatively straight fwd to park threaded IRQs on irq-suspend like activity for example. So it is again a matter of a massive patch adding that, like with the approach of converting kernel threads to workqueues. Yeah, but not nearly all kthreads can be converted to workqueues. And there is various problems with workqueues that make it undesirable for some even if possible. By the way, if kthread_park() was implemented all through the kernel, would we still need the freezer for kernel threads at all? Since parking seems to be stronger than freezing, it could also be used for that purpose. I think not; there might of course be horrible exceptions but in general parking should be good enough indeed. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 18, 2015 at 09:17:55PM +0100, Ingo Molnar wrote: > > * Jiri Kosina wrote: > > > On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > > > > And what's wrong with using known good spots like the freezer? > > > > Quoting Tejun from the thread Jiri Slaby likely had on > > mind: > > > > "The fact that they may coincide often can be useful as a > > guideline or whatever but I'm completely against just > > mushing it together when it isn't correct. This kind of > > things quickly lead to ambiguous situations where people > > are not sure about the specific semantics or guarantees > > of the construct and implement weird voodoo code followed > > by voodoo fixes. We already had a full round of that > > with the kernel freezer itself, where people thought that > > the freezer magically makes PM work properly for a > > subsystem. Let's please not do that again." > > I don't follow this vague argument. > > The concept of 'freezing' all userspace execution is pretty > unambiguous: tasks that are running are trapped out at > known safe points such as context switch points or syscall > entry. Once all tasks have stopped, the system is frozen in > the sense that only the code we want is running, so you can > run special code without worrying about races. > > What's the problem with that? Why would it be fundamentally > unsuitable for live patching? For live patching it doesn't matter whether code is running, sleeping or frozen. What matters is whether there is state before patching that may not be valid after patching. For userspace tasks, the exit from a syscall is a perfect moment for switching to the "after" state, as all stacks, and thus all local variables are gone and no local state exists in the kernel for the thread. The freezer is a logical choice for kernel threads, however, given that kernel threads have no defined entry/exit point and execute within a single main function, local variables stay and thus local state persists from before to after freezing. Defining that no local state within a kernel thread may be relied upon after exiting from the freezer is certainly possible, and is already true for many kernel threads. It isn't a given property of the freezer itself, though. And isn't obvious for author of new kernel threads either. The ideal solution would be to convert the majority of kernel threads to workqueues, because then there is a defined entry/exit point over which state isn't transferred. That is a lot of work, though, and has other drawbacks, particularly in the realtime space. -- Vojtech Pavlik Director SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
* Jiri Kosina wrote: > On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > > And what's wrong with using known good spots like the freezer? > > Quoting Tejun from the thread Jiri Slaby likely had on > mind: > > "The fact that they may coincide often can be useful as a > guideline or whatever but I'm completely against just > mushing it together when it isn't correct. This kind of > things quickly lead to ambiguous situations where people > are not sure about the specific semantics or guarantees > of the construct and implement weird voodoo code followed > by voodoo fixes. We already had a full round of that > with the kernel freezer itself, where people thought that > the freezer magically makes PM work properly for a > subsystem. Let's please not do that again." I don't follow this vague argument. The concept of 'freezing' all userspace execution is pretty unambiguous: tasks that are running are trapped out at known safe points such as context switch points or syscall entry. Once all tasks have stopped, the system is frozen in the sense that only the code we want is running, so you can run special code without worrying about races. What's the problem with that? Why would it be fundamentally unsuitable for live patching? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, 18 Feb 2015, Josh Poimboeuf wrote: > On Wed, Feb 18, 2015 at 01:42:56PM +0100, Miroslav Benes wrote: > > On Tue, 17 Feb 2015, Josh Poimboeuf wrote: > > > > > On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote: > > > > On Tue, 17 Feb 2015, Josh Poimboeuf wrote: > > > > > > > > > On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote: > > > > > > > > > and externs for functions are redundant. > > > > > > > > > > I agree, but it seems to be the norm in Linux. I have no idea why. > > > > > I'm > > > > > just following the existing convention. > > > > > > > > Yes, I know. It seems that each author does it differently. You can > > > > find > > > > both forms even in one header file in the kernel. There is no > > > > functional > > > > difference AFAIK (it is not the case for variables of course). So as > > > > long > > > > as we are consistent I do not care. And since we have externs already > > > > in > > > > livepatch.h... you can scratch this remark if you want to :) > > > > > > Ok. If there are no objections, let's stick with our existing > > > nonsensical convention for now :-) > > > > So I was thinking about it again and we should not use bad patterns in our > > code from the beginning. Externs do not make sense so let's get rid of > > them everywhere (i.e. in the consistency model and also in livepatch.h). > > > > The C specification talks about extern in context of internal and external > > linkages or in context of inline functions but it does not make any sense > > to me. Could you look at the specification and tell me if it makes any > > sense to you, please? > > Relevant parts from C11: > > For an identifier declared with the storage-class specifier extern in a > scope in which a prior declaration of that identifier is visible, if the > prior declaration specifies internal or external linkage, the linkage of > the identifier at the later declaration is the same as the linkage > specified at the prior declaration. If no prior declaration is visible, > or if the prior declaration specifies no linkage, then the identifier > has external linkage. > > If the declaration of an identifier for a function has no storage-class > specifier, its linkage is determined exactly as if it were declared with > the storage-class specifier extern .If the declaration of an identifier > for an object has file scope and no storage-class specifier, its linkage > is external. > > Sounds to me like "extern" is redundant for functions. I'm fine with > removing it. Care to work up a patch for livepatch.h? Agreed. I'll do that. Thanks. Miroslav -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 18, 2015 at 01:42:56PM +0100, Miroslav Benes wrote: > On Tue, 17 Feb 2015, Josh Poimboeuf wrote: > > > On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote: > > > On Tue, 17 Feb 2015, Josh Poimboeuf wrote: > > > > > > > On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote: > > > > > > > and externs for functions are redundant. > > > > > > > > I agree, but it seems to be the norm in Linux. I have no idea why. I'm > > > > just following the existing convention. > > > > > > Yes, I know. It seems that each author does it differently. You can find > > > both forms even in one header file in the kernel. There is no functional > > > difference AFAIK (it is not the case for variables of course). So as long > > > as we are consistent I do not care. And since we have externs already in > > > livepatch.h... you can scratch this remark if you want to :) > > > > Ok. If there are no objections, let's stick with our existing > > nonsensical convention for now :-) > > So I was thinking about it again and we should not use bad patterns in our > code from the beginning. Externs do not make sense so let's get rid of > them everywhere (i.e. in the consistency model and also in livepatch.h). > > The C specification talks about extern in context of internal and external > linkages or in context of inline functions but it does not make any sense > to me. Could you look at the specification and tell me if it makes any > sense to you, please? Relevant parts from C11: For an identifier declared with the storage-class specifier extern in a scope in which a prior declaration of that identifier is visible, if the prior declaration specifies internal or external linkage, the linkage of the identifier at the later declaration is the same as the linkage specified at the prior declaration. If no prior declaration is visible, or if the prior declaration specifies no linkage, then the identifier has external linkage. If the declaration of an identifier for a function has no storage-class specifier, its linkage is determined exactly as if it were declared with the storage-class specifier extern .If the declaration of an identifier for an object has file scope and no storage-class specifier, its linkage is external. Sounds to me like "extern" is redundant for functions. I'm fine with removing it. Care to work up a patch for livepatch.h? > > Jiri, Vojtech, do you have any opinion about this? > > Miroslav -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, 17 Feb 2015, Josh Poimboeuf wrote: > On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote: > > On Tue, 17 Feb 2015, Josh Poimboeuf wrote: > > > > > On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote: > > > > > and externs for functions are redundant. > > > > > > I agree, but it seems to be the norm in Linux. I have no idea why. I'm > > > just following the existing convention. > > > > Yes, I know. It seems that each author does it differently. You can find > > both forms even in one header file in the kernel. There is no functional > > difference AFAIK (it is not the case for variables of course). So as long > > as we are consistent I do not care. And since we have externs already in > > livepatch.h... you can scratch this remark if you want to :) > > Ok. If there are no objections, let's stick with our existing > nonsensical convention for now :-) So I was thinking about it again and we should not use bad patterns in our code from the beginning. Externs do not make sense so let's get rid of them everywhere (i.e. in the consistency model and also in livepatch.h). The C specification talks about extern in context of internal and external linkages or in context of inline functions but it does not make any sense to me. Could you look at the specification and tell me if it makes any sense to you, please? Jiri, Vojtech, do you have any opinion about this? Miroslav -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 18, 2015 at 09:17:55PM +0100, Ingo Molnar wrote: * Jiri Kosina jkos...@suse.cz wrote: On Thu, 12 Feb 2015, Peter Zijlstra wrote: And what's wrong with using known good spots like the freezer? Quoting Tejun from the thread Jiri Slaby likely had on mind: The fact that they may coincide often can be useful as a guideline or whatever but I'm completely against just mushing it together when it isn't correct. This kind of things quickly lead to ambiguous situations where people are not sure about the specific semantics or guarantees of the construct and implement weird voodoo code followed by voodoo fixes. We already had a full round of that with the kernel freezer itself, where people thought that the freezer magically makes PM work properly for a subsystem. Let's please not do that again. I don't follow this vague argument. The concept of 'freezing' all userspace execution is pretty unambiguous: tasks that are running are trapped out at known safe points such as context switch points or syscall entry. Once all tasks have stopped, the system is frozen in the sense that only the code we want is running, so you can run special code without worrying about races. What's the problem with that? Why would it be fundamentally unsuitable for live patching? For live patching it doesn't matter whether code is running, sleeping or frozen. What matters is whether there is state before patching that may not be valid after patching. For userspace tasks, the exit from a syscall is a perfect moment for switching to the after state, as all stacks, and thus all local variables are gone and no local state exists in the kernel for the thread. The freezer is a logical choice for kernel threads, however, given that kernel threads have no defined entry/exit point and execute within a single main function, local variables stay and thus local state persists from before to after freezing. Defining that no local state within a kernel thread may be relied upon after exiting from the freezer is certainly possible, and is already true for many kernel threads. It isn't a given property of the freezer itself, though. And isn't obvious for author of new kernel threads either. The ideal solution would be to convert the majority of kernel threads to workqueues, because then there is a defined entry/exit point over which state isn't transferred. That is a lot of work, though, and has other drawbacks, particularly in the realtime space. -- Vojtech Pavlik Director SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
* Jiri Kosina jkos...@suse.cz wrote: On Thu, 12 Feb 2015, Peter Zijlstra wrote: And what's wrong with using known good spots like the freezer? Quoting Tejun from the thread Jiri Slaby likely had on mind: The fact that they may coincide often can be useful as a guideline or whatever but I'm completely against just mushing it together when it isn't correct. This kind of things quickly lead to ambiguous situations where people are not sure about the specific semantics or guarantees of the construct and implement weird voodoo code followed by voodoo fixes. We already had a full round of that with the kernel freezer itself, where people thought that the freezer magically makes PM work properly for a subsystem. Let's please not do that again. I don't follow this vague argument. The concept of 'freezing' all userspace execution is pretty unambiguous: tasks that are running are trapped out at known safe points such as context switch points or syscall entry. Once all tasks have stopped, the system is frozen in the sense that only the code we want is running, so you can run special code without worrying about races. What's the problem with that? Why would it be fundamentally unsuitable for live patching? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, 18 Feb 2015, Josh Poimboeuf wrote: On Wed, Feb 18, 2015 at 01:42:56PM +0100, Miroslav Benes wrote: On Tue, 17 Feb 2015, Josh Poimboeuf wrote: On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote: On Tue, 17 Feb 2015, Josh Poimboeuf wrote: On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote: and externs for functions are redundant. I agree, but it seems to be the norm in Linux. I have no idea why. I'm just following the existing convention. Yes, I know. It seems that each author does it differently. You can find both forms even in one header file in the kernel. There is no functional difference AFAIK (it is not the case for variables of course). So as long as we are consistent I do not care. And since we have externs already in livepatch.h... you can scratch this remark if you want to :) Ok. If there are no objections, let's stick with our existing nonsensical convention for now :-) So I was thinking about it again and we should not use bad patterns in our code from the beginning. Externs do not make sense so let's get rid of them everywhere (i.e. in the consistency model and also in livepatch.h). The C specification talks about extern in context of internal and external linkages or in context of inline functions but it does not make any sense to me. Could you look at the specification and tell me if it makes any sense to you, please? Relevant parts from C11: For an identifier declared with the storage-class specifier extern in a scope in which a prior declaration of that identifier is visible, if the prior declaration specifies internal or external linkage, the linkage of the identifier at the later declaration is the same as the linkage specified at the prior declaration. If no prior declaration is visible, or if the prior declaration specifies no linkage, then the identifier has external linkage. If the declaration of an identifier for a function has no storage-class specifier, its linkage is determined exactly as if it were declared with the storage-class specifier extern .If the declaration of an identifier for an object has file scope and no storage-class specifier, its linkage is external. Sounds to me like extern is redundant for functions. I'm fine with removing it. Care to work up a patch for livepatch.h? Agreed. I'll do that. Thanks. Miroslav -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, 17 Feb 2015, Josh Poimboeuf wrote: On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote: On Tue, 17 Feb 2015, Josh Poimboeuf wrote: On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote: and externs for functions are redundant. I agree, but it seems to be the norm in Linux. I have no idea why. I'm just following the existing convention. Yes, I know. It seems that each author does it differently. You can find both forms even in one header file in the kernel. There is no functional difference AFAIK (it is not the case for variables of course). So as long as we are consistent I do not care. And since we have externs already in livepatch.h... you can scratch this remark if you want to :) Ok. If there are no objections, let's stick with our existing nonsensical convention for now :-) So I was thinking about it again and we should not use bad patterns in our code from the beginning. Externs do not make sense so let's get rid of them everywhere (i.e. in the consistency model and also in livepatch.h). The C specification talks about extern in context of internal and external linkages or in context of inline functions but it does not make any sense to me. Could you look at the specification and tell me if it makes any sense to you, please? Jiri, Vojtech, do you have any opinion about this? Miroslav -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 18, 2015 at 01:42:56PM +0100, Miroslav Benes wrote: On Tue, 17 Feb 2015, Josh Poimboeuf wrote: On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote: On Tue, 17 Feb 2015, Josh Poimboeuf wrote: On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote: and externs for functions are redundant. I agree, but it seems to be the norm in Linux. I have no idea why. I'm just following the existing convention. Yes, I know. It seems that each author does it differently. You can find both forms even in one header file in the kernel. There is no functional difference AFAIK (it is not the case for variables of course). So as long as we are consistent I do not care. And since we have externs already in livepatch.h... you can scratch this remark if you want to :) Ok. If there are no objections, let's stick with our existing nonsensical convention for now :-) So I was thinking about it again and we should not use bad patterns in our code from the beginning. Externs do not make sense so let's get rid of them everywhere (i.e. in the consistency model and also in livepatch.h). The C specification talks about extern in context of internal and external linkages or in context of inline functions but it does not make any sense to me. Could you look at the specification and tell me if it makes any sense to you, please? Relevant parts from C11: For an identifier declared with the storage-class specifier extern in a scope in which a prior declaration of that identifier is visible, if the prior declaration specifies internal or external linkage, the linkage of the identifier at the later declaration is the same as the linkage specified at the prior declaration. If no prior declaration is visible, or if the prior declaration specifies no linkage, then the identifier has external linkage. If the declaration of an identifier for a function has no storage-class specifier, its linkage is determined exactly as if it were declared with the storage-class specifier extern .If the declaration of an identifier for an object has file scope and no storage-class specifier, its linkage is external. Sounds to me like extern is redundant for functions. I'm fine with removing it. Care to work up a patch for livepatch.h? Jiri, Vojtech, do you have any opinion about this? Miroslav -- Josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote:
> On Tue, 17 Feb 2015, Josh Poimboeuf wrote:
>
> > On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
> > > On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> > >
>
> [...]
>
> > > > +
> > > > +void klp_unpatch_objects(struct klp_patch *patch)
> > > > +{
> > > > + struct klp_object *obj;
> > > > +
> > > > + for (obj = patch->objs; obj->funcs; obj++)
> > > > + if (obj->patched)
> > > > + klp_unpatch_object(obj);
> > > > +}
> > >
> > > Maybe we should introduce for_each_* macros which could be used in the
> > > code and avoid such functions. I do not have strong opinion about it.
> >
> > Yeah, but each such loop seems to differ a little bit, so I'm not quite
> > sure how to structure the macros such that they'd be useful. Maybe for
> > a future patch.
>
> Yes, that is correct. The code in the caller of klp_unpatch_objects would
> look something like this
>
> klp_for_each_object(obj, patch->objs)
> if (obj->patched)
> klp_unpatch_object(obj)
Yeah, that is slightly more readable and less error prone. I'll do it.
> > > and externs for functions are redundant.
> >
> > I agree, but it seems to be the norm in Linux. I have no idea why. I'm
> > just following the existing convention.
>
> Yes, I know. It seems that each author does it differently. You can find
> both forms even in one header file in the kernel. There is no functional
> difference AFAIK (it is not the case for variables of course). So as long
> as we are consistent I do not care. And since we have externs already in
> livepatch.h... you can scratch this remark if you want to :)
Ok. If there are no objections, let's stick with our existing
nonsensical convention for now :-)
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, 17 Feb 2015, Josh Poimboeuf wrote:
> On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
> > On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> >
[...]
> > > +
> > > +void klp_unpatch_objects(struct klp_patch *patch)
> > > +{
> > > + struct klp_object *obj;
> > > +
> > > + for (obj = patch->objs; obj->funcs; obj++)
> > > + if (obj->patched)
> > > + klp_unpatch_object(obj);
> > > +}
> >
> > Maybe we should introduce for_each_* macros which could be used in the
> > code and avoid such functions. I do not have strong opinion about it.
>
> Yeah, but each such loop seems to differ a little bit, so I'm not quite
> sure how to structure the macros such that they'd be useful. Maybe for
> a future patch.
Yes, that is correct. The code in the caller of klp_unpatch_objects would
look something like this
klp_for_each_object(obj, patch->objs)
if (obj->patched)
klp_unpatch_object(obj)
So there is in fact no change (compared to opencoding of
klp_unpatch_objects), but IMO it is more legible. The upside is
that we wouldn't introduce functions with similar names which could be
confusing in the future AND we could use such macros throughout the code.
One step more could be macro klp_for_each_patched_object which would
include the check.
However it is a nitpick, matter of taste and it is up to you.
>
> > > diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
> > > index bb34bd3..1648259 100644
> > > --- a/kernel/livepatch/patch.h
> > > +++ b/kernel/livepatch/patch.h
> > > @@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
> > >
> > > extern int klp_patch_object(struct klp_object *obj);
> > > extern void klp_unpatch_object(struct klp_object *obj);
> > > +extern void klp_unpatch_objects(struct klp_patch *patch);
> >
> > [...]
> >
> > > diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> > > new file mode 100644
> > > index 000..ba9a55c
> > > --- /dev/null
> > > +++ b/kernel/livepatch/transition.h
> > > @@ -0,0 +1,16 @@
> > > +#include
> > > +
> > > +enum {
> > > + KLP_UNIVERSE_UNDEFINED = -1,
> > > + KLP_UNIVERSE_OLD,
> > > + KLP_UNIVERSE_NEW,
> > > +};
> > > +
> > > +extern struct mutex klp_mutex;
> > > +extern struct klp_patch *klp_transition_patch;
> > > +
> > > +extern void klp_init_transition(struct klp_patch *patch, int universe);
> > > +extern void klp_start_transition(int universe);
> > > +extern void klp_reverse_transition(void);
> > > +extern void klp_try_complete_transition(void);
> > > +extern void klp_complete_transition(void);
> >
> > Double inclusion protection is missing
>
> Ok.
>
> > and externs for functions are redundant.
>
> I agree, but it seems to be the norm in Linux. I have no idea why. I'm
> just following the existing convention.
Yes, I know. It seems that each author does it differently. You can find
both forms even in one header file in the kernel. There is no functional
difference AFAIK (it is not the case for variables of course). So as long
as we are consistent I do not care. And since we have externs already in
livepatch.h... you can scratch this remark if you want to :)
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Sat, Feb 14, 2015 at 12:40:01PM +0100, Jiri Slaby wrote:
> On 02/09/2015, 06:31 PM, Josh Poimboeuf wrote:
> > Add a basic per-task consistency model. This is the foundation which
> > will eventually enable us to patch those ~10% of security patches which
> > change function prototypes and/or data semantics.
> >
> > When a patch is enabled, livepatch enters into a transition state where
> > tasks are converging from the old universe to the new universe. If a
> > given task isn't using any of the patched functions, it's switched to
> > the new universe. Once all the tasks have been converged to the new
> > universe, patching is complete.
> >
> > The same sequence occurs when a patch is disabled, except the tasks
> > converge from the new universe to the old universe.
> >
> > The /sys/kernel/livepatch//transition file shows whether a patch
> > is in transition. Only a single patch (the topmost patch on the stack)
> > can be in transition at a given time. A patch can remain in the
> > transition state indefinitely, if any of the tasks are stuck in the
> > previous universe.
> >
> > A transition can be reversed and effectively canceled by writing the
> > opposite value to the /sys/kernel/livepatch//enabled file while
> > the transition is in progress. Then all the tasks will attempt to
> > converge back to the original universe.
> >
> > Signed-off-by: Josh Poimboeuf
> > ---
> > include/linux/livepatch.h | 18 ++-
> > include/linux/sched.h | 3 +
> > kernel/fork.c | 2 +
> > kernel/livepatch/Makefile | 2 +-
> > kernel/livepatch/core.c | 71 ++
> > kernel/livepatch/patch.c | 34 -
> > kernel/livepatch/patch.h | 1 +
> > kernel/livepatch/transition.c | 300
> > ++
> > kernel/livepatch/transition.h | 16 +++
> > kernel/sched/core.c | 2 +
> > 10 files changed, 423 insertions(+), 26 deletions(-)
> > create mode 100644 kernel/livepatch/transition.c
> > create mode 100644 kernel/livepatch/transition.h
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 0e65b4d..b8c2f15 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -40,6 +40,7 @@
> > * @old_size: size of the old function
> > * @new_size: size of the new function
> > * @patched: the func has been added to the klp_ops list
> > + * @transition:the func is currently being applied or reverted
> > */
> > struct klp_func {
> > /* external */
> > @@ -60,6 +61,7 @@ struct klp_func {
> > struct list_head stack_node;
> > unsigned long old_size, new_size;
> > int patched;
> > + int transition;
> > };
> >
> > /**
> > @@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
> > extern int klp_enable_patch(struct klp_patch *);
> > extern int klp_disable_patch(struct klp_patch *);
> >
> > -#endif /* CONFIG_LIVEPATCH */
> > +extern int klp_universe_goal;
> > +
> > +static inline void klp_update_task_universe(struct task_struct *t)
> > +{
> > + /* corresponding smp_wmb() is in klp_set_universe_goal() */
> > + smp_rmb();
> > +
> > + t->klp_universe = klp_universe_goal;
> > +}
> > +
> > +#else /* !CONFIG_LIVEPATCH */
> > +
> > +static inline void klp_update_task_universe(struct task_struct *t) {}
> > +
> > +#endif /* !CONFIG_LIVEPATCH */
> >
> > #endif /* _LINUX_LIVEPATCH_H_ */
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 8db31ef..a95e59a 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1701,6 +1701,9 @@ struct task_struct {
> > #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> > unsigned long task_state_change;
> > #endif
> > +#ifdef CONFIG_LIVEPATCH
> > + int klp_universe;
> > +#endif
> > };
> >
> > /* Future-safe accessor for struct task_struct's cpus_allowed. */
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 4dc2dda..1dcbebe 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -74,6 +74,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
> > clone_flags,
> > total_forks++;
> > spin_unlock(>sighand->siglock);
> > syscall_tracepoint_update(p);
> > + klp_update_task_universe(p);
> > write_unlock_irq(_lock);
> >
> > proc_fork_connector(p);
> > diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> > index e136dad..2b8bdb1 100644
> > --- a/kernel/livepatch/Makefile
> > +++ b/kernel/livepatch/Makefile
> > @@ -1,3 +1,3 @@
> > obj-$(CONFIG_LIVEPATCH) += livepatch.o
> >
> > -livepatch-objs := core.o patch.o
> > +livepatch-objs := core.o patch.o transition.o
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 85d4ef7..790dc10 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -28,14 +28,17 @@
> >
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
> On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
>
> > Add a basic per-task consistency model. This is the foundation which
> > will eventually enable us to patch those ~10% of security patches which
> > change function prototypes and/or data semantics.
> >
> > When a patch is enabled, livepatch enters into a transition state where
> > tasks are converging from the old universe to the new universe. If a
> > given task isn't using any of the patched functions, it's switched to
> > the new universe. Once all the tasks have been converged to the new
> > universe, patching is complete.
> >
> > The same sequence occurs when a patch is disabled, except the tasks
> > converge from the new universe to the old universe.
> >
> > The /sys/kernel/livepatch//transition file shows whether a patch
> > is in transition. Only a single patch (the topmost patch on the stack)
> > can be in transition at a given time. A patch can remain in the
> > transition state indefinitely, if any of the tasks are stuck in the
> > previous universe.
> >
> > A transition can be reversed and effectively canceled by writing the
> > opposite value to the /sys/kernel/livepatch//enabled file while
> > the transition is in progress. Then all the tasks will attempt to
> > converge back to the original universe.
>
> I finally managed to go through this patch and I have only few comments
> apart from what Jiri has already written...
>
> I think it would be useful to add more comments throughout the code.
Ok, I'll try to add more comments throughout.
> sysfs documentation (Documentation/ABI/testing/sysfs-kernel-livepatch)
> should be updated as well. Also the meaning of enabled attribute was
> changed a bit (by different patch of the set though).
Ok.
> > +
> > +void klp_unpatch_objects(struct klp_patch *patch)
> > +{
> > + struct klp_object *obj;
> > +
> > + for (obj = patch->objs; obj->funcs; obj++)
> > + if (obj->patched)
> > + klp_unpatch_object(obj);
> > +}
>
> Maybe we should introduce for_each_* macros which could be used in the
> code and avoid such functions. I do not have strong opinion about it.
Yeah, but each such loop seems to differ a little bit, so I'm not quite
sure how to structure the macros such that they'd be useful. Maybe for
a future patch.
> > diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
> > index bb34bd3..1648259 100644
> > --- a/kernel/livepatch/patch.h
> > +++ b/kernel/livepatch/patch.h
> > @@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
> >
> > extern int klp_patch_object(struct klp_object *obj);
> > extern void klp_unpatch_object(struct klp_object *obj);
> > +extern void klp_unpatch_objects(struct klp_patch *patch);
>
> [...]
>
> > diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> > new file mode 100644
> > index 000..ba9a55c
> > --- /dev/null
> > +++ b/kernel/livepatch/transition.h
> > @@ -0,0 +1,16 @@
> > +#include
> > +
> > +enum {
> > + KLP_UNIVERSE_UNDEFINED = -1,
> > + KLP_UNIVERSE_OLD,
> > + KLP_UNIVERSE_NEW,
> > +};
> > +
> > +extern struct mutex klp_mutex;
> > +extern struct klp_patch *klp_transition_patch;
> > +
> > +extern void klp_init_transition(struct klp_patch *patch, int universe);
> > +extern void klp_start_transition(int universe);
> > +extern void klp_reverse_transition(void);
> > +extern void klp_try_complete_transition(void);
> > +extern void klp_complete_transition(void);
>
> Double inclusion protection is missing
Ok.
> and externs for functions are redundant.
I agree, but it seems to be the norm in Linux. I have no idea why. I'm
just following the existing convention.
> Otherwise it looks quite ok.
Thanks!
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 17, 2015 at 04:48:39PM +0100, Miroslav Benes wrote:
On Tue, 17 Feb 2015, Josh Poimboeuf wrote:
On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
[...]
+
+void klp_unpatch_objects(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+
+ for (obj = patch-objs; obj-funcs; obj++)
+ if (obj-patched)
+ klp_unpatch_object(obj);
+}
Maybe we should introduce for_each_* macros which could be used in the
code and avoid such functions. I do not have strong opinion about it.
Yeah, but each such loop seems to differ a little bit, so I'm not quite
sure how to structure the macros such that they'd be useful. Maybe for
a future patch.
Yes, that is correct. The code in the caller of klp_unpatch_objects would
look something like this
klp_for_each_object(obj, patch-objs)
if (obj-patched)
klp_unpatch_object(obj)
Yeah, that is slightly more readable and less error prone. I'll do it.
and externs for functions are redundant.
I agree, but it seems to be the norm in Linux. I have no idea why. I'm
just following the existing convention.
Yes, I know. It seems that each author does it differently. You can find
both forms even in one header file in the kernel. There is no functional
difference AFAIK (it is not the case for variables of course). So as long
as we are consistent I do not care. And since we have externs already in
livepatch.h... you can scratch this remark if you want to :)
Ok. If there are no objections, let's stick with our existing
nonsensical convention for now :-)
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Sat, Feb 14, 2015 at 12:40:01PM +0100, Jiri Slaby wrote:
On 02/09/2015, 06:31 PM, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Signed-off-by: Josh Poimboeuf jpoim...@redhat.com
---
include/linux/livepatch.h | 18 ++-
include/linux/sched.h | 3 +
kernel/fork.c | 2 +
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 71 ++
kernel/livepatch/patch.c | 34 -
kernel/livepatch/patch.h | 1 +
kernel/livepatch/transition.c | 300
++
kernel/livepatch/transition.h | 16 +++
kernel/sched/core.c | 2 +
10 files changed, 423 insertions(+), 26 deletions(-)
create mode 100644 kernel/livepatch/transition.c
create mode 100644 kernel/livepatch/transition.h
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0e65b4d..b8c2f15 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -40,6 +40,7 @@
* @old_size: size of the old function
* @new_size: size of the new function
* @patched: the func has been added to the klp_ops list
+ * @transition:the func is currently being applied or reverted
*/
struct klp_func {
/* external */
@@ -60,6 +61,7 @@ struct klp_func {
struct list_head stack_node;
unsigned long old_size, new_size;
int patched;
+ int transition;
};
/**
@@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
extern int klp_enable_patch(struct klp_patch *);
extern int klp_disable_patch(struct klp_patch *);
-#endif /* CONFIG_LIVEPATCH */
+extern int klp_universe_goal;
+
+static inline void klp_update_task_universe(struct task_struct *t)
+{
+ /* corresponding smp_wmb() is in klp_set_universe_goal() */
+ smp_rmb();
+
+ t-klp_universe = klp_universe_goal;
+}
+
+#else /* !CONFIG_LIVEPATCH */
+
+static inline void klp_update_task_universe(struct task_struct *t) {}
+
+#endif /* !CONFIG_LIVEPATCH */
#endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8db31ef..a95e59a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1701,6 +1701,9 @@ struct task_struct {
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
unsigned long task_state_change;
#endif
+#ifdef CONFIG_LIVEPATCH
+ int klp_universe;
+#endif
};
/* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4dc2dda..1dcbebe 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -74,6 +74,7 @@
#include linux/uprobes.h
#include linux/aio.h
#include linux/compiler.h
+#include linux/livepatch.h
#include asm/pgtable.h
#include asm/pgalloc.h
@@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
total_forks++;
spin_unlock(current-sighand-siglock);
syscall_tracepoint_update(p);
+ klp_update_task_universe(p);
write_unlock_irq(tasklist_lock);
proc_fork_connector(p);
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e136dad..2b8bdb1 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
obj-$(CONFIG_LIVEPATCH) += livepatch.o
-livepatch-objs := core.o patch.o
+livepatch-objs := core.o patch.o transition.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 85d4ef7..790dc10 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,14 +28,17 @@
#include linux/kallsyms.h
#include patch.h
+#include transition.h
/*
- * The klp_mutex protects the global lists and state
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, 17 Feb 2015, Josh Poimboeuf wrote:
On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
[...]
+
+void klp_unpatch_objects(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+
+ for (obj = patch-objs; obj-funcs; obj++)
+ if (obj-patched)
+ klp_unpatch_object(obj);
+}
Maybe we should introduce for_each_* macros which could be used in the
code and avoid such functions. I do not have strong opinion about it.
Yeah, but each such loop seems to differ a little bit, so I'm not quite
sure how to structure the macros such that they'd be useful. Maybe for
a future patch.
Yes, that is correct. The code in the caller of klp_unpatch_objects would
look something like this
klp_for_each_object(obj, patch-objs)
if (obj-patched)
klp_unpatch_object(obj)
So there is in fact no change (compared to opencoding of
klp_unpatch_objects), but IMO it is more legible. The upside is
that we wouldn't introduce functions with similar names which could be
confusing in the future AND we could use such macros throughout the code.
One step more could be macro klp_for_each_patched_object which would
include the check.
However it is a nitpick, matter of taste and it is up to you.
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index bb34bd3..1648259 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
extern int klp_patch_object(struct klp_object *obj);
extern void klp_unpatch_object(struct klp_object *obj);
+extern void klp_unpatch_objects(struct klp_patch *patch);
[...]
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 000..ba9a55c
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,16 @@
+#include linux/livepatch.h
+
+enum {
+ KLP_UNIVERSE_UNDEFINED = -1,
+ KLP_UNIVERSE_OLD,
+ KLP_UNIVERSE_NEW,
+};
+
+extern struct mutex klp_mutex;
+extern struct klp_patch *klp_transition_patch;
+
+extern void klp_init_transition(struct klp_patch *patch, int universe);
+extern void klp_start_transition(int universe);
+extern void klp_reverse_transition(void);
+extern void klp_try_complete_transition(void);
+extern void klp_complete_transition(void);
Double inclusion protection is missing
Ok.
and externs for functions are redundant.
I agree, but it seems to be the norm in Linux. I have no idea why. I'm
just following the existing convention.
Yes, I know. It seems that each author does it differently. You can find
both forms even in one header file in the kernel. There is no functional
difference AFAIK (it is not the case for variables of course). So as long
as we are consistent I do not care. And since we have externs already in
livepatch.h... you can scratch this remark if you want to :)
Miroslav
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
I finally managed to go through this patch and I have only few comments
apart from what Jiri has already written...
I think it would be useful to add more comments throughout the code.
Ok, I'll try to add more comments throughout.
sysfs documentation (Documentation/ABI/testing/sysfs-kernel-livepatch)
should be updated as well. Also the meaning of enabled attribute was
changed a bit (by different patch of the set though).
Ok.
+
+void klp_unpatch_objects(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+
+ for (obj = patch-objs; obj-funcs; obj++)
+ if (obj-patched)
+ klp_unpatch_object(obj);
+}
Maybe we should introduce for_each_* macros which could be used in the
code and avoid such functions. I do not have strong opinion about it.
Yeah, but each such loop seems to differ a little bit, so I'm not quite
sure how to structure the macros such that they'd be useful. Maybe for
a future patch.
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index bb34bd3..1648259 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
extern int klp_patch_object(struct klp_object *obj);
extern void klp_unpatch_object(struct klp_object *obj);
+extern void klp_unpatch_objects(struct klp_patch *patch);
[...]
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 000..ba9a55c
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,16 @@
+#include linux/livepatch.h
+
+enum {
+ KLP_UNIVERSE_UNDEFINED = -1,
+ KLP_UNIVERSE_OLD,
+ KLP_UNIVERSE_NEW,
+};
+
+extern struct mutex klp_mutex;
+extern struct klp_patch *klp_transition_patch;
+
+extern void klp_init_transition(struct klp_patch *patch, int universe);
+extern void klp_start_transition(int universe);
+extern void klp_reverse_transition(void);
+extern void klp_try_complete_transition(void);
+extern void klp_complete_transition(void);
Double inclusion protection is missing
Ok.
and externs for functions are redundant.
I agree, but it seems to be the norm in Linux. I have no idea why. I'm
just following the existing convention.
Otherwise it looks quite ok.
Thanks!
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which
> will eventually enable us to patch those ~10% of security patches which
> change function prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
I finally managed to go through this patch and I have only few comments
apart from what Jiri has already written...
I think it would be useful to add more comments throughout the code.
[...]
> /**
> @@ -407,6 +418,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
> * /sys/kernel/livepatch
> * /sys/kernel/livepatch/
> * /sys/kernel/livepatch//enabled
> + * /sys/kernel/livepatch//transition
> * /sys/kernel/livepatch//
> * /sys/kernel/livepatch///
> */
> @@ -435,7 +447,9 @@ static ssize_t enabled_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> goto err;
> }
>
> - if (val) {
> + if (klp_transition_patch == patch) {
> + klp_reverse_transition();
> + } else if (val) {
> ret = __klp_enable_patch(patch);
> if (ret)
> goto err;
> @@ -463,9 +477,21 @@ static ssize_t enabled_show(struct kobject *kobj,
> return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
> }
>
> +static ssize_t transition_show(struct kobject *kobj,
> +struct kobj_attribute *attr, char *buf)
> +{
> + struct klp_patch *patch;
> +
> + patch = container_of(kobj, struct klp_patch, kobj);
> + return snprintf(buf, PAGE_SIZE-1, "%d\n",
> + klp_transition_patch == patch);
> +}
> +
> static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
> +static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
> static struct attribute *klp_patch_attrs[] = {
> _kobj_attr.attr,
> + _kobj_attr.attr,
> NULL
> };
sysfs documentation (Documentation/ABI/testing/sysfs-kernel-livepatch)
should be updated as well. Also the meaning of enabled attribute was
changed a bit (by different patch of the set though).
[...]
> +
> +void klp_unpatch_objects(struct klp_patch *patch)
> +{
> + struct klp_object *obj;
> +
> + for (obj = patch->objs; obj->funcs; obj++)
> + if (obj->patched)
> + klp_unpatch_object(obj);
> +}
Maybe we should introduce for_each_* macros which could be used in the
code and avoid such functions. I do not have strong opinion about it.
> diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
> index bb34bd3..1648259 100644
> --- a/kernel/livepatch/patch.h
> +++ b/kernel/livepatch/patch.h
> @@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
>
> extern int klp_patch_object(struct klp_object *obj);
> extern void klp_unpatch_object(struct klp_object *obj);
> +extern void klp_unpatch_objects(struct klp_patch *patch);
[...]
> diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> new file mode 100644
> index 000..ba9a55c
> --- /dev/null
> +++ b/kernel/livepatch/transition.h
> @@ -0,0 +1,16 @@
> +#include
> +
> +enum {
> + KLP_UNIVERSE_UNDEFINED = -1,
> + KLP_UNIVERSE_OLD,
> + KLP_UNIVERSE_NEW,
> +};
> +
> +extern struct mutex klp_mutex;
> +extern struct klp_patch *klp_transition_patch;
> +
> +extern void klp_init_transition(struct klp_patch *patch, int universe);
> +extern void klp_start_transition(int universe);
> +extern void klp_reverse_transition(void);
> +extern void klp_try_complete_transition(void);
> +extern void klp_complete_transition(void);
Double inclusion protection is missing and externs for functions are
redundant.
Otherwise it looks quite ok.
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
I finally managed to go through this patch and I have only few comments
apart from what Jiri has already written...
I think it would be useful to add more comments throughout the code.
[...]
/**
@@ -407,6 +418,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
* /sys/kernel/livepatch
* /sys/kernel/livepatch/patch
* /sys/kernel/livepatch/patch/enabled
+ * /sys/kernel/livepatch/patch/transition
* /sys/kernel/livepatch/patch/object
* /sys/kernel/livepatch/patch/object/func
*/
@@ -435,7 +447,9 @@ static ssize_t enabled_store(struct kobject *kobj, struct
kobj_attribute *attr,
goto err;
}
- if (val) {
+ if (klp_transition_patch == patch) {
+ klp_reverse_transition();
+ } else if (val) {
ret = __klp_enable_patch(patch);
if (ret)
goto err;
@@ -463,9 +477,21 @@ static ssize_t enabled_show(struct kobject *kobj,
return snprintf(buf, PAGE_SIZE-1, %d\n, patch-enabled);
}
+static ssize_t transition_show(struct kobject *kobj,
+struct kobj_attribute *attr, char *buf)
+{
+ struct klp_patch *patch;
+
+ patch = container_of(kobj, struct klp_patch, kobj);
+ return snprintf(buf, PAGE_SIZE-1, %d\n,
+ klp_transition_patch == patch);
+}
+
static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
+static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
static struct attribute *klp_patch_attrs[] = {
enabled_kobj_attr.attr,
+ transition_kobj_attr.attr,
NULL
};
sysfs documentation (Documentation/ABI/testing/sysfs-kernel-livepatch)
should be updated as well. Also the meaning of enabled attribute was
changed a bit (by different patch of the set though).
[...]
+
+void klp_unpatch_objects(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+
+ for (obj = patch-objs; obj-funcs; obj++)
+ if (obj-patched)
+ klp_unpatch_object(obj);
+}
Maybe we should introduce for_each_* macros which could be used in the
code and avoid such functions. I do not have strong opinion about it.
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index bb34bd3..1648259 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
extern int klp_patch_object(struct klp_object *obj);
extern void klp_unpatch_object(struct klp_object *obj);
+extern void klp_unpatch_objects(struct klp_patch *patch);
[...]
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 000..ba9a55c
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,16 @@
+#include linux/livepatch.h
+
+enum {
+ KLP_UNIVERSE_UNDEFINED = -1,
+ KLP_UNIVERSE_OLD,
+ KLP_UNIVERSE_NEW,
+};
+
+extern struct mutex klp_mutex;
+extern struct klp_patch *klp_transition_patch;
+
+extern void klp_init_transition(struct klp_patch *patch, int universe);
+extern void klp_start_transition(int universe);
+extern void klp_reverse_transition(void);
+extern void klp_try_complete_transition(void);
+extern void klp_complete_transition(void);
Double inclusion protection is missing and externs for functions are
redundant.
Otherwise it looks quite ok.
Miroslav
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On 02/09/2015, 06:31 PM, Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which
> will eventually enable us to patch those ~10% of security patches which
> change function prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
>
> Signed-off-by: Josh Poimboeuf
> ---
> include/linux/livepatch.h | 18 ++-
> include/linux/sched.h | 3 +
> kernel/fork.c | 2 +
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/core.c | 71 ++
> kernel/livepatch/patch.c | 34 -
> kernel/livepatch/patch.h | 1 +
> kernel/livepatch/transition.c | 300
> ++
> kernel/livepatch/transition.h | 16 +++
> kernel/sched/core.c | 2 +
> 10 files changed, 423 insertions(+), 26 deletions(-)
> create mode 100644 kernel/livepatch/transition.c
> create mode 100644 kernel/livepatch/transition.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 0e65b4d..b8c2f15 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -40,6 +40,7 @@
> * @old_size:size of the old function
> * @new_size:size of the new function
> * @patched: the func has been added to the klp_ops list
> + * @transition: the func is currently being applied or reverted
> */
> struct klp_func {
> /* external */
> @@ -60,6 +61,7 @@ struct klp_func {
> struct list_head stack_node;
> unsigned long old_size, new_size;
> int patched;
> + int transition;
> };
>
> /**
> @@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
> extern int klp_enable_patch(struct klp_patch *);
> extern int klp_disable_patch(struct klp_patch *);
>
> -#endif /* CONFIG_LIVEPATCH */
> +extern int klp_universe_goal;
> +
> +static inline void klp_update_task_universe(struct task_struct *t)
> +{
> + /* corresponding smp_wmb() is in klp_set_universe_goal() */
> + smp_rmb();
> +
> + t->klp_universe = klp_universe_goal;
> +}
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline void klp_update_task_universe(struct task_struct *t) {}
> +
> +#endif /* !CONFIG_LIVEPATCH */
>
> #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 8db31ef..a95e59a 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1701,6 +1701,9 @@ struct task_struct {
> #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> unsigned long task_state_change;
> #endif
> +#ifdef CONFIG_LIVEPATCH
> + int klp_universe;
> +#endif
> };
>
> /* Future-safe accessor for struct task_struct's cpus_allowed. */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4dc2dda..1dcbebe 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -74,6 +74,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
> clone_flags,
> total_forks++;
> spin_unlock(>sighand->siglock);
> syscall_tracepoint_update(p);
> + klp_update_task_universe(p);
> write_unlock_irq(_lock);
>
> proc_fork_connector(p);
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> index e136dad..2b8bdb1 100644
> --- a/kernel/livepatch/Makefile
> +++ b/kernel/livepatch/Makefile
> @@ -1,3 +1,3 @@
> obj-$(CONFIG_LIVEPATCH) += livepatch.o
>
> -livepatch-objs := core.o patch.o
> +livepatch-objs := core.o patch.o transition.o
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 85d4ef7..790dc10 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,14 +28,17 @@
> #include
>
> #include "patch.h"
> +#include "transition.h"
>
> /*
> - * The klp_mutex protects the global lists and state transitions of any
> - * structure reachable from them. References to any structure must be
> obtained
> - * under mutex protection (except in klp_ftrace_handler(), which
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On 02/09/2015, 06:31 PM, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Signed-off-by: Josh Poimboeuf jpoim...@redhat.com
---
include/linux/livepatch.h | 18 ++-
include/linux/sched.h | 3 +
kernel/fork.c | 2 +
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 71 ++
kernel/livepatch/patch.c | 34 -
kernel/livepatch/patch.h | 1 +
kernel/livepatch/transition.c | 300
++
kernel/livepatch/transition.h | 16 +++
kernel/sched/core.c | 2 +
10 files changed, 423 insertions(+), 26 deletions(-)
create mode 100644 kernel/livepatch/transition.c
create mode 100644 kernel/livepatch/transition.h
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0e65b4d..b8c2f15 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -40,6 +40,7 @@
* @old_size:size of the old function
* @new_size:size of the new function
* @patched: the func has been added to the klp_ops list
+ * @transition: the func is currently being applied or reverted
*/
struct klp_func {
/* external */
@@ -60,6 +61,7 @@ struct klp_func {
struct list_head stack_node;
unsigned long old_size, new_size;
int patched;
+ int transition;
};
/**
@@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
extern int klp_enable_patch(struct klp_patch *);
extern int klp_disable_patch(struct klp_patch *);
-#endif /* CONFIG_LIVEPATCH */
+extern int klp_universe_goal;
+
+static inline void klp_update_task_universe(struct task_struct *t)
+{
+ /* corresponding smp_wmb() is in klp_set_universe_goal() */
+ smp_rmb();
+
+ t-klp_universe = klp_universe_goal;
+}
+
+#else /* !CONFIG_LIVEPATCH */
+
+static inline void klp_update_task_universe(struct task_struct *t) {}
+
+#endif /* !CONFIG_LIVEPATCH */
#endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8db31ef..a95e59a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1701,6 +1701,9 @@ struct task_struct {
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
unsigned long task_state_change;
#endif
+#ifdef CONFIG_LIVEPATCH
+ int klp_universe;
+#endif
};
/* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4dc2dda..1dcbebe 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -74,6 +74,7 @@
#include linux/uprobes.h
#include linux/aio.h
#include linux/compiler.h
+#include linux/livepatch.h
#include asm/pgtable.h
#include asm/pgalloc.h
@@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
total_forks++;
spin_unlock(current-sighand-siglock);
syscall_tracepoint_update(p);
+ klp_update_task_universe(p);
write_unlock_irq(tasklist_lock);
proc_fork_connector(p);
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e136dad..2b8bdb1 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
obj-$(CONFIG_LIVEPATCH) += livepatch.o
-livepatch-objs := core.o patch.o
+livepatch-objs := core.o patch.o transition.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 85d4ef7..790dc10 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,14 +28,17 @@
#include linux/kallsyms.h
#include patch.h
+#include transition.h
/*
- * The klp_mutex protects the global lists and state transitions of any
- * structure reachable from them. References to any structure must be
obtained
- * under mutex protection (except in klp_ftrace_handler(), which
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 02:26:42PM +0100, Jiri Slaby wrote: > On 02/12/2015, 04:21 AM, Josh Poimboeuf wrote: > > Ingo, Peter, > > > > Would you have any objections to making task_rq_lock/unlock() non-static > > (or moving them to kernel/sched/sched.h) so they can be called by the > > livepatch code? > > > > To provide some background, I'm looking for a way to temporarily prevent > > a sleeping task from running while its stack is examined, to decide > > whether it can be safely switched to the new patching "universe". For > > more details see klp_transition_task() in the patch below. > > > > Using task_rq_lock() is the most straightforward way I could find to > > achieve that. > > Hi, I cannot speak whether it is the proper way or not. > > But if so, would it make sense to do the opposite: expose an API to walk > through the processes' stack and make the decision? Concretely, move > parts of klp_stacktrace_address_verify_func to sched.c or somewhere in > kernel/sched/ and leave task_rq_lock untouched. Yeah, it makes sense in theory. But I'm not sure how to do that in a way that prevents races when switching the task's universe. I think we need the rq locked for both the stack walk and the universe switch. In general, I agree it would be good to find a way to keep the rq locking functions in sched.c. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 03:08:38PM +0100, Jiri Kosina wrote: > On Thu, 12 Feb 2015, Peter Zijlstra wrote: > I personally am not a big fan of the task_rq_lock() public exposure > either. What might be generally useful though (not only for livepatching) > would be an API that would allow for "safe" stack dump (where "safe" means > that guarantee, that it wouldn't be interferred by process waking up in > the middle of dumping, would be provided). In general, I think a safe stack dump is needed. A lot of the stack dumping in the kernel seems dangerous. For example, it looks like doing a `cat /proc/pid/stack` while the process is writing the stack could easily go off into the weeds. But I don't see how it would help the livepatch case. What happens if the process starts running in the to-be-patched function after we call the "safe" dump_stack() but before switching it to the new universe? -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: > And what's wrong with using known good spots like the freezer? Quoting Tejun from the thread Jiri Slaby likely had on mind: "The fact that they may coincide often can be useful as a guideline or whatever but I'm completely against just mushing it together when it isn't correct. This kind of things quickly lead to ambiguous situations where people are not sure about the specific semantics or guarantees of the construct and implement weird voodoo code followed by voodoo fixes. We already had a full round of that with the kernel freezer itself, where people thought that the freezer magically makes PM work properly for a subsystem. Let's please not do that again." The whole thread begins here, in case everything hasn't been covered here yet: https://lkml.org/lkml/2014/7/2/328 Thanks again for looking into this, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Josh Poimboeuf wrote:
> > and you have a livepatch patching foo() and changing its return value
> > semantics. Then freezer doesn't really help.
>
> Don't we have the same issue with livepatch? For example:
>
> while (some_condition) {
> ret = foo();
> ...
> schedule(); <-- switch to the new universe while it's sleeps
> ...
> // use ret in an unexpected way
> }
Well if ret is changing semantics, the livepatch will also have to patch
the calling function (so that it handles new semantics properly), and
therefore by looking at the stacks you would see that fact and wouldn't
migrate the scheduled-out task to the new universe.
> I think it's not really a problem, just something the patch author needs
> to be aware of regardless.
Exactly; that's just up to the patch author to undersntad what the
semantical aspects of the patch he is writing are, and make appropriate
consistency model choice.
Thanks,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 02:16:07PM +0100, Jiri Kosina wrote:
> On Thu, 12 Feb 2015, Peter Zijlstra wrote:
>
> > > The short answer is: I need a way to ensure that a task isn't sleeping
> > > on any of the functions we're trying to patch. If it's not, then I can
> > > switch the task over to start using new versions of functions.
> > >
> > > Obviously, there are many more details than that. If you have specific
> > > questions I can try to answer them.
> >
> > How can one task run new and another task old functions? Once you patch
> > any indirect function pointer any task will see the new call.
>
> Patched functions are redirected through ftrace trampoline, and decision
> is being made there which function (old or new) to redirect to.
>
> Function calls through pointer always go first to the original function,
> and get redirected from its __fentry__ site.
>
> Once the system is in fully patched state, the overhead of the trampoline
> is reduced (no expensive decision-making to be made there, etc) to
> minimum.
>
> Sure, you will never be on a 100% of performance of the unpatched kernel
> for redirected functions, the indirect call through the trampoline will
> always be there (although ftrace with dynamic trampolines is really
> minimizing this penalty to few extra instructions, one extra call and one
> extra ret being the expensive ones).
>
> > And what's wrong with using known good spots like the freezer?
>
> It has undefined semantics when it comes to what you want to achieve here.
>
> Say for example you have a kernel thread which does something like
>
> while (some_condition) {
> ret = foo();
> ...
> try_to_freeze();
> ...
> }
>
> and you have a livepatch patching foo() and changing its return value
> semantics. Then freezer doesn't really help.
Don't we have the same issue with livepatch? For example:
while (some_condition) {
ret = foo();
...
schedule(); <-- switch to the new universe while it's sleeps
...
// use ret in an unexpected way
}
I think it's not really a problem, just something the patch author needs
to be aware of regardless. It should be part of the checklist. You
always need to be extremely careful when changing a function's return
semantics.
IIRC, when I looked at the freezer before, the biggest problems I found
were that it's too disruptive to the process, and that not all kthreads
are freezable. And I don't see anything inherently safer about it
compared to just stack checking.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On 02/12/2015, 02:35 PM, Peter Zijlstra wrote: > On Thu, Feb 12, 2015 at 02:16:28PM +0100, Jiri Slaby wrote: >>> And what's wrong with using known good spots like the freezer? >> >> This was already discussed too. Please STA. > > WTF is STA? You guys want something from me; I don't have time, not > inclination to go hunt down whatever dark corner of the interweb > contains your ramblings. You definitely do not need STA, if you don't want to know the details. I think repeating the whole thread would not be productive for all of us. The short answer you can read from the above is: it is not possible. On the top of that, Jiri provided you with a simple example to answer why. > If you can't be arsed to explain things, I certainly cannot be arsed to > consider your request. Please see above. > So you now have my full NAK on touching the scheduler, have at it, go > deal with someone else. Ok, we already got your expressed attitude towards live patching. This is not a kind of input we were hoping for though. Could you comment on the technical aspects and the proposed solutions instead? thanks, -- js suse labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > > And what's wrong with using known good spots like the freezer? > > > > This was already discussed too. Please STA. > > WTF is STA? You guys want something from me; I don't have time, not > inclination to go hunt down whatever dark corner of the interweb > contains your ramblings. > > If you can't be arsed to explain things, I certainly cannot be arsed to > consider your request. I believe I have provided answer to the freezer question in my previous mail, so please let's continue the discussion there if needed. > So you now have my full NAK on touching the scheduler, have at it, go > deal with someone else. I personally am not a big fan of the task_rq_lock() public exposure either. What might be generally useful though (not only for livepatching) would be an API that would allow for "safe" stack dump (where "safe" means that guarantee, that it wouldn't be interferred by process waking up in the middle of dumping, would be provided). Does that sound like even remotely acceptable idea to you? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 02:16:28PM +0100, Jiri Slaby wrote: > > And what's wrong with using known good spots like the freezer? > > This was already discussed too. Please STA. WTF is STA? You guys want something from me; I don't have time, not inclination to go hunt down whatever dark corner of the interweb contains your ramblings. If you can't be arsed to explain things, I certainly cannot be arsed to consider your request. So you now have my full NAK on touching the scheduler, have at it, go deal with someone else. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On 02/12/2015, 04:21 AM, Josh Poimboeuf wrote: > Ingo, Peter, > > Would you have any objections to making task_rq_lock/unlock() non-static > (or moving them to kernel/sched/sched.h) so they can be called by the > livepatch code? > > To provide some background, I'm looking for a way to temporarily prevent > a sleeping task from running while its stack is examined, to decide > whether it can be safely switched to the new patching "universe". For > more details see klp_transition_task() in the patch below. > > Using task_rq_lock() is the most straightforward way I could find to > achieve that. Hi, I cannot speak whether it is the proper way or not. But if so, would it make sense to do the opposite: expose an API to walk through the processes' stack and make the decision? Concretely, move parts of klp_stacktrace_address_verify_func to sched.c or somewhere in kernel/sched/ and leave task_rq_lock untouched. regards, -- js suse labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
Hi, On 02/12/2015, 02:08 PM, Peter Zijlstra wrote: > How can one task run new and another task old functions? because this is how it is designed to work in one of the consistency models. > Once you patch > any indirect function pointer any task will see the new call. It does not patch any pointers. Callees' fentrys are "patched" using ftrace. > And what's wrong with using known good spots like the freezer? This was already discussed too. Please STA. thanks, -- js suse labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote:
> > The short answer is: I need a way to ensure that a task isn't sleeping
> > on any of the functions we're trying to patch. If it's not, then I can
> > switch the task over to start using new versions of functions.
> >
> > Obviously, there are many more details than that. If you have specific
> > questions I can try to answer them.
>
> How can one task run new and another task old functions? Once you patch
> any indirect function pointer any task will see the new call.
Patched functions are redirected through ftrace trampoline, and decision
is being made there which function (old or new) to redirect to.
Function calls through pointer always go first to the original function,
and get redirected from its __fentry__ site.
Once the system is in fully patched state, the overhead of the trampoline
is reduced (no expensive decision-making to be made there, etc) to
minimum.
Sure, you will never be on a 100% of performance of the unpatched kernel
for redirected functions, the indirect call through the trampoline will
always be there (although ftrace with dynamic trampolines is really
minimizing this penalty to few extra instructions, one extra call and one
extra ret being the expensive ones).
> And what's wrong with using known good spots like the freezer?
It has undefined semantics when it comes to what you want to achieve here.
Say for example you have a kernel thread which does something like
while (some_condition) {
ret = foo();
...
try_to_freeze();
...
}
and you have a livepatch patching foo() and changing its return value
semantics. Then freezer doesn't really help.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 06:51:49AM -0600, Josh Poimboeuf wrote: > > > To provide some background, I'm looking for a way to temporarily prevent > > > a sleeping task from running while its stack is examined, to decide > > > whether it can be safely switched to the new patching "universe". For > > > more details see klp_transition_task() in the patch below. > > > > > > Using task_rq_lock() is the most straightforward way I could find to > > > achieve that. > > > > Its not at all clear how all this would work to me. And I'm not > > motivated enough to go try and reverse engineer your patch; > > The short answer is: I need a way to ensure that a task isn't sleeping > on any of the functions we're trying to patch. If it's not, then I can > switch the task over to start using new versions of functions. > > Obviously, there are many more details than that. If you have specific > questions I can try to answer them. How can one task run new and another task old functions? Once you patch any indirect function pointer any task will see the new call. And what's wrong with using known good spots like the freezer? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 01:42:01PM +0100, Jiri Kosina wrote: > On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > > > Well, the fact indisputable fact is that there is a demand for this. It's > > > not about one machine, it's about scheduling dowtimes of datacentres. > > > > The changelog says: > > > > > ... A patch can remain in the > > > transition state indefinitely, if any of the tasks are stuck in the > > > previous universe. > > > > Therefore there is no scheduling anything. Without timeliness guarantees > > you can't make a schedule. > > > > Might as well just reboot, at least that's fairly well guaranteed to > > happen. > > All running (reasonably alive) tasks will be running patched code though. > > You can't just claim complete victory (and get ready for accepting another > patch, etc) if there is a long-time sleeper that hasn't been converted > yet. Agreed. And also we have several strategies for reducing the time needed to get all tasks to a patched state (see patch 9 of this series for more details). The goal is to not leave systems in limbo for more than a few seconds. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 12:56:28PM +0100, Peter Zijlstra wrote: > On Wed, Feb 11, 2015 at 09:21:21PM -0600, Josh Poimboeuf wrote: > > Ingo, Peter, > > > > Would you have any objections to making task_rq_lock/unlock() non-static > > (or moving them to kernel/sched/sched.h) so they can be called by the > > livepatch code? > > Basically yes. I really don't want to expose that. And > kernel/sched/sched.h is very much not intended for use outside of > kernel/sched/ so even that is a no go. > > > To provide some background, I'm looking for a way to temporarily prevent > > a sleeping task from running while its stack is examined, to decide > > whether it can be safely switched to the new patching "universe". For > > more details see klp_transition_task() in the patch below. > > > > Using task_rq_lock() is the most straightforward way I could find to > > achieve that. > > Its not at all clear how all this would work to me. And I'm not > motivated enough to go try and reverse engineer your patch; The short answer is: I need a way to ensure that a task isn't sleeping on any of the functions we're trying to patch. If it's not, then I can switch the task over to start using new versions of functions. Obviously, there are many more details than that. If you have specific questions I can try to answer them. > IMO livepatching is utter fail. > > If your infrastructure relies on the uptime of a single machine you've > lost already. It's not always about uptime. IMO it's usually more about decoupling your reboot schedule from your distro's kernel release schedule. Most users want to plan in advance when they're going to reboot, rather than being at the mercy of when CVEs and kernel fixes are released. Rebooting is costly and risky, even (or often especially) for large systems for which you have to stagger the reboots. You want to do it at a time when you're ready for something bad to happen, without having to also worry about security in the mean time while you're waiting for your reboot window. > FWIW, the barriers in klp_update_task_universe() and > klp_set_universe_goal() look like complete crack, and their comments are > seriously deficient. Ok, I'll try to improve the comments for the barriers. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > Well, the fact indisputable fact is that there is a demand for this. It's > > not about one machine, it's about scheduling dowtimes of datacentres. > > The changelog says: > > > ... A patch can remain in the > > transition state indefinitely, if any of the tasks are stuck in the > > previous universe. > > Therefore there is no scheduling anything. Without timeliness guarantees > you can't make a schedule. > > Might as well just reboot, at least that's fairly well guaranteed to > happen. All running (reasonably alive) tasks will be running patched code though. You can't just claim complete victory (and get ready for accepting another patch, etc) if there is a long-time sleeper that hasn't been converted yet. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > > FWIW, the barriers in klp_update_task_universe() and > > > klp_set_universe_goal() look like complete crack, and their comments are > > > seriously deficient. > > > > These particular barriers seem correct to me; you basically need to make > > sure that whenever a thread with TIF_KLP_NEED_UPDATE goes through > > do_notify_resume(), it sees proper universe number to be converted to. > > I'm not seeing how they're going to help with that. > > The comment should describe the data race and how the barriers are > making it not happen. > > putting wmb after a store and rmb before a read doesn't avoid the reader > seeing the old value in any universe I know of. This is about dependency between klp_universe_goal and TIF_KLP_NEED_UPDATE in threadinfo flags. What is confusing here is that threadinfo flags are not set in klp_set_universe_goal() directly, but in the caller (klp_start_transition()). I fully agree with you that this deserves better comment though. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 01:25:14PM +0100, Jiri Kosina wrote: > On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > > Its not at all clear how all this would work to me. And I'm not > > motivated enough to go try and reverse engineer your patch; IMO > > livepatching is utter fail. > > > > If your infrastructure relies on the uptime of a single machine you've > > lost already. > > Well, the fact indisputable fact is that there is a demand for this. It's > not about one machine, it's about scheduling dowtimes of datacentres. The changelog says: > ... A patch can remain in the > transition state indefinitely, if any of the tasks are stuck in the > previous universe. Therefore there is no scheduling anything. Without timeliness guarantees you can't make a schedule. Might as well just reboot, at least that's fairly well guaranteed to happen. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 01:25:14PM +0100, Jiri Kosina wrote: > On Thu, 12 Feb 2015, Peter Zijlstra wrote: > > FWIW, the barriers in klp_update_task_universe() and > > klp_set_universe_goal() look like complete crack, and their comments are > > seriously deficient. > > These particular barriers seem correct to me; you basically need to make > sure that whenever a thread with TIF_KLP_NEED_UPDATE goes through > do_notify_resume(), it sees proper universe number to be converted to. I'm not seeing how they're going to help with that. The comment should describe the data race and how the barriers are making it not happen. putting wmb after a store and rmb before a read doesn't avoid the reader seeing the old value in any universe I know of. Barriers are about order, you need two consecutive stores for a wmb to make sense, and two consecutive reads for an rmb, and if they're paired the stores and reads need to be to the same addresses. Without that they're pointless. The comment doesn't describe which two variables are ordered how. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: > Its not at all clear how all this would work to me. And I'm not > motivated enough to go try and reverse engineer your patch; IMO > livepatching is utter fail. > > If your infrastructure relies on the uptime of a single machine you've > lost already. Well, the fact indisputable fact is that there is a demand for this. It's not about one machine, it's about scheduling dowtimes of datacentres. But if this needs to be discussed, it should be done outside of this thread I guess. > FWIW, the barriers in klp_update_task_universe() and > klp_set_universe_goal() look like complete crack, and their comments are > seriously deficient. These particular barriers seem correct to me; you basically need to make sure that whenever a thread with TIF_KLP_NEED_UPDATE goes through do_notify_resume(), it sees proper universe number to be converted to. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 11, 2015 at 09:21:21PM -0600, Josh Poimboeuf wrote: > Ingo, Peter, > > Would you have any objections to making task_rq_lock/unlock() non-static > (or moving them to kernel/sched/sched.h) so they can be called by the > livepatch code? Basically yes. I really don't want to expose that. And kernel/sched/sched.h is very much not intended for use outside of kernel/sched/ so even that is a no go. > To provide some background, I'm looking for a way to temporarily prevent > a sleeping task from running while its stack is examined, to decide > whether it can be safely switched to the new patching "universe". For > more details see klp_transition_task() in the patch below. > > Using task_rq_lock() is the most straightforward way I could find to > achieve that. Its not at all clear how all this would work to me. And I'm not motivated enough to go try and reverse engineer your patch; IMO livepatching is utter fail. If your infrastructure relies on the uptime of a single machine you've lost already. FWIW, the barriers in klp_update_task_universe() and klp_set_universe_goal() look like complete crack, and their comments are seriously deficient. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, 11 Feb 2015, Josh Poimboeuf wrote:
> On Wed, Feb 11, 2015 at 11:21:51AM +0100, Miroslav Benes wrote:
> >
> > On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> >
> > [...]
> >
> > > @@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long
> > > ip,
> > > ops = container_of(fops, struct klp_ops, fops);
> > >
> > > rcu_read_lock();
> > > +
> > > func = list_first_or_null_rcu(>func_stack, struct klp_func,
> > > stack_node);
> > > - rcu_read_unlock();
> > >
> > > if (WARN_ON_ONCE(!func))
> > > - return;
> > > + goto unlock;
> > > +
> > > + if (unlikely(func->transition)) {
> > > + /* corresponding smp_wmb() is in klp_init_transition() */
> > > + smp_rmb();
> > > +
> > > + if (current->klp_universe == KLP_UNIVERSE_OLD) {
> > > + /*
> > > + * Use the previously patched version of the function.
> > > + * If no previous patches exist, use the original
> > > + * function.
> > > + */
> > > + func = list_entry_rcu(func->stack_node.next,
> > > + struct klp_func, stack_node);
> > > +
> > > + if (>stack_node == >func_stack)
> > > + goto unlock;
> > > + }
> > > + }
> > >
> > > klp_arch_set_pc(regs, (unsigned long)func->new_func);
> > > +unlock:
> > > + rcu_read_unlock();
> > > }
> >
> > I decided to understand the code more before answering the email about the
> > race and found another problem. I think.
> >
> > Imagine we patched some function foo() with foo_1() from patch_1 and now
> > we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch
> > calls klp_init_transition which sets klp_universe for all processes to
> > KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1).
> > Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo().
> > BUT what if somebody calls foo() right between klp_init_transition and
> > the loop in __klp_enable_patch? The ftrace handler first returns the
> > first entry in the list which is foo_1() (foo_2() is still not present),
> > then it checks for func->transition. It is 1.
>
> No, actually foo_1()'s func->transition will be 0. Only foo_2()'s
> func->transition will be 1.
Ah, you're right in both cases. Sorry for the noise.
Miroslav
>
> > It checks for
> > current->klp_universe which is KLP_UNIVERSE_OLD and so the next entry is
> > retrieved. There is no such and therefore foo() is called. This is
> > obviously wrong because foo_1() was expected.
> >
> > Everything would work fine if one would call foo() before
> > klp_start_transition and after the loop in __klp_enable_patch. The
> > solution might be to move the setting of func->transition to
> > klp_start_transition, but this could break something different. I don't
> > know yet.
> >
> > Am I wrong?
> >
> > Miroslav
>
> --
> Josh
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 11, 2015 at 09:21:21PM -0600, Josh Poimboeuf wrote: Ingo, Peter, Would you have any objections to making task_rq_lock/unlock() non-static (or moving them to kernel/sched/sched.h) so they can be called by the livepatch code? Basically yes. I really don't want to expose that. And kernel/sched/sched.h is very much not intended for use outside of kernel/sched/ so even that is a no go. To provide some background, I'm looking for a way to temporarily prevent a sleeping task from running while its stack is examined, to decide whether it can be safely switched to the new patching universe. For more details see klp_transition_task() in the patch below. Using task_rq_lock() is the most straightforward way I could find to achieve that. Its not at all clear how all this would work to me. And I'm not motivated enough to go try and reverse engineer your patch; IMO livepatching is utter fail. If your infrastructure relies on the uptime of a single machine you've lost already. FWIW, the barriers in klp_update_task_universe() and klp_set_universe_goal() look like complete crack, and their comments are seriously deficient. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, 11 Feb 2015, Josh Poimboeuf wrote:
On Wed, Feb 11, 2015 at 11:21:51AM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
[...]
@@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long
ip,
ops = container_of(fops, struct klp_ops, fops);
rcu_read_lock();
+
func = list_first_or_null_rcu(ops-func_stack, struct klp_func,
stack_node);
- rcu_read_unlock();
if (WARN_ON_ONCE(!func))
- return;
+ goto unlock;
+
+ if (unlikely(func-transition)) {
+ /* corresponding smp_wmb() is in klp_init_transition() */
+ smp_rmb();
+
+ if (current-klp_universe == KLP_UNIVERSE_OLD) {
+ /*
+ * Use the previously patched version of the function.
+ * If no previous patches exist, use the original
+ * function.
+ */
+ func = list_entry_rcu(func-stack_node.next,
+ struct klp_func, stack_node);
+
+ if (func-stack_node == ops-func_stack)
+ goto unlock;
+ }
+ }
klp_arch_set_pc(regs, (unsigned long)func-new_func);
+unlock:
+ rcu_read_unlock();
}
I decided to understand the code more before answering the email about the
race and found another problem. I think.
Imagine we patched some function foo() with foo_1() from patch_1 and now
we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch
calls klp_init_transition which sets klp_universe for all processes to
KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1).
Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo().
BUT what if somebody calls foo() right between klp_init_transition and
the loop in __klp_enable_patch? The ftrace handler first returns the
first entry in the list which is foo_1() (foo_2() is still not present),
then it checks for func-transition. It is 1.
No, actually foo_1()'s func-transition will be 0. Only foo_2()'s
func-transition will be 1.
Ah, you're right in both cases. Sorry for the noise.
Miroslav
It checks for
current-klp_universe which is KLP_UNIVERSE_OLD and so the next entry is
retrieved. There is no such and therefore foo() is called. This is
obviously wrong because foo_1() was expected.
Everything would work fine if one would call foo() before
klp_start_transition and after the loop in __klp_enable_patch. The
solution might be to move the setting of func-transition to
klp_start_transition, but this could break something different. I don't
know yet.
Am I wrong?
Miroslav
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 06:51:49AM -0600, Josh Poimboeuf wrote: To provide some background, I'm looking for a way to temporarily prevent a sleeping task from running while its stack is examined, to decide whether it can be safely switched to the new patching universe. For more details see klp_transition_task() in the patch below. Using task_rq_lock() is the most straightforward way I could find to achieve that. Its not at all clear how all this would work to me. And I'm not motivated enough to go try and reverse engineer your patch; The short answer is: I need a way to ensure that a task isn't sleeping on any of the functions we're trying to patch. If it's not, then I can switch the task over to start using new versions of functions. Obviously, there are many more details than that. If you have specific questions I can try to answer them. How can one task run new and another task old functions? Once you patch any indirect function pointer any task will see the new call. And what's wrong with using known good spots like the freezer? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 02:16:28PM +0100, Jiri Slaby wrote: And what's wrong with using known good spots like the freezer? This was already discussed too. Please STA. WTF is STA? You guys want something from me; I don't have time, not inclination to go hunt down whatever dark corner of the interweb contains your ramblings. If you can't be arsed to explain things, I certainly cannot be arsed to consider your request. So you now have my full NAK on touching the scheduler, have at it, go deal with someone else. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 02:16:07PM +0100, Jiri Kosina wrote:
On Thu, 12 Feb 2015, Peter Zijlstra wrote:
The short answer is: I need a way to ensure that a task isn't sleeping
on any of the functions we're trying to patch. If it's not, then I can
switch the task over to start using new versions of functions.
Obviously, there are many more details than that. If you have specific
questions I can try to answer them.
How can one task run new and another task old functions? Once you patch
any indirect function pointer any task will see the new call.
Patched functions are redirected through ftrace trampoline, and decision
is being made there which function (old or new) to redirect to.
Function calls through pointer always go first to the original function,
and get redirected from its __fentry__ site.
Once the system is in fully patched state, the overhead of the trampoline
is reduced (no expensive decision-making to be made there, etc) to
minimum.
Sure, you will never be on a 100% of performance of the unpatched kernel
for redirected functions, the indirect call through the trampoline will
always be there (although ftrace with dynamic trampolines is really
minimizing this penalty to few extra instructions, one extra call and one
extra ret being the expensive ones).
And what's wrong with using known good spots like the freezer?
It has undefined semantics when it comes to what you want to achieve here.
Say for example you have a kernel thread which does something like
while (some_condition) {
ret = foo();
...
try_to_freeze();
...
}
and you have a livepatch patching foo() and changing its return value
semantics. Then freezer doesn't really help.
Don't we have the same issue with livepatch? For example:
while (some_condition) {
ret = foo();
...
schedule(); -- switch to the new universe while it's sleeps
...
// use ret in an unexpected way
}
I think it's not really a problem, just something the patch author needs
to be aware of regardless. It should be part of the checklist. You
always need to be extremely careful when changing a function's return
semantics.
IIRC, when I looked at the freezer before, the biggest problems I found
were that it's too disruptive to the process, and that not all kthreads
are freezable. And I don't see anything inherently safer about it
compared to just stack checking.
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Josh Poimboeuf wrote:
and you have a livepatch patching foo() and changing its return value
semantics. Then freezer doesn't really help.
Don't we have the same issue with livepatch? For example:
while (some_condition) {
ret = foo();
...
schedule(); -- switch to the new universe while it's sleeps
...
// use ret in an unexpected way
}
Well if ret is changing semantics, the livepatch will also have to patch
the calling function (so that it handles new semantics properly), and
therefore by looking at the stacks you would see that fact and wouldn't
migrate the scheduled-out task to the new universe.
I think it's not really a problem, just something the patch author needs
to be aware of regardless.
Exactly; that's just up to the patch author to undersntad what the
semantical aspects of the patch he is writing are, and make appropriate
consistency model choice.
Thanks,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: And what's wrong with using known good spots like the freezer? This was already discussed too. Please STA. WTF is STA? You guys want something from me; I don't have time, not inclination to go hunt down whatever dark corner of the interweb contains your ramblings. If you can't be arsed to explain things, I certainly cannot be arsed to consider your request. I believe I have provided answer to the freezer question in my previous mail, so please let's continue the discussion there if needed. So you now have my full NAK on touching the scheduler, have at it, go deal with someone else. I personally am not a big fan of the task_rq_lock() public exposure either. What might be generally useful though (not only for livepatching) would be an API that would allow for safe stack dump (where safe means that guarantee, that it wouldn't be interferred by process waking up in the middle of dumping, would be provided). Does that sound like even remotely acceptable idea to you? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On 02/12/2015, 02:35 PM, Peter Zijlstra wrote: On Thu, Feb 12, 2015 at 02:16:28PM +0100, Jiri Slaby wrote: And what's wrong with using known good spots like the freezer? This was already discussed too. Please STA. WTF is STA? You guys want something from me; I don't have time, not inclination to go hunt down whatever dark corner of the interweb contains your ramblings. You definitely do not need STA, if you don't want to know the details. I think repeating the whole thread would not be productive for all of us. The short answer you can read from the above is: it is not possible. On the top of that, Jiri provided you with a simple example to answer why. If you can't be arsed to explain things, I certainly cannot be arsed to consider your request. Please see above. So you now have my full NAK on touching the scheduler, have at it, go deal with someone else. Ok, we already got your expressed attitude towards live patching. This is not a kind of input we were hoping for though. Could you comment on the technical aspects and the proposed solutions instead? thanks, -- js suse labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: And what's wrong with using known good spots like the freezer? Quoting Tejun from the thread Jiri Slaby likely had on mind: The fact that they may coincide often can be useful as a guideline or whatever but I'm completely against just mushing it together when it isn't correct. This kind of things quickly lead to ambiguous situations where people are not sure about the specific semantics or guarantees of the construct and implement weird voodoo code followed by voodoo fixes. We already had a full round of that with the kernel freezer itself, where people thought that the freezer magically makes PM work properly for a subsystem. Let's please not do that again. The whole thread begins here, in case everything hasn't been covered here yet: https://lkml.org/lkml/2014/7/2/328 Thanks again for looking into this, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 12:56:28PM +0100, Peter Zijlstra wrote: On Wed, Feb 11, 2015 at 09:21:21PM -0600, Josh Poimboeuf wrote: Ingo, Peter, Would you have any objections to making task_rq_lock/unlock() non-static (or moving them to kernel/sched/sched.h) so they can be called by the livepatch code? Basically yes. I really don't want to expose that. And kernel/sched/sched.h is very much not intended for use outside of kernel/sched/ so even that is a no go. To provide some background, I'm looking for a way to temporarily prevent a sleeping task from running while its stack is examined, to decide whether it can be safely switched to the new patching universe. For more details see klp_transition_task() in the patch below. Using task_rq_lock() is the most straightforward way I could find to achieve that. Its not at all clear how all this would work to me. And I'm not motivated enough to go try and reverse engineer your patch; The short answer is: I need a way to ensure that a task isn't sleeping on any of the functions we're trying to patch. If it's not, then I can switch the task over to start using new versions of functions. Obviously, there are many more details than that. If you have specific questions I can try to answer them. IMO livepatching is utter fail. If your infrastructure relies on the uptime of a single machine you've lost already. It's not always about uptime. IMO it's usually more about decoupling your reboot schedule from your distro's kernel release schedule. Most users want to plan in advance when they're going to reboot, rather than being at the mercy of when CVEs and kernel fixes are released. Rebooting is costly and risky, even (or often especially) for large systems for which you have to stagger the reboots. You want to do it at a time when you're ready for something bad to happen, without having to also worry about security in the mean time while you're waiting for your reboot window. FWIW, the barriers in klp_update_task_universe() and klp_set_universe_goal() look like complete crack, and their comments are seriously deficient. Ok, I'll try to improve the comments for the barriers. -- Josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 01:42:01PM +0100, Jiri Kosina wrote: On Thu, 12 Feb 2015, Peter Zijlstra wrote: Well, the fact indisputable fact is that there is a demand for this. It's not about one machine, it's about scheduling dowtimes of datacentres. The changelog says: ... A patch can remain in the transition state indefinitely, if any of the tasks are stuck in the previous universe. Therefore there is no scheduling anything. Without timeliness guarantees you can't make a schedule. Might as well just reboot, at least that's fairly well guaranteed to happen. All running (reasonably alive) tasks will be running patched code though. You can't just claim complete victory (and get ready for accepting another patch, etc) if there is a long-time sleeper that hasn't been converted yet. Agreed. And also we have several strategies for reducing the time needed to get all tasks to a patched state (see patch 9 of this series for more details). The goal is to not leave systems in limbo for more than a few seconds. -- Josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
Hi, On 02/12/2015, 02:08 PM, Peter Zijlstra wrote: How can one task run new and another task old functions? because this is how it is designed to work in one of the consistency models. Once you patch any indirect function pointer any task will see the new call. It does not patch any pointers. Callees' fentrys are patched using ftrace. And what's wrong with using known good spots like the freezer? This was already discussed too. Please STA. thanks, -- js suse labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote:
The short answer is: I need a way to ensure that a task isn't sleeping
on any of the functions we're trying to patch. If it's not, then I can
switch the task over to start using new versions of functions.
Obviously, there are many more details than that. If you have specific
questions I can try to answer them.
How can one task run new and another task old functions? Once you patch
any indirect function pointer any task will see the new call.
Patched functions are redirected through ftrace trampoline, and decision
is being made there which function (old or new) to redirect to.
Function calls through pointer always go first to the original function,
and get redirected from its __fentry__ site.
Once the system is in fully patched state, the overhead of the trampoline
is reduced (no expensive decision-making to be made there, etc) to
minimum.
Sure, you will never be on a 100% of performance of the unpatched kernel
for redirected functions, the indirect call through the trampoline will
always be there (although ftrace with dynamic trampolines is really
minimizing this penalty to few extra instructions, one extra call and one
extra ret being the expensive ones).
And what's wrong with using known good spots like the freezer?
It has undefined semantics when it comes to what you want to achieve here.
Say for example you have a kernel thread which does something like
while (some_condition) {
ret = foo();
...
try_to_freeze();
...
}
and you have a livepatch patching foo() and changing its return value
semantics. Then freezer doesn't really help.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On 02/12/2015, 04:21 AM, Josh Poimboeuf wrote: Ingo, Peter, Would you have any objections to making task_rq_lock/unlock() non-static (or moving them to kernel/sched/sched.h) so they can be called by the livepatch code? To provide some background, I'm looking for a way to temporarily prevent a sleeping task from running while its stack is examined, to decide whether it can be safely switched to the new patching universe. For more details see klp_transition_task() in the patch below. Using task_rq_lock() is the most straightforward way I could find to achieve that. Hi, I cannot speak whether it is the proper way or not. But if so, would it make sense to do the opposite: expose an API to walk through the processes' stack and make the decision? Concretely, move parts of klp_stacktrace_address_verify_func to sched.c or somewhere in kernel/sched/ and leave task_rq_lock untouched. regards, -- js suse labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: Well, the fact indisputable fact is that there is a demand for this. It's not about one machine, it's about scheduling dowtimes of datacentres. The changelog says: ... A patch can remain in the transition state indefinitely, if any of the tasks are stuck in the previous universe. Therefore there is no scheduling anything. Without timeliness guarantees you can't make a schedule. Might as well just reboot, at least that's fairly well guaranteed to happen. All running (reasonably alive) tasks will be running patched code though. You can't just claim complete victory (and get ready for accepting another patch, etc) if there is a long-time sleeper that hasn't been converted yet. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 01:25:14PM +0100, Jiri Kosina wrote: On Thu, 12 Feb 2015, Peter Zijlstra wrote: FWIW, the barriers in klp_update_task_universe() and klp_set_universe_goal() look like complete crack, and their comments are seriously deficient. These particular barriers seem correct to me; you basically need to make sure that whenever a thread with TIF_KLP_NEED_UPDATE goes through do_notify_resume(), it sees proper universe number to be converted to. I'm not seeing how they're going to help with that. The comment should describe the data race and how the barriers are making it not happen. putting wmb after a store and rmb before a read doesn't avoid the reader seeing the old value in any universe I know of. Barriers are about order, you need two consecutive stores for a wmb to make sense, and two consecutive reads for an rmb, and if they're paired the stores and reads need to be to the same addresses. Without that they're pointless. The comment doesn't describe which two variables are ordered how. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: Its not at all clear how all this would work to me. And I'm not motivated enough to go try and reverse engineer your patch; IMO livepatching is utter fail. If your infrastructure relies on the uptime of a single machine you've lost already. Well, the fact indisputable fact is that there is a demand for this. It's not about one machine, it's about scheduling dowtimes of datacentres. But if this needs to be discussed, it should be done outside of this thread I guess. FWIW, the barriers in klp_update_task_universe() and klp_set_universe_goal() look like complete crack, and their comments are seriously deficient. These particular barriers seem correct to me; you basically need to make sure that whenever a thread with TIF_KLP_NEED_UPDATE goes through do_notify_resume(), it sees proper universe number to be converted to. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, 12 Feb 2015, Peter Zijlstra wrote: FWIW, the barriers in klp_update_task_universe() and klp_set_universe_goal() look like complete crack, and their comments are seriously deficient. These particular barriers seem correct to me; you basically need to make sure that whenever a thread with TIF_KLP_NEED_UPDATE goes through do_notify_resume(), it sees proper universe number to be converted to. I'm not seeing how they're going to help with that. The comment should describe the data race and how the barriers are making it not happen. putting wmb after a store and rmb before a read doesn't avoid the reader seeing the old value in any universe I know of. This is about dependency between klp_universe_goal and TIF_KLP_NEED_UPDATE in threadinfo flags. What is confusing here is that threadinfo flags are not set in klp_set_universe_goal() directly, but in the caller (klp_start_transition()). I fully agree with you that this deserves better comment though. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 01:25:14PM +0100, Jiri Kosina wrote: On Thu, 12 Feb 2015, Peter Zijlstra wrote: Its not at all clear how all this would work to me. And I'm not motivated enough to go try and reverse engineer your patch; IMO livepatching is utter fail. If your infrastructure relies on the uptime of a single machine you've lost already. Well, the fact indisputable fact is that there is a demand for this. It's not about one machine, it's about scheduling dowtimes of datacentres. The changelog says: ... A patch can remain in the transition state indefinitely, if any of the tasks are stuck in the previous universe. Therefore there is no scheduling anything. Without timeliness guarantees you can't make a schedule. Might as well just reboot, at least that's fairly well guaranteed to happen. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 03:08:38PM +0100, Jiri Kosina wrote: On Thu, 12 Feb 2015, Peter Zijlstra wrote: I personally am not a big fan of the task_rq_lock() public exposure either. What might be generally useful though (not only for livepatching) would be an API that would allow for safe stack dump (where safe means that guarantee, that it wouldn't be interferred by process waking up in the middle of dumping, would be provided). In general, I think a safe stack dump is needed. A lot of the stack dumping in the kernel seems dangerous. For example, it looks like doing a `cat /proc/pid/stack` while the process is writing the stack could easily go off into the weeds. But I don't see how it would help the livepatch case. What happens if the process starts running in the to-be-patched function after we call the safe dump_stack() but before switching it to the new universe? -- Josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Thu, Feb 12, 2015 at 02:26:42PM +0100, Jiri Slaby wrote: On 02/12/2015, 04:21 AM, Josh Poimboeuf wrote: Ingo, Peter, Would you have any objections to making task_rq_lock/unlock() non-static (or moving them to kernel/sched/sched.h) so they can be called by the livepatch code? To provide some background, I'm looking for a way to temporarily prevent a sleeping task from running while its stack is examined, to decide whether it can be safely switched to the new patching universe. For more details see klp_transition_task() in the patch below. Using task_rq_lock() is the most straightforward way I could find to achieve that. Hi, I cannot speak whether it is the proper way or not. But if so, would it make sense to do the opposite: expose an API to walk through the processes' stack and make the decision? Concretely, move parts of klp_stacktrace_address_verify_func to sched.c or somewhere in kernel/sched/ and leave task_rq_lock untouched. Yeah, it makes sense in theory. But I'm not sure how to do that in a way that prevents races when switching the task's universe. I think we need the rq locked for both the stack walk and the universe switch. In general, I agree it would be good to find a way to keep the rq locking functions in sched.c. -- Josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
Ingo, Peter,
Would you have any objections to making task_rq_lock/unlock() non-static
(or moving them to kernel/sched/sched.h) so they can be called by the
livepatch code?
To provide some background, I'm looking for a way to temporarily prevent
a sleeping task from running while its stack is examined, to decide
whether it can be safely switched to the new patching "universe". For
more details see klp_transition_task() in the patch below.
Using task_rq_lock() is the most straightforward way I could find to
achieve that.
On Mon, Feb 09, 2015 at 11:31:18AM -0600, Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which
> will eventually enable us to patch those ~10% of security patches which
> change function prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
>
> Signed-off-by: Josh Poimboeuf
> ---
> include/linux/livepatch.h | 18 ++-
> include/linux/sched.h | 3 +
> kernel/fork.c | 2 +
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/core.c | 71 ++
> kernel/livepatch/patch.c | 34 -
> kernel/livepatch/patch.h | 1 +
> kernel/livepatch/transition.c | 300
> ++
> kernel/livepatch/transition.h | 16 +++
> kernel/sched/core.c | 2 +
> 10 files changed, 423 insertions(+), 26 deletions(-)
> create mode 100644 kernel/livepatch/transition.c
> create mode 100644 kernel/livepatch/transition.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 0e65b4d..b8c2f15 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -40,6 +40,7 @@
> * @old_size:size of the old function
> * @new_size:size of the new function
> * @patched: the func has been added to the klp_ops list
> + * @transition: the func is currently being applied or reverted
> */
> struct klp_func {
> /* external */
> @@ -60,6 +61,7 @@ struct klp_func {
> struct list_head stack_node;
> unsigned long old_size, new_size;
> int patched;
> + int transition;
> };
>
> /**
> @@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
> extern int klp_enable_patch(struct klp_patch *);
> extern int klp_disable_patch(struct klp_patch *);
>
> -#endif /* CONFIG_LIVEPATCH */
> +extern int klp_universe_goal;
> +
> +static inline void klp_update_task_universe(struct task_struct *t)
> +{
> + /* corresponding smp_wmb() is in klp_set_universe_goal() */
> + smp_rmb();
> +
> + t->klp_universe = klp_universe_goal;
> +}
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline void klp_update_task_universe(struct task_struct *t) {}
> +
> +#endif /* !CONFIG_LIVEPATCH */
>
> #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 8db31ef..a95e59a 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1701,6 +1701,9 @@ struct task_struct {
> #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> unsigned long task_state_change;
> #endif
> +#ifdef CONFIG_LIVEPATCH
> + int klp_universe;
> +#endif
> };
>
> /* Future-safe accessor for struct task_struct's cpus_allowed. */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4dc2dda..1dcbebe 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -74,6 +74,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
> clone_flags,
> total_forks++;
> spin_unlock(>sighand->siglock);
> syscall_tracepoint_update(p);
> + klp_update_task_universe(p);
> write_unlock_irq(_lock);
>
> proc_fork_connector(p);
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> index e136dad..2b8bdb1 100644
> --- a/kernel/livepatch/Makefile
> +++ b/kernel/livepatch/Makefile
> @@ -1,3 +1,3 @@
> obj-$(CONFIG_LIVEPATCH) += livepatch.o
>
> -livepatch-objs :=
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 11, 2015 at 05:28:13PM +0100, Miroslav Benes wrote:
> On Tue, 10 Feb 2015, Josh Poimboeuf wrote:
>
> > On Tue, Feb 10, 2015 at 04:59:17PM +0100, Miroslav Benes wrote:
> > >
> > > On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> > >
> > > > Add a basic per-task consistency model. This is the foundation which
> > > > will eventually enable us to patch those ~10% of security patches which
> > > > change function prototypes and/or data semantics.
> > > >
> > > > When a patch is enabled, livepatch enters into a transition state where
> > > > tasks are converging from the old universe to the new universe. If a
> > > > given task isn't using any of the patched functions, it's switched to
> > > > the new universe. Once all the tasks have been converged to the new
> > > > universe, patching is complete.
> > > >
> > > > The same sequence occurs when a patch is disabled, except the tasks
> > > > converge from the new universe to the old universe.
> > > >
> > > > The /sys/kernel/livepatch//transition file shows whether a patch
> > > > is in transition. Only a single patch (the topmost patch on the stack)
> > > > can be in transition at a given time. A patch can remain in the
> > > > transition state indefinitely, if any of the tasks are stuck in the
> > > > previous universe.
> > > >
> > > > A transition can be reversed and effectively canceled by writing the
> > > > opposite value to the /sys/kernel/livepatch//enabled file while
> > > > the transition is in progress. Then all the tasks will attempt to
> > > > converge back to the original universe.
> > >
> > > Hi Josh,
> > >
> > > first, thanks a lot for great work. I'm starting to go through it and
> > > it's
> > > gonna take me some time to do and send a complete review.
> >
> > I know there are a lot of details to look at, please take your time. I
> > really appreciate your review. (And everybody else's, for that matter
> > :-)
> >
> > > > + /* success! unpatch obsolete functions and do some cleanup */
> > > > +
> > > > + if (klp_universe_goal == KLP_UNIVERSE_OLD) {
> > > > + klp_unpatch_objects(klp_transition_patch);
> > > > +
> > > > + /* prevent ftrace handler from reading old
> > > > func->transition */
> > > > + synchronize_rcu();
> > > > + }
> > > > +
> > > > + pr_notice("'%s': %s complete\n",
> > > > klp_transition_patch->mod->name,
> > > > + klp_universe_goal == KLP_UNIVERSE_NEW ? "patching" :
> > > > + "unpatching");
> > > > +
> > > > + klp_complete_transition();
> > > > +}
> > >
> > > ...synchronize_rcu() could be insufficient. There still can be some
> > > process in our ftrace handler after the call.
> > >
> > > Consider the following scenario:
> > >
> > > When synchronize_rcu is called some process could have been preempted on
> > > some other cpu somewhere at the start of the ftrace handler before
> > > rcu_read_lock. synchronize_rcu waits for the grace period to pass, but
> > > that
> > > does not mean anything for our process in the handler, because it is not
> > > in rcu critical section. There is no guarantee that after synchronize_rcu
> > > the process would be away from the handler.
> > >
> > > "Meanwhile" klp_try_complete_transition continues and calls
> > > klp_complete_transition. This clears func->transition flags. Now the
> > > process in the handler could be scheduled again. It reads the wrong value
> > > of func->transition and redirection to the wrong function is done.
> > >
> > > What do you think? I hope I made myself clear.
> >
> > You really made me think. But I don't think there's a race here.
> >
> > Consider the two separate cases, patching and unpatching:
> >
> > 1. patching has completed: klp_universe_goal and all tasks'
> >klp_universes are at KLP_UNIVERSE_NEW. In this case, the value of
> >func->transition doesn't matter, because we want to use the func at
> >the top of the stack, and if klp_universe is NEW, the ftrace handler
> >will do that, regardless of the value of func->transition. This is
> >why I didn't do the rcu_synchronize() in this case. But maybe you're
> >not worried about this case anyway, I just described it for the sake
> >of completeness :-)
>
> Yes, this case shouldn't be a problem :)
>
> > 2. unpatching has completed: klp_universe_goal and all tasks'
> >klp_universes are at KLP_UNIVERSE_OLD. In this case, the value of
> >func->transition _does_ matter. However, notice that
> >klp_unpatch_objects() is called before rcu_synchronize(). That
> >removes the "new" func from the klp_ops stack. Since the ftrace
> >handler accesses the list _after_ calling rcu_read_lock(), it will
> >never see the "new" func, and thus func->transition will never be
> >set.
>
> Hm, so indeed I messed it up. Let me rework the scenario a bit. We have a
> function foo(), which
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 11, 2015 at 11:21:51AM +0100, Miroslav Benes wrote:
>
> On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
>
> [...]
>
> > @@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> > ops = container_of(fops, struct klp_ops, fops);
> >
> > rcu_read_lock();
> > +
> > func = list_first_or_null_rcu(>func_stack, struct klp_func,
> > stack_node);
> > - rcu_read_unlock();
> >
> > if (WARN_ON_ONCE(!func))
> > - return;
> > + goto unlock;
> > +
> > + if (unlikely(func->transition)) {
> > + /* corresponding smp_wmb() is in klp_init_transition() */
> > + smp_rmb();
> > +
> > + if (current->klp_universe == KLP_UNIVERSE_OLD) {
> > + /*
> > +* Use the previously patched version of the function.
> > +* If no previous patches exist, use the original
> > +* function.
> > +*/
> > + func = list_entry_rcu(func->stack_node.next,
> > + struct klp_func, stack_node);
> > +
> > + if (>stack_node == >func_stack)
> > + goto unlock;
> > + }
> > + }
> >
> > klp_arch_set_pc(regs, (unsigned long)func->new_func);
> > +unlock:
> > + rcu_read_unlock();
> > }
>
> I decided to understand the code more before answering the email about the
> race and found another problem. I think.
>
> Imagine we patched some function foo() with foo_1() from patch_1 and now
> we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch
> calls klp_init_transition which sets klp_universe for all processes to
> KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1).
> Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo().
> BUT what if somebody calls foo() right between klp_init_transition and
> the loop in __klp_enable_patch? The ftrace handler first returns the
> first entry in the list which is foo_1() (foo_2() is still not present),
> then it checks for func->transition. It is 1.
No, actually foo_1()'s func->transition will be 0. Only foo_2()'s
func->transition will be 1.
> It checks for
> current->klp_universe which is KLP_UNIVERSE_OLD and so the next entry is
> retrieved. There is no such and therefore foo() is called. This is
> obviously wrong because foo_1() was expected.
>
> Everything would work fine if one would call foo() before
> klp_start_transition and after the loop in __klp_enable_patch. The
> solution might be to move the setting of func->transition to
> klp_start_transition, but this could break something different. I don't
> know yet.
>
> Am I wrong?
>
> Miroslav
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, 10 Feb 2015, Josh Poimboeuf wrote:
> On Tue, Feb 10, 2015 at 04:59:17PM +0100, Miroslav Benes wrote:
> >
> > On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> >
> > > Add a basic per-task consistency model. This is the foundation which
> > > will eventually enable us to patch those ~10% of security patches which
> > > change function prototypes and/or data semantics.
> > >
> > > When a patch is enabled, livepatch enters into a transition state where
> > > tasks are converging from the old universe to the new universe. If a
> > > given task isn't using any of the patched functions, it's switched to
> > > the new universe. Once all the tasks have been converged to the new
> > > universe, patching is complete.
> > >
> > > The same sequence occurs when a patch is disabled, except the tasks
> > > converge from the new universe to the old universe.
> > >
> > > The /sys/kernel/livepatch//transition file shows whether a patch
> > > is in transition. Only a single patch (the topmost patch on the stack)
> > > can be in transition at a given time. A patch can remain in the
> > > transition state indefinitely, if any of the tasks are stuck in the
> > > previous universe.
> > >
> > > A transition can be reversed and effectively canceled by writing the
> > > opposite value to the /sys/kernel/livepatch//enabled file while
> > > the transition is in progress. Then all the tasks will attempt to
> > > converge back to the original universe.
> >
> > Hi Josh,
> >
> > first, thanks a lot for great work. I'm starting to go through it and it's
> > gonna take me some time to do and send a complete review.
>
> I know there are a lot of details to look at, please take your time. I
> really appreciate your review. (And everybody else's, for that matter
> :-)
>
> > > + /* success! unpatch obsolete functions and do some cleanup */
> > > +
> > > + if (klp_universe_goal == KLP_UNIVERSE_OLD) {
> > > + klp_unpatch_objects(klp_transition_patch);
> > > +
> > > + /* prevent ftrace handler from reading old func->transition */
> > > + synchronize_rcu();
> > > + }
> > > +
> > > + pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
> > > + klp_universe_goal == KLP_UNIVERSE_NEW ? "patching" :
> > > + "unpatching");
> > > +
> > > + klp_complete_transition();
> > > +}
> >
> > ...synchronize_rcu() could be insufficient. There still can be some
> > process in our ftrace handler after the call.
> >
> > Consider the following scenario:
> >
> > When synchronize_rcu is called some process could have been preempted on
> > some other cpu somewhere at the start of the ftrace handler before
> > rcu_read_lock. synchronize_rcu waits for the grace period to pass, but that
> > does not mean anything for our process in the handler, because it is not
> > in rcu critical section. There is no guarantee that after synchronize_rcu
> > the process would be away from the handler.
> >
> > "Meanwhile" klp_try_complete_transition continues and calls
> > klp_complete_transition. This clears func->transition flags. Now the
> > process in the handler could be scheduled again. It reads the wrong value
> > of func->transition and redirection to the wrong function is done.
> >
> > What do you think? I hope I made myself clear.
>
> You really made me think. But I don't think there's a race here.
>
> Consider the two separate cases, patching and unpatching:
>
> 1. patching has completed: klp_universe_goal and all tasks'
>klp_universes are at KLP_UNIVERSE_NEW. In this case, the value of
>func->transition doesn't matter, because we want to use the func at
>the top of the stack, and if klp_universe is NEW, the ftrace handler
>will do that, regardless of the value of func->transition. This is
>why I didn't do the rcu_synchronize() in this case. But maybe you're
>not worried about this case anyway, I just described it for the sake
>of completeness :-)
Yes, this case shouldn't be a problem :)
> 2. unpatching has completed: klp_universe_goal and all tasks'
>klp_universes are at KLP_UNIVERSE_OLD. In this case, the value of
>func->transition _does_ matter. However, notice that
>klp_unpatch_objects() is called before rcu_synchronize(). That
>removes the "new" func from the klp_ops stack. Since the ftrace
>handler accesses the list _after_ calling rcu_read_lock(), it will
>never see the "new" func, and thus func->transition will never be
>set.
Hm, so indeed I messed it up. Let me rework the scenario a bit. We have a
function foo(), which has been already patched with foo_1() from patch_1
and foo_2() from patch_2. Now we would like to unpatch patch_2. It is
successfully completed and klp_try_complete_transition calls
klp_unpatch_objects and synchronize_rcu. Thus foo_2() is removed from the
RCU list in ops.
Now to the funny part. After synchronize_rcu() and before
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
[...]
> @@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> ops = container_of(fops, struct klp_ops, fops);
>
> rcu_read_lock();
> +
> func = list_first_or_null_rcu(>func_stack, struct klp_func,
> stack_node);
> - rcu_read_unlock();
>
> if (WARN_ON_ONCE(!func))
> - return;
> + goto unlock;
> +
> + if (unlikely(func->transition)) {
> + /* corresponding smp_wmb() is in klp_init_transition() */
> + smp_rmb();
> +
> + if (current->klp_universe == KLP_UNIVERSE_OLD) {
> + /*
> + * Use the previously patched version of the function.
> + * If no previous patches exist, use the original
> + * function.
> + */
> + func = list_entry_rcu(func->stack_node.next,
> + struct klp_func, stack_node);
> +
> + if (>stack_node == >func_stack)
> + goto unlock;
> + }
> + }
>
> klp_arch_set_pc(regs, (unsigned long)func->new_func);
> +unlock:
> + rcu_read_unlock();
> }
I decided to understand the code more before answering the email about the
race and found another problem. I think.
Imagine we patched some function foo() with foo_1() from patch_1 and now
we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch
calls klp_init_transition which sets klp_universe for all processes to
KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1).
Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo().
BUT what if somebody calls foo() right between klp_init_transition and
the loop in __klp_enable_patch? The ftrace handler first returns the
first entry in the list which is foo_1() (foo_2() is still not present),
then it checks for func->transition. It is 1. It checks for
current->klp_universe which is KLP_UNIVERSE_OLD and so the next entry is
retrieved. There is no such and therefore foo() is called. This is
obviously wrong because foo_1() was expected.
Everything would work fine if one would call foo() before
klp_start_transition and after the loop in __klp_enable_patch. The
solution might be to move the setting of func->transition to
klp_start_transition, but this could break something different. I don't
know yet.
Am I wrong?
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, 10 Feb 2015, Josh Poimboeuf wrote:
On Tue, Feb 10, 2015 at 04:59:17PM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Hi Josh,
first, thanks a lot for great work. I'm starting to go through it and it's
gonna take me some time to do and send a complete review.
I know there are a lot of details to look at, please take your time. I
really appreciate your review. (And everybody else's, for that matter
:-)
+ /* success! unpatch obsolete functions and do some cleanup */
+
+ if (klp_universe_goal == KLP_UNIVERSE_OLD) {
+ klp_unpatch_objects(klp_transition_patch);
+
+ /* prevent ftrace handler from reading old func-transition */
+ synchronize_rcu();
+ }
+
+ pr_notice('%s': %s complete\n, klp_transition_patch-mod-name,
+ klp_universe_goal == KLP_UNIVERSE_NEW ? patching :
+ unpatching);
+
+ klp_complete_transition();
+}
...synchronize_rcu() could be insufficient. There still can be some
process in our ftrace handler after the call.
Consider the following scenario:
When synchronize_rcu is called some process could have been preempted on
some other cpu somewhere at the start of the ftrace handler before
rcu_read_lock. synchronize_rcu waits for the grace period to pass, but that
does not mean anything for our process in the handler, because it is not
in rcu critical section. There is no guarantee that after synchronize_rcu
the process would be away from the handler.
Meanwhile klp_try_complete_transition continues and calls
klp_complete_transition. This clears func-transition flags. Now the
process in the handler could be scheduled again. It reads the wrong value
of func-transition and redirection to the wrong function is done.
What do you think? I hope I made myself clear.
You really made me think. But I don't think there's a race here.
Consider the two separate cases, patching and unpatching:
1. patching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_NEW. In this case, the value of
func-transition doesn't matter, because we want to use the func at
the top of the stack, and if klp_universe is NEW, the ftrace handler
will do that, regardless of the value of func-transition. This is
why I didn't do the rcu_synchronize() in this case. But maybe you're
not worried about this case anyway, I just described it for the sake
of completeness :-)
Yes, this case shouldn't be a problem :)
2. unpatching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_OLD. In this case, the value of
func-transition _does_ matter. However, notice that
klp_unpatch_objects() is called before rcu_synchronize(). That
removes the new func from the klp_ops stack. Since the ftrace
handler accesses the list _after_ calling rcu_read_lock(), it will
never see the new func, and thus func-transition will never be
set.
Hm, so indeed I messed it up. Let me rework the scenario a bit. We have a
function foo(), which has been already patched with foo_1() from patch_1
and foo_2() from patch_2. Now we would like to unpatch patch_2. It is
successfully completed and klp_try_complete_transition calls
klp_unpatch_objects and synchronize_rcu. Thus foo_2() is removed from the
RCU list in ops.
Now to the funny part. After synchronize_rcu() and before
klp_complete_transition some process might get to the ftrace handler (it
is still there because of the patch_1 still being present). It gets foo_1
from the list_first_or_null_rcu, sees that func-transition is 1 (it
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 11, 2015 at 05:28:13PM +0100, Miroslav Benes wrote:
On Tue, 10 Feb 2015, Josh Poimboeuf wrote:
On Tue, Feb 10, 2015 at 04:59:17PM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Hi Josh,
first, thanks a lot for great work. I'm starting to go through it and
it's
gonna take me some time to do and send a complete review.
I know there are a lot of details to look at, please take your time. I
really appreciate your review. (And everybody else's, for that matter
:-)
+ /* success! unpatch obsolete functions and do some cleanup */
+
+ if (klp_universe_goal == KLP_UNIVERSE_OLD) {
+ klp_unpatch_objects(klp_transition_patch);
+
+ /* prevent ftrace handler from reading old
func-transition */
+ synchronize_rcu();
+ }
+
+ pr_notice('%s': %s complete\n,
klp_transition_patch-mod-name,
+ klp_universe_goal == KLP_UNIVERSE_NEW ? patching :
+ unpatching);
+
+ klp_complete_transition();
+}
...synchronize_rcu() could be insufficient. There still can be some
process in our ftrace handler after the call.
Consider the following scenario:
When synchronize_rcu is called some process could have been preempted on
some other cpu somewhere at the start of the ftrace handler before
rcu_read_lock. synchronize_rcu waits for the grace period to pass, but
that
does not mean anything for our process in the handler, because it is not
in rcu critical section. There is no guarantee that after synchronize_rcu
the process would be away from the handler.
Meanwhile klp_try_complete_transition continues and calls
klp_complete_transition. This clears func-transition flags. Now the
process in the handler could be scheduled again. It reads the wrong value
of func-transition and redirection to the wrong function is done.
What do you think? I hope I made myself clear.
You really made me think. But I don't think there's a race here.
Consider the two separate cases, patching and unpatching:
1. patching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_NEW. In this case, the value of
func-transition doesn't matter, because we want to use the func at
the top of the stack, and if klp_universe is NEW, the ftrace handler
will do that, regardless of the value of func-transition. This is
why I didn't do the rcu_synchronize() in this case. But maybe you're
not worried about this case anyway, I just described it for the sake
of completeness :-)
Yes, this case shouldn't be a problem :)
2. unpatching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_OLD. In this case, the value of
func-transition _does_ matter. However, notice that
klp_unpatch_objects() is called before rcu_synchronize(). That
removes the new func from the klp_ops stack. Since the ftrace
handler accesses the list _after_ calling rcu_read_lock(), it will
never see the new func, and thus func-transition will never be
set.
Hm, so indeed I messed it up. Let me rework the scenario a bit. We have a
function foo(), which has been already patched with foo_1() from patch_1
and foo_2() from patch_2. Now we would like to unpatch patch_2. It is
successfully completed and klp_try_complete_transition calls
klp_unpatch_objects and synchronize_rcu. Thus foo_2() is removed from the
RCU list in ops.
Now to the funny part. After
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Wed, Feb 11, 2015 at 11:21:51AM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
[...]
@@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip,
ops = container_of(fops, struct klp_ops, fops);
rcu_read_lock();
+
func = list_first_or_null_rcu(ops-func_stack, struct klp_func,
stack_node);
- rcu_read_unlock();
if (WARN_ON_ONCE(!func))
- return;
+ goto unlock;
+
+ if (unlikely(func-transition)) {
+ /* corresponding smp_wmb() is in klp_init_transition() */
+ smp_rmb();
+
+ if (current-klp_universe == KLP_UNIVERSE_OLD) {
+ /*
+* Use the previously patched version of the function.
+* If no previous patches exist, use the original
+* function.
+*/
+ func = list_entry_rcu(func-stack_node.next,
+ struct klp_func, stack_node);
+
+ if (func-stack_node == ops-func_stack)
+ goto unlock;
+ }
+ }
klp_arch_set_pc(regs, (unsigned long)func-new_func);
+unlock:
+ rcu_read_unlock();
}
I decided to understand the code more before answering the email about the
race and found another problem. I think.
Imagine we patched some function foo() with foo_1() from patch_1 and now
we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch
calls klp_init_transition which sets klp_universe for all processes to
KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1).
Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo().
BUT what if somebody calls foo() right between klp_init_transition and
the loop in __klp_enable_patch? The ftrace handler first returns the
first entry in the list which is foo_1() (foo_2() is still not present),
then it checks for func-transition. It is 1.
No, actually foo_1()'s func-transition will be 0. Only foo_2()'s
func-transition will be 1.
It checks for
current-klp_universe which is KLP_UNIVERSE_OLD and so the next entry is
retrieved. There is no such and therefore foo() is called. This is
obviously wrong because foo_1() was expected.
Everything would work fine if one would call foo() before
klp_start_transition and after the loop in __klp_enable_patch. The
solution might be to move the setting of func-transition to
klp_start_transition, but this could break something different. I don't
know yet.
Am I wrong?
Miroslav
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
Ingo, Peter,
Would you have any objections to making task_rq_lock/unlock() non-static
(or moving them to kernel/sched/sched.h) so they can be called by the
livepatch code?
To provide some background, I'm looking for a way to temporarily prevent
a sleeping task from running while its stack is examined, to decide
whether it can be safely switched to the new patching universe. For
more details see klp_transition_task() in the patch below.
Using task_rq_lock() is the most straightforward way I could find to
achieve that.
On Mon, Feb 09, 2015 at 11:31:18AM -0600, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Signed-off-by: Josh Poimboeuf jpoim...@redhat.com
---
include/linux/livepatch.h | 18 ++-
include/linux/sched.h | 3 +
kernel/fork.c | 2 +
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 71 ++
kernel/livepatch/patch.c | 34 -
kernel/livepatch/patch.h | 1 +
kernel/livepatch/transition.c | 300
++
kernel/livepatch/transition.h | 16 +++
kernel/sched/core.c | 2 +
10 files changed, 423 insertions(+), 26 deletions(-)
create mode 100644 kernel/livepatch/transition.c
create mode 100644 kernel/livepatch/transition.h
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0e65b4d..b8c2f15 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -40,6 +40,7 @@
* @old_size:size of the old function
* @new_size:size of the new function
* @patched: the func has been added to the klp_ops list
+ * @transition: the func is currently being applied or reverted
*/
struct klp_func {
/* external */
@@ -60,6 +61,7 @@ struct klp_func {
struct list_head stack_node;
unsigned long old_size, new_size;
int patched;
+ int transition;
};
/**
@@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
extern int klp_enable_patch(struct klp_patch *);
extern int klp_disable_patch(struct klp_patch *);
-#endif /* CONFIG_LIVEPATCH */
+extern int klp_universe_goal;
+
+static inline void klp_update_task_universe(struct task_struct *t)
+{
+ /* corresponding smp_wmb() is in klp_set_universe_goal() */
+ smp_rmb();
+
+ t-klp_universe = klp_universe_goal;
+}
+
+#else /* !CONFIG_LIVEPATCH */
+
+static inline void klp_update_task_universe(struct task_struct *t) {}
+
+#endif /* !CONFIG_LIVEPATCH */
#endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8db31ef..a95e59a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1701,6 +1701,9 @@ struct task_struct {
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
unsigned long task_state_change;
#endif
+#ifdef CONFIG_LIVEPATCH
+ int klp_universe;
+#endif
};
/* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4dc2dda..1dcbebe 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -74,6 +74,7 @@
#include linux/uprobes.h
#include linux/aio.h
#include linux/compiler.h
+#include linux/livepatch.h
#include asm/pgtable.h
#include asm/pgalloc.h
@@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
total_forks++;
spin_unlock(current-sighand-siglock);
syscall_tracepoint_update(p);
+ klp_update_task_universe(p);
write_unlock_irq(tasklist_lock);
proc_fork_connector(p);
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e136dad..2b8bdb1 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
obj-$(CONFIG_LIVEPATCH) += livepatch.o
-livepatch-objs :=
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
[...]
@@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip,
ops = container_of(fops, struct klp_ops, fops);
rcu_read_lock();
+
func = list_first_or_null_rcu(ops-func_stack, struct klp_func,
stack_node);
- rcu_read_unlock();
if (WARN_ON_ONCE(!func))
- return;
+ goto unlock;
+
+ if (unlikely(func-transition)) {
+ /* corresponding smp_wmb() is in klp_init_transition() */
+ smp_rmb();
+
+ if (current-klp_universe == KLP_UNIVERSE_OLD) {
+ /*
+ * Use the previously patched version of the function.
+ * If no previous patches exist, use the original
+ * function.
+ */
+ func = list_entry_rcu(func-stack_node.next,
+ struct klp_func, stack_node);
+
+ if (func-stack_node == ops-func_stack)
+ goto unlock;
+ }
+ }
klp_arch_set_pc(regs, (unsigned long)func-new_func);
+unlock:
+ rcu_read_unlock();
}
I decided to understand the code more before answering the email about the
race and found another problem. I think.
Imagine we patched some function foo() with foo_1() from patch_1 and now
we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch
calls klp_init_transition which sets klp_universe for all processes to
KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1).
Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo().
BUT what if somebody calls foo() right between klp_init_transition and
the loop in __klp_enable_patch? The ftrace handler first returns the
first entry in the list which is foo_1() (foo_2() is still not present),
then it checks for func-transition. It is 1. It checks for
current-klp_universe which is KLP_UNIVERSE_OLD and so the next entry is
retrieved. There is no such and therefore foo() is called. This is
obviously wrong because foo_1() was expected.
Everything would work fine if one would call foo() before
klp_start_transition and after the loop in __klp_enable_patch. The
solution might be to move the setting of func-transition to
klp_start_transition, but this could break something different. I don't
know yet.
Am I wrong?
Miroslav
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 10, 2015 at 01:27:59PM -0600, Seth Jennings wrote:
> On Mon, Feb 09, 2015 at 11:31:18AM -0600, Josh Poimboeuf wrote:
> > diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> > new file mode 100644
> > index 000..ba9a55c
> > --- /dev/null
> > +++ b/kernel/livepatch/transition.h
> > @@ -0,0 +1,16 @@
> > +#include
> > +
> > +enum {
> > + KLP_UNIVERSE_UNDEFINED = -1,
> > + KLP_UNIVERSE_OLD,
> > + KLP_UNIVERSE_NEW,
> > +};
> > +
> > +extern struct mutex klp_mutex;
>
> klp_mutex isn't defined in transition.c. Maybe this extern should be in
> the transition.c file or in a core.h file, since core.c provides the
> definition?
I originally had the extern in transition.c, but then checkpatch
complained so I moved it to transition.h. But yeah, it doesn't really
belong there either.
It's kind of ugly for transition.c to be using that mutex anyway. I
think it'll be cleaner if I just move the work_fn into core.c.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, Feb 09, 2015 at 11:31:18AM -0600, Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which
> will eventually enable us to patch those ~10% of security patches which
> change function prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
>
> Signed-off-by: Josh Poimboeuf
> ---
> include/linux/livepatch.h | 18 ++-
> include/linux/sched.h | 3 +
> kernel/fork.c | 2 +
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/core.c | 71 ++
> kernel/livepatch/patch.c | 34 -
> kernel/livepatch/patch.h | 1 +
> kernel/livepatch/transition.c | 300
> ++
> kernel/livepatch/transition.h | 16 +++
> kernel/sched/core.c | 2 +
> 10 files changed, 423 insertions(+), 26 deletions(-)
> create mode 100644 kernel/livepatch/transition.c
> create mode 100644 kernel/livepatch/transition.h
>
> diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> new file mode 100644
> index 000..ba9a55c
> --- /dev/null
> +++ b/kernel/livepatch/transition.h
> @@ -0,0 +1,16 @@
> +#include
> +
> +enum {
> + KLP_UNIVERSE_UNDEFINED = -1,
> + KLP_UNIVERSE_OLD,
> + KLP_UNIVERSE_NEW,
> +};
> +
> +extern struct mutex klp_mutex;
klp_mutex isn't defined in transition.c. Maybe this extern should be in
the transition.c file or in a core.h file, since core.c provides the
definition?
Thanks,
Seth
> +extern struct klp_patch *klp_transition_patch;
> +
> +extern void klp_init_transition(struct klp_patch *patch, int universe);
> +extern void klp_start_transition(int universe);
> +extern void klp_reverse_transition(void);
> +extern void klp_try_complete_transition(void);
> +extern void klp_complete_transition(void);
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 78d91e6..7b877f4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -74,6 +74,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -4601,6 +4602,7 @@ void init_idle(struct task_struct *idle, int cpu)
> #if defined(CONFIG_SMP)
> sprintf(idle->comm, "%s/%d", INIT_TASK_COMM, cpu);
> #endif
> + klp_update_task_universe(idle);
> }
>
> int cpuset_cpumask_can_shrink(const struct cpumask *cur,
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 10, 2015 at 04:59:17PM +0100, Miroslav Benes wrote:
>
> On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
>
> > Add a basic per-task consistency model. This is the foundation which
> > will eventually enable us to patch those ~10% of security patches which
> > change function prototypes and/or data semantics.
> >
> > When a patch is enabled, livepatch enters into a transition state where
> > tasks are converging from the old universe to the new universe. If a
> > given task isn't using any of the patched functions, it's switched to
> > the new universe. Once all the tasks have been converged to the new
> > universe, patching is complete.
> >
> > The same sequence occurs when a patch is disabled, except the tasks
> > converge from the new universe to the old universe.
> >
> > The /sys/kernel/livepatch//transition file shows whether a patch
> > is in transition. Only a single patch (the topmost patch on the stack)
> > can be in transition at a given time. A patch can remain in the
> > transition state indefinitely, if any of the tasks are stuck in the
> > previous universe.
> >
> > A transition can be reversed and effectively canceled by writing the
> > opposite value to the /sys/kernel/livepatch//enabled file while
> > the transition is in progress. Then all the tasks will attempt to
> > converge back to the original universe.
>
> Hi Josh,
>
> first, thanks a lot for great work. I'm starting to go through it and it's
> gonna take me some time to do and send a complete review.
I know there are a lot of details to look at, please take your time. I
really appreciate your review. (And everybody else's, for that matter
:-)
> > + /* success! unpatch obsolete functions and do some cleanup */
> > +
> > + if (klp_universe_goal == KLP_UNIVERSE_OLD) {
> > + klp_unpatch_objects(klp_transition_patch);
> > +
> > + /* prevent ftrace handler from reading old func->transition */
> > + synchronize_rcu();
> > + }
> > +
> > + pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
> > + klp_universe_goal == KLP_UNIVERSE_NEW ? "patching" :
> > + "unpatching");
> > +
> > + klp_complete_transition();
> > +}
>
> ...synchronize_rcu() could be insufficient. There still can be some
> process in our ftrace handler after the call.
>
> Consider the following scenario:
>
> When synchronize_rcu is called some process could have been preempted on
> some other cpu somewhere at the start of the ftrace handler before
> rcu_read_lock. synchronize_rcu waits for the grace period to pass, but that
> does not mean anything for our process in the handler, because it is not
> in rcu critical section. There is no guarantee that after synchronize_rcu
> the process would be away from the handler.
>
> "Meanwhile" klp_try_complete_transition continues and calls
> klp_complete_transition. This clears func->transition flags. Now the
> process in the handler could be scheduled again. It reads the wrong value
> of func->transition and redirection to the wrong function is done.
>
> What do you think? I hope I made myself clear.
You really made me think. But I don't think there's a race here.
Consider the two separate cases, patching and unpatching:
1. patching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_NEW. In this case, the value of
func->transition doesn't matter, because we want to use the func at
the top of the stack, and if klp_universe is NEW, the ftrace handler
will do that, regardless of the value of func->transition. This is
why I didn't do the rcu_synchronize() in this case. But maybe you're
not worried about this case anyway, I just described it for the sake
of completeness :-)
2. unpatching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_OLD. In this case, the value of
func->transition _does_ matter. However, notice that
klp_unpatch_objects() is called before rcu_synchronize(). That
removes the "new" func from the klp_ops stack. Since the ftrace
handler accesses the list _after_ calling rcu_read_lock(), it will
never see the "new" func, and thus func->transition will never be
set.
That said, I think there is a race where the WARN_ON_ONCE(!func)
could trigger here, and it wouldn't be an error. So I think I'll
remove the warning.
Does that make sense?
> There is the similar problem for dynamic trampolines in ftrace. You
> cannot remove them unless there is no process in the handler. I think
> rcu-tasks were merged a while ago for this purpose. However ftrace
> does not use them yet and I don't know if we could exploit them to
> solve this issue. I need to think more about it.
Ok, sounds like that's an ftrace bug that could affect us.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which
> will eventually enable us to patch those ~10% of security patches which
> change function prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
Hi Josh,
first, thanks a lot for great work. I'm starting to go through it and it's
gonna take me some time to do and send a complete review. Anyway, I
suspect there is a possible race in the code. I'm still not sure though.
See below...
[...]
> @@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> ops = container_of(fops, struct klp_ops, fops);
>
> rcu_read_lock();
> +
> func = list_first_or_null_rcu(>func_stack, struct klp_func,
> stack_node);
> - rcu_read_unlock();
>
> if (WARN_ON_ONCE(!func))
> - return;
> + goto unlock;
> +
> + if (unlikely(func->transition)) {
> + /* corresponding smp_wmb() is in klp_init_transition() */
> + smp_rmb();
> +
> + if (current->klp_universe == KLP_UNIVERSE_OLD) {
> + /*
> + * Use the previously patched version of the function.
> + * If no previous patches exist, use the original
> + * function.
> + */
> + func = list_entry_rcu(func->stack_node.next,
> + struct klp_func, stack_node);
> +
> + if (>stack_node == >func_stack)
> + goto unlock;
> + }
> + }
>
> klp_arch_set_pc(regs, (unsigned long)func->new_func);
> +unlock:
> + rcu_read_unlock();
> }
The problem is that there is no guarantee that ftrace handler is called in
an atomic context. Hence it could be preempted (if CONFIG_PREEMPT is y)
and it could be preempted anywhere before rcu_read_lock (which disables
preemption for CONFIG_PREEMPT). Ftrace often uses ftrace_ops_list_func as
a callback which calls the handlers with preemption disabled. But not
always. For dynamic trampolines it should call the handlers directly and
preemption is not disabled.
So...
> +/*
> + * Try to transition all tasks to the universe goal. If any tasks are still
> + * stuck in the original universe, schedule a retry.
> + */
> +void klp_try_complete_transition(void)
> +{
> + unsigned int cpu;
> + struct task_struct *g, *t;
> + bool complete = true;
> +
> + /* try to transition all normal tasks */
> + read_lock(_lock);
> + for_each_process_thread(g, t)
> + if (!klp_transition_task(t))
> + complete = false;
> + read_unlock(_lock);
> +
> + /* try to transition the idle "swapper" tasks */
> + get_online_cpus();
> + for_each_online_cpu(cpu)
> + if (!klp_transition_task(idle_task(cpu)))
> + complete = false;
> + put_online_cpus();
> +
> + /* if not complete, try again later */
> + if (!complete) {
> + schedule_delayed_work(_transition_work,
> + round_jiffies_relative(HZ));
> + return;
> + }
> +
> + /* success! unpatch obsolete functions and do some cleanup */
> +
> + if (klp_universe_goal == KLP_UNIVERSE_OLD) {
> + klp_unpatch_objects(klp_transition_patch);
> +
> + /* prevent ftrace handler from reading old func->transition */
> + synchronize_rcu();
> + }
> +
> + pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
> + klp_universe_goal == KLP_UNIVERSE_NEW ? "patching" :
> + "unpatching");
> +
> + klp_complete_transition();
> +}
...synchronize_rcu() could be insufficient. There still can be some
process in our ftrace handler after the call.
Consider the following scenario:
When synchronize_rcu is called
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 10, 2015 at 07:58:30PM +0900, Masami Hiramatsu wrote:
> (2015/02/10 2:31), Josh Poimboeuf wrote:
> > +/*
> > + * Try to safely transition a task to the universe goal. If the task is
> > + * currently running or is sleeping on a to-be-patched or to-be-unpatched
> > + * function, return false.
> > + */
> > +static bool klp_transition_task(struct task_struct *t)
> > +{
> > + struct rq *rq;
> > + unsigned long flags;
> > + int ret;
> > + bool success = false;
> > +
> > + if (t->klp_universe == klp_universe_goal)
> > + return true;
> > +
> > + rq = task_rq_lock(t, );
> > +
> > + if (task_running(rq, t) && t != current) {
> > + pr_debug("%s: pid %d (%s) is running\n", __func__, t->pid,
> > +t->comm);
> > + goto done;
> > + }
>
> Let me confirm that this always skips running tasks, and klp retries
> checking by using delayed worker, correct?
Correct. Also, patch 9 of the series adds other ways to convert tasks,
using syscalls, irqs and signals.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
(2015/02/10 2:31), Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which
> will eventually enable us to patch those ~10% of security patches which
> change function prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
>
> Signed-off-by: Josh Poimboeuf
> ---
> include/linux/livepatch.h | 18 ++-
> include/linux/sched.h | 3 +
> kernel/fork.c | 2 +
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/core.c | 71 ++
> kernel/livepatch/patch.c | 34 -
> kernel/livepatch/patch.h | 1 +
> kernel/livepatch/transition.c | 300
> ++
> kernel/livepatch/transition.h | 16 +++
> kernel/sched/core.c | 2 +
> 10 files changed, 423 insertions(+), 26 deletions(-)
> create mode 100644 kernel/livepatch/transition.c
> create mode 100644 kernel/livepatch/transition.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 0e65b4d..b8c2f15 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -40,6 +40,7 @@
> * @old_size:size of the old function
> * @new_size:size of the new function
> * @patched: the func has been added to the klp_ops list
> + * @transition: the func is currently being applied or reverted
> */
> struct klp_func {
> /* external */
> @@ -60,6 +61,7 @@ struct klp_func {
> struct list_head stack_node;
> unsigned long old_size, new_size;
> int patched;
> + int transition;
> };
>
> /**
> @@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
> extern int klp_enable_patch(struct klp_patch *);
> extern int klp_disable_patch(struct klp_patch *);
>
> -#endif /* CONFIG_LIVEPATCH */
> +extern int klp_universe_goal;
> +
> +static inline void klp_update_task_universe(struct task_struct *t)
> +{
> + /* corresponding smp_wmb() is in klp_set_universe_goal() */
> + smp_rmb();
> +
> + t->klp_universe = klp_universe_goal;
> +}
> +
> +#else /* !CONFIG_LIVEPATCH */
> +
> +static inline void klp_update_task_universe(struct task_struct *t) {}
> +
> +#endif /* !CONFIG_LIVEPATCH */
>
> #endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 8db31ef..a95e59a 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1701,6 +1701,9 @@ struct task_struct {
> #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> unsigned long task_state_change;
> #endif
> +#ifdef CONFIG_LIVEPATCH
> + int klp_universe;
> +#endif
> };
>
> /* Future-safe accessor for struct task_struct's cpus_allowed. */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4dc2dda..1dcbebe 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -74,6 +74,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
> clone_flags,
> total_forks++;
> spin_unlock(>sighand->siglock);
> syscall_tracepoint_update(p);
> + klp_update_task_universe(p);
> write_unlock_irq(_lock);
>
> proc_fork_connector(p);
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> index e136dad..2b8bdb1 100644
> --- a/kernel/livepatch/Makefile
> +++ b/kernel/livepatch/Makefile
> @@ -1,3 +1,3 @@
> obj-$(CONFIG_LIVEPATCH) += livepatch.o
>
> -livepatch-objs := core.o patch.o
> +livepatch-objs := core.o patch.o transition.o
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 85d4ef7..790dc10 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,14 +28,17 @@
> #include
>
> #include "patch.h"
> +#include "transition.h"
>
> /*
> - * The klp_mutex protects the global lists and state transitions of any
> - * structure reachable from them. References to any structure must be
> obtained
> - * under mutex protection (except in klp_ftrace_handler(), which uses
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Hi Josh,
first, thanks a lot for great work. I'm starting to go through it and it's
gonna take me some time to do and send a complete review. Anyway, I
suspect there is a possible race in the code. I'm still not sure though.
See below...
[...]
@@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip,
ops = container_of(fops, struct klp_ops, fops);
rcu_read_lock();
+
func = list_first_or_null_rcu(ops-func_stack, struct klp_func,
stack_node);
- rcu_read_unlock();
if (WARN_ON_ONCE(!func))
- return;
+ goto unlock;
+
+ if (unlikely(func-transition)) {
+ /* corresponding smp_wmb() is in klp_init_transition() */
+ smp_rmb();
+
+ if (current-klp_universe == KLP_UNIVERSE_OLD) {
+ /*
+ * Use the previously patched version of the function.
+ * If no previous patches exist, use the original
+ * function.
+ */
+ func = list_entry_rcu(func-stack_node.next,
+ struct klp_func, stack_node);
+
+ if (func-stack_node == ops-func_stack)
+ goto unlock;
+ }
+ }
klp_arch_set_pc(regs, (unsigned long)func-new_func);
+unlock:
+ rcu_read_unlock();
}
The problem is that there is no guarantee that ftrace handler is called in
an atomic context. Hence it could be preempted (if CONFIG_PREEMPT is y)
and it could be preempted anywhere before rcu_read_lock (which disables
preemption for CONFIG_PREEMPT). Ftrace often uses ftrace_ops_list_func as
a callback which calls the handlers with preemption disabled. But not
always. For dynamic trampolines it should call the handlers directly and
preemption is not disabled.
So...
+/*
+ * Try to transition all tasks to the universe goal. If any tasks are still
+ * stuck in the original universe, schedule a retry.
+ */
+void klp_try_complete_transition(void)
+{
+ unsigned int cpu;
+ struct task_struct *g, *t;
+ bool complete = true;
+
+ /* try to transition all normal tasks */
+ read_lock(tasklist_lock);
+ for_each_process_thread(g, t)
+ if (!klp_transition_task(t))
+ complete = false;
+ read_unlock(tasklist_lock);
+
+ /* try to transition the idle swapper tasks */
+ get_online_cpus();
+ for_each_online_cpu(cpu)
+ if (!klp_transition_task(idle_task(cpu)))
+ complete = false;
+ put_online_cpus();
+
+ /* if not complete, try again later */
+ if (!complete) {
+ schedule_delayed_work(klp_transition_work,
+ round_jiffies_relative(HZ));
+ return;
+ }
+
+ /* success! unpatch obsolete functions and do some cleanup */
+
+ if (klp_universe_goal == KLP_UNIVERSE_OLD) {
+ klp_unpatch_objects(klp_transition_patch);
+
+ /* prevent ftrace handler from reading old func-transition */
+ synchronize_rcu();
+ }
+
+ pr_notice('%s': %s complete\n, klp_transition_patch-mod-name,
+ klp_universe_goal == KLP_UNIVERSE_NEW ? patching :
+ unpatching);
+
+ klp_complete_transition();
+}
...synchronize_rcu() could be insufficient. There still can be some
process in our ftrace handler after the call.
Consider the following scenario:
When synchronize_rcu is called some process could have been preempted on
some other cpu somewhere at the start
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 10, 2015 at 07:58:30PM +0900, Masami Hiramatsu wrote:
(2015/02/10 2:31), Josh Poimboeuf wrote:
+/*
+ * Try to safely transition a task to the universe goal. If the task is
+ * currently running or is sleeping on a to-be-patched or to-be-unpatched
+ * function, return false.
+ */
+static bool klp_transition_task(struct task_struct *t)
+{
+ struct rq *rq;
+ unsigned long flags;
+ int ret;
+ bool success = false;
+
+ if (t-klp_universe == klp_universe_goal)
+ return true;
+
+ rq = task_rq_lock(t, flags);
+
+ if (task_running(rq, t) t != current) {
+ pr_debug(%s: pid %d (%s) is running\n, __func__, t-pid,
+t-comm);
+ goto done;
+ }
Let me confirm that this always skips running tasks, and klp retries
checking by using delayed worker, correct?
Correct. Also, patch 9 of the series adds other ways to convert tasks,
using syscalls, irqs and signals.
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 10, 2015 at 04:59:17PM +0100, Miroslav Benes wrote:
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Hi Josh,
first, thanks a lot for great work. I'm starting to go through it and it's
gonna take me some time to do and send a complete review.
I know there are a lot of details to look at, please take your time. I
really appreciate your review. (And everybody else's, for that matter
:-)
+ /* success! unpatch obsolete functions and do some cleanup */
+
+ if (klp_universe_goal == KLP_UNIVERSE_OLD) {
+ klp_unpatch_objects(klp_transition_patch);
+
+ /* prevent ftrace handler from reading old func-transition */
+ synchronize_rcu();
+ }
+
+ pr_notice('%s': %s complete\n, klp_transition_patch-mod-name,
+ klp_universe_goal == KLP_UNIVERSE_NEW ? patching :
+ unpatching);
+
+ klp_complete_transition();
+}
...synchronize_rcu() could be insufficient. There still can be some
process in our ftrace handler after the call.
Consider the following scenario:
When synchronize_rcu is called some process could have been preempted on
some other cpu somewhere at the start of the ftrace handler before
rcu_read_lock. synchronize_rcu waits for the grace period to pass, but that
does not mean anything for our process in the handler, because it is not
in rcu critical section. There is no guarantee that after synchronize_rcu
the process would be away from the handler.
Meanwhile klp_try_complete_transition continues and calls
klp_complete_transition. This clears func-transition flags. Now the
process in the handler could be scheduled again. It reads the wrong value
of func-transition and redirection to the wrong function is done.
What do you think? I hope I made myself clear.
You really made me think. But I don't think there's a race here.
Consider the two separate cases, patching and unpatching:
1. patching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_NEW. In this case, the value of
func-transition doesn't matter, because we want to use the func at
the top of the stack, and if klp_universe is NEW, the ftrace handler
will do that, regardless of the value of func-transition. This is
why I didn't do the rcu_synchronize() in this case. But maybe you're
not worried about this case anyway, I just described it for the sake
of completeness :-)
2. unpatching has completed: klp_universe_goal and all tasks'
klp_universes are at KLP_UNIVERSE_OLD. In this case, the value of
func-transition _does_ matter. However, notice that
klp_unpatch_objects() is called before rcu_synchronize(). That
removes the new func from the klp_ops stack. Since the ftrace
handler accesses the list _after_ calling rcu_read_lock(), it will
never see the new func, and thus func-transition will never be
set.
That said, I think there is a race where the WARN_ON_ONCE(!func)
could trigger here, and it wouldn't be an error. So I think I'll
remove the warning.
Does that make sense?
There is the similar problem for dynamic trampolines in ftrace. You
cannot remove them unless there is no process in the handler. I think
rcu-tasks were merged a while ago for this purpose. However ftrace
does not use them yet and I don't know if we could exploit them to
solve this issue. I need to think more about it.
Ok, sounds like that's an ftrace bug that could affect us.
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
(2015/02/10 2:31), Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Signed-off-by: Josh Poimboeuf jpoim...@redhat.com
---
include/linux/livepatch.h | 18 ++-
include/linux/sched.h | 3 +
kernel/fork.c | 2 +
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 71 ++
kernel/livepatch/patch.c | 34 -
kernel/livepatch/patch.h | 1 +
kernel/livepatch/transition.c | 300
++
kernel/livepatch/transition.h | 16 +++
kernel/sched/core.c | 2 +
10 files changed, 423 insertions(+), 26 deletions(-)
create mode 100644 kernel/livepatch/transition.c
create mode 100644 kernel/livepatch/transition.h
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0e65b4d..b8c2f15 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -40,6 +40,7 @@
* @old_size:size of the old function
* @new_size:size of the new function
* @patched: the func has been added to the klp_ops list
+ * @transition: the func is currently being applied or reverted
*/
struct klp_func {
/* external */
@@ -60,6 +61,7 @@ struct klp_func {
struct list_head stack_node;
unsigned long old_size, new_size;
int patched;
+ int transition;
};
/**
@@ -128,6 +130,20 @@ extern int klp_unregister_patch(struct klp_patch *);
extern int klp_enable_patch(struct klp_patch *);
extern int klp_disable_patch(struct klp_patch *);
-#endif /* CONFIG_LIVEPATCH */
+extern int klp_universe_goal;
+
+static inline void klp_update_task_universe(struct task_struct *t)
+{
+ /* corresponding smp_wmb() is in klp_set_universe_goal() */
+ smp_rmb();
+
+ t-klp_universe = klp_universe_goal;
+}
+
+#else /* !CONFIG_LIVEPATCH */
+
+static inline void klp_update_task_universe(struct task_struct *t) {}
+
+#endif /* !CONFIG_LIVEPATCH */
#endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8db31ef..a95e59a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1701,6 +1701,9 @@ struct task_struct {
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
unsigned long task_state_change;
#endif
+#ifdef CONFIG_LIVEPATCH
+ int klp_universe;
+#endif
};
/* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/fork.c b/kernel/fork.c
index 4dc2dda..1dcbebe 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -74,6 +74,7 @@
#include linux/uprobes.h
#include linux/aio.h
#include linux/compiler.h
+#include linux/livepatch.h
#include asm/pgtable.h
#include asm/pgalloc.h
@@ -1538,6 +1539,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
total_forks++;
spin_unlock(current-sighand-siglock);
syscall_tracepoint_update(p);
+ klp_update_task_universe(p);
write_unlock_irq(tasklist_lock);
proc_fork_connector(p);
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e136dad..2b8bdb1 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
obj-$(CONFIG_LIVEPATCH) += livepatch.o
-livepatch-objs := core.o patch.o
+livepatch-objs := core.o patch.o transition.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 85d4ef7..790dc10 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,14 +28,17 @@
#include linux/kallsyms.h
#include patch.h
+#include transition.h
/*
- * The klp_mutex protects the global lists and state transitions of any
- * structure reachable from them. References to any structure must be
obtained
- * under mutex protection (except in klp_ftrace_handler(), which uses RCU
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Mon, Feb 09, 2015 at 11:31:18AM -0600, Josh Poimboeuf wrote:
Add a basic per-task consistency model. This is the foundation which
will eventually enable us to patch those ~10% of security patches which
change function prototypes and/or data semantics.
When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe. If a
given task isn't using any of the patched functions, it's switched to
the new universe. Once all the tasks have been converged to the new
universe, patching is complete.
The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.
The /sys/kernel/livepatch/patch/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/patch/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original universe.
Signed-off-by: Josh Poimboeuf jpoim...@redhat.com
---
include/linux/livepatch.h | 18 ++-
include/linux/sched.h | 3 +
kernel/fork.c | 2 +
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 71 ++
kernel/livepatch/patch.c | 34 -
kernel/livepatch/patch.h | 1 +
kernel/livepatch/transition.c | 300
++
kernel/livepatch/transition.h | 16 +++
kernel/sched/core.c | 2 +
10 files changed, 423 insertions(+), 26 deletions(-)
create mode 100644 kernel/livepatch/transition.c
create mode 100644 kernel/livepatch/transition.h
snip
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 000..ba9a55c
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,16 @@
+#include linux/livepatch.h
+
+enum {
+ KLP_UNIVERSE_UNDEFINED = -1,
+ KLP_UNIVERSE_OLD,
+ KLP_UNIVERSE_NEW,
+};
+
+extern struct mutex klp_mutex;
klp_mutex isn't defined in transition.c. Maybe this extern should be in
the transition.c file or in a core.h file, since core.c provides the
definition?
Thanks,
Seth
+extern struct klp_patch *klp_transition_patch;
+
+extern void klp_init_transition(struct klp_patch *patch, int universe);
+extern void klp_start_transition(int universe);
+extern void klp_reverse_transition(void);
+extern void klp_try_complete_transition(void);
+extern void klp_complete_transition(void);
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 78d91e6..7b877f4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -74,6 +74,7 @@
#include linux/binfmts.h
#include linux/context_tracking.h
#include linux/compiler.h
+#include linux/livepatch.h
#include asm/switch_to.h
#include asm/tlb.h
@@ -4601,6 +4602,7 @@ void init_idle(struct task_struct *idle, int cpu)
#if defined(CONFIG_SMP)
sprintf(idle-comm, %s/%d, INIT_TASK_COMM, cpu);
#endif
+ klp_update_task_universe(idle);
}
int cpuset_cpumask_can_shrink(const struct cpumask *cur,
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 6/9] livepatch: create per-task consistency model
On Tue, Feb 10, 2015 at 01:27:59PM -0600, Seth Jennings wrote:
On Mon, Feb 09, 2015 at 11:31:18AM -0600, Josh Poimboeuf wrote:
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 000..ba9a55c
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,16 @@
+#include linux/livepatch.h
+
+enum {
+ KLP_UNIVERSE_UNDEFINED = -1,
+ KLP_UNIVERSE_OLD,
+ KLP_UNIVERSE_NEW,
+};
+
+extern struct mutex klp_mutex;
klp_mutex isn't defined in transition.c. Maybe this extern should be in
the transition.c file or in a core.h file, since core.c provides the
definition?
I originally had the extern in transition.c, but then checkpatch
complained so I moved it to transition.h. But yeah, it doesn't really
belong there either.
It's kind of ugly for transition.c to be using that mutex anyway. I
think it'll be cleaner if I just move the work_fn into core.c.
--
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
