Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Tue 2018-10-23 11:39:43, Josh Poimboeuf wrote: > On Mon, Oct 22, 2018 at 03:25:10PM +0200, Petr Mladek wrote: > > On Fri 2018-10-19 09:36:04, Josh Poimboeuf wrote: > > > On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > > > > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > > > As long as we're talking about radical changes... how about we just > > > > > don't allow disabling patches at all? Instead a patch can be replaced > > > > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > > > > code simpler and also ensure there's an audit trail. > > > > > > > The revert operation allows to remove a livepatch stuck in the > > transition without forcing. > > True, though I question the real world value of that. We ended in this situation few times with kGraft when a kthread was not annotated and migrated. We have not seen this with upstream livepatch code yet but we shipped first product with it only few months ago. I would say that it is nice to have but it is not must to have. > > One big problem would be how to keep the system consistent. You > > would need to solve races between loading modules and livepatches > > anyway. > > > > For example, you could not load fixed/patched modules when the system > > is not fully patched yet. You would need to load the module and > > the related livepatch at the same time and follow the consistency > > model as we do now. > > Yeah, I think that's pretty much the crazy idea Miroslav mentioned. The > patch would consist of several modules. The parent module would > register the patch and patch vmlinux. Each child module would be > associated with a to-be-patched module. The child modules could be > loaded on demand, either by special klp code or by modprobe. Yup, something like this. > As you described, there would be some races to think about. However, it > would also have some benefits. > > I *hope* it would mean we could get rid of a lot of our ugly hacks, like > > - klp symbols, klp relas The access to external static symbols would still need so klp-specific relocations. > - preserving ELF data, PLT's, other horrible arch-specific things > - klp.arch..altinstructions, klp.arch..parainstructions > - manually calling apply_relocate_add() Yup, these might be candidates to go. > However... we might still need some of those things for another reason: > to bypass exported symbol protections. It needs some more > investigation. > > Given this discussion, I'm thinking there wouldn't be much to discuss at > LPC for this topic unless we had a prototype to look at (which I won't > have time to do). So I may drop my talk in favor of giving more time > for other more tangible discussions. Sounds reasonable. At least I would not be able to say much more about it without seeing a more detailed proposal and ideally a prototype code. That said, I definitely do not want to discourage you from playing with the idea. Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Tue 2018-10-23 11:39:43, Josh Poimboeuf wrote: > On Mon, Oct 22, 2018 at 03:25:10PM +0200, Petr Mladek wrote: > > On Fri 2018-10-19 09:36:04, Josh Poimboeuf wrote: > > > On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > > > > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > > > As long as we're talking about radical changes... how about we just > > > > > don't allow disabling patches at all? Instead a patch can be replaced > > > > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > > > > code simpler and also ensure there's an audit trail. > > > > > > > The revert operation allows to remove a livepatch stuck in the > > transition without forcing. > > True, though I question the real world value of that. We ended in this situation few times with kGraft when a kthread was not annotated and migrated. We have not seen this with upstream livepatch code yet but we shipped first product with it only few months ago. I would say that it is nice to have but it is not must to have. > > One big problem would be how to keep the system consistent. You > > would need to solve races between loading modules and livepatches > > anyway. > > > > For example, you could not load fixed/patched modules when the system > > is not fully patched yet. You would need to load the module and > > the related livepatch at the same time and follow the consistency > > model as we do now. > > Yeah, I think that's pretty much the crazy idea Miroslav mentioned. The > patch would consist of several modules. The parent module would > register the patch and patch vmlinux. Each child module would be > associated with a to-be-patched module. The child modules could be > loaded on demand, either by special klp code or by modprobe. Yup, something like this. > As you described, there would be some races to think about. However, it > would also have some benefits. > > I *hope* it would mean we could get rid of a lot of our ugly hacks, like > > - klp symbols, klp relas The access to external static symbols would still need so klp-specific relocations. > - preserving ELF data, PLT's, other horrible arch-specific things > - klp.arch..altinstructions, klp.arch..parainstructions > - manually calling apply_relocate_add() Yup, these might be candidates to go. > However... we might still need some of those things for another reason: > to bypass exported symbol protections. It needs some more > investigation. > > Given this discussion, I'm thinking there wouldn't be much to discuss at > LPC for this topic unless we had a prototype to look at (which I won't > have time to do). So I may drop my talk in favor of giving more time > for other more tangible discussions. Sounds reasonable. At least I would not be able to say much more about it without seeing a more detailed proposal and ideally a prototype code. That said, I definitely do not want to discourage you from playing with the idea. Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Tue, Oct 23, 2018 at 11:39:43AM -0500, Josh Poimboeuf wrote: > Given this discussion, I'm thinking there wouldn't be much to discuss at > LPC for this topic unless we had a prototype to look at (which I won't > have time to do). So I may drop my talk in favor of giving more time > for other more tangible discussions. I dropped my talk to give us time for the other topics, and I somewhat arbitrarily added 5 minutes to each of the first 3 talks. Suggestions welcome. -- Josh
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Tue, Oct 23, 2018 at 11:39:43AM -0500, Josh Poimboeuf wrote: > Given this discussion, I'm thinking there wouldn't be much to discuss at > LPC for this topic unless we had a prototype to look at (which I won't > have time to do). So I may drop my talk in favor of giving more time > for other more tangible discussions. I dropped my talk to give us time for the other topics, and I somewhat arbitrarily added 5 minutes to each of the first 3 talks. Suggestions welcome. -- Josh
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Mon, Oct 22, 2018 at 03:25:10PM +0200, Petr Mladek wrote: > On Fri 2018-10-19 09:36:04, Josh Poimboeuf wrote: > > On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > > > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > > > > > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > > > > much more sense than "enable" now. > > > > > > > > > > It might be used also for the reverse operation the same way as > > > > > "enable" was used before. I think that standalone "reverse" might > > > > > be confusing when we allow to reverse the operation in both > > > > > directions. > > > > > > > > As long as we're talking about radical changes... how about we just > > > > don't allow disabling patches at all? Instead a patch can be replaced > > > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > > > code simpler and also ensure there's an audit trail. > > > > > > > > (Apologies if we've already talked about this. My brain is still mushy > > > > thanks to Spectre and friends.) > > > > > > I think we talked about it last year in Prague and I think we convinced > > > you that it was not a good idea (...not to allow disabling patches at > > > all). > > > > > > BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. > > > > I definitely remember talking about it in Prague, but I don't remember > > any conclusions. > > The revert operation allows to remove a livepatch stuck in the > transition without forcing. True, though I question the real world value of that. > Also implementing empty cumulative patch might be tricky because > of the callbacks. The current proposal is to call callbacks only > from the new livepatch. It helps tp keep the interactions easy > and under control. The way how to take over some change between > an old and new patch depends on the particular functionality. Presumably a 'no-op' patch would be special, in that it would call the un-patch callbacks. I think the only *real* benefit of this proposal would be that history would be a straight line, with no backtracking. Similar to git rebase vs merge. You'd be able to tell what has been applied and reverted just by looking at what modules are loaded. But, I now realize that in order for that to be the case, we'd have to disallow the unloading of replaced modules. But I think we're going to end up *allowing* the unloading of replaced modules, right? So maybe it's not worth it. I'll drop it. > It would mean that the empty patch might need to be custom. > Users probably would need to ask and wait for it. > > > > My livepatch-related brain cache lines have been > > flushed thanks to the aforementioned CVEs and my rapidly advancing > > senility. > > Uff, I am not the only one. :-) > > > > The amount of flexibility we allow is kind of crazy, considering how > > > > delicate of an operation live patching is. That reminds me that I > > > > should bring up my other favorite idea at LPC: require modules to be > > > > loaded before we "patch" them. > > > > > > We talked about this as well and if I remember correctly we came to a > > > conclusion that it is all about a distribution and maintenance. We cannot > > > ask customers to load modules they do not need just because we need to > > > patch them. > > > > Fair enough. > > > > > One cumulative patch is not that great in this case. I remember you > > > had a crazy idea how to solve it, but I don't remember details. My > > > notes from the event say... > > > > > > - livepatch code complexity > > > - make it synchronous with respect to modules loading > > > - Josh's crazy idea > > > > > > That's not much :D > > > > > > So yes, we can talk about it and hopefully make proper notes this time. > > > > Heh, better notes would be good, otherwise I'll just keep complaining > > about the same things every year :-) I'll try to remember what my crazy > > idea was, or maybe come up with some new ones to keep it fresh. > > If we do not want to force users to load all patched modules then > we would need to create a livepatch-per-module. This just moves > the complexity somewhere else. > > One big problem would be how to keep the system consistent. You > would need to solve races between loading modules and livepatches > anyway. > > For example, you could not load fixed/patched modules when the system > is not fully patched yet. You would need to load the module and > the related livepatch at the same time and follow the consistency > model as we do now. > > OK, there was the idea to refuse loading modules when livepatch > transition is in progress. But it might not be acceptable, > especially when the transition gets blocked infinitely > and manual intervention would be needed. > > I agree that the current solution adds complexity to > the livepatching code but it is not that complicated. > Races with loading modules
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Mon, Oct 22, 2018 at 03:25:10PM +0200, Petr Mladek wrote: > On Fri 2018-10-19 09:36:04, Josh Poimboeuf wrote: > > On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > > > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > > > > > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > > > > much more sense than "enable" now. > > > > > > > > > > It might be used also for the reverse operation the same way as > > > > > "enable" was used before. I think that standalone "reverse" might > > > > > be confusing when we allow to reverse the operation in both > > > > > directions. > > > > > > > > As long as we're talking about radical changes... how about we just > > > > don't allow disabling patches at all? Instead a patch can be replaced > > > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > > > code simpler and also ensure there's an audit trail. > > > > > > > > (Apologies if we've already talked about this. My brain is still mushy > > > > thanks to Spectre and friends.) > > > > > > I think we talked about it last year in Prague and I think we convinced > > > you that it was not a good idea (...not to allow disabling patches at > > > all). > > > > > > BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. > > > > I definitely remember talking about it in Prague, but I don't remember > > any conclusions. > > The revert operation allows to remove a livepatch stuck in the > transition without forcing. True, though I question the real world value of that. > Also implementing empty cumulative patch might be tricky because > of the callbacks. The current proposal is to call callbacks only > from the new livepatch. It helps tp keep the interactions easy > and under control. The way how to take over some change between > an old and new patch depends on the particular functionality. Presumably a 'no-op' patch would be special, in that it would call the un-patch callbacks. I think the only *real* benefit of this proposal would be that history would be a straight line, with no backtracking. Similar to git rebase vs merge. You'd be able to tell what has been applied and reverted just by looking at what modules are loaded. But, I now realize that in order for that to be the case, we'd have to disallow the unloading of replaced modules. But I think we're going to end up *allowing* the unloading of replaced modules, right? So maybe it's not worth it. I'll drop it. > It would mean that the empty patch might need to be custom. > Users probably would need to ask and wait for it. > > > > My livepatch-related brain cache lines have been > > flushed thanks to the aforementioned CVEs and my rapidly advancing > > senility. > > Uff, I am not the only one. :-) > > > > The amount of flexibility we allow is kind of crazy, considering how > > > > delicate of an operation live patching is. That reminds me that I > > > > should bring up my other favorite idea at LPC: require modules to be > > > > loaded before we "patch" them. > > > > > > We talked about this as well and if I remember correctly we came to a > > > conclusion that it is all about a distribution and maintenance. We cannot > > > ask customers to load modules they do not need just because we need to > > > patch them. > > > > Fair enough. > > > > > One cumulative patch is not that great in this case. I remember you > > > had a crazy idea how to solve it, but I don't remember details. My > > > notes from the event say... > > > > > > - livepatch code complexity > > > - make it synchronous with respect to modules loading > > > - Josh's crazy idea > > > > > > That's not much :D > > > > > > So yes, we can talk about it and hopefully make proper notes this time. > > > > Heh, better notes would be good, otherwise I'll just keep complaining > > about the same things every year :-) I'll try to remember what my crazy > > idea was, or maybe come up with some new ones to keep it fresh. > > If we do not want to force users to load all patched modules then > we would need to create a livepatch-per-module. This just moves > the complexity somewhere else. > > One big problem would be how to keep the system consistent. You > would need to solve races between loading modules and livepatches > anyway. > > For example, you could not load fixed/patched modules when the system > is not fully patched yet. You would need to load the module and > the related livepatch at the same time and follow the consistency > model as we do now. > > OK, there was the idea to refuse loading modules when livepatch > transition is in progress. But it might not be acceptable, > especially when the transition gets blocked infinitely > and manual intervention would be needed. > > I agree that the current solution adds complexity to > the livepatching code but it is not that complicated. > Races with loading modules
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Fri 2018-10-19 09:36:04, Josh Poimboeuf wrote: > On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > > > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > > > much more sense than "enable" now. > > > > > > > > It might be used also for the reverse operation the same way as > > > > "enable" was used before. I think that standalone "reverse" might > > > > be confusing when we allow to reverse the operation in both > > > > directions. > > > > > > As long as we're talking about radical changes... how about we just > > > don't allow disabling patches at all? Instead a patch can be replaced > > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > > code simpler and also ensure there's an audit trail. > > > > > > (Apologies if we've already talked about this. My brain is still mushy > > > thanks to Spectre and friends.) > > > > I think we talked about it last year in Prague and I think we convinced > > you that it was not a good idea (...not to allow disabling patches at > > all). > > > > BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. > > I definitely remember talking about it in Prague, but I don't remember > any conclusions. The revert operation allows to remove a livepatch stuck in the transition without forcing. Also implementing empty cumulative patch might be tricky because of the callbacks. The current proposal is to call callbacks only from the new livepatch. It helps tp keep the interactions easy and under control. The way how to take over some change between an old and new patch depends on the particular functionality. It would mean that the empty patch might need to be custom. Users probably would need to ask and wait for it. > My livepatch-related brain cache lines have been > flushed thanks to the aforementioned CVEs and my rapidly advancing > senility. Uff, I am not the only one. > > > The amount of flexibility we allow is kind of crazy, considering how > > > delicate of an operation live patching is. That reminds me that I > > > should bring up my other favorite idea at LPC: require modules to be > > > loaded before we "patch" them. > > > > We talked about this as well and if I remember correctly we came to a > > conclusion that it is all about a distribution and maintenance. We cannot > > ask customers to load modules they do not need just because we need to > > patch them. > > Fair enough. > > > One cumulative patch is not that great in this case. I remember you > > had a crazy idea how to solve it, but I don't remember details. My > > notes from the event say... > > > > - livepatch code complexity > > - make it synchronous with respect to modules loading > > - Josh's crazy idea > > > > That's not much :D > > > > So yes, we can talk about it and hopefully make proper notes this time. > > Heh, better notes would be good, otherwise I'll just keep complaining > about the same things every year :-) I'll try to remember what my crazy > idea was, or maybe come up with some new ones to keep it fresh. If we do not want to force users to load all patched modules then we would need to create a livepatch-per-module. This just moves the complexity somewhere else. One big problem would be how to keep the system consistent. You would need to solve races between loading modules and livepatches anyway. For example, you could not load fixed/patched modules when the system is not fully patched yet. You would need to load the module and the related livepatch at the same time and follow the consistency model as we do now. OK, there was the idea to refuse loading modules when livepatch transition is in progress. But it might not be acceptable, especially when the transition gets blocked infinitely and manual intervention would be needed. I agree that the current solution adds complexity to the livepatching code but it is not that complicated. Races with loading modules and livepatches in parallel are solved by mod->klp_active flag. There are no other races because all other operations are done on code that is not actively used. One good thing is that everything is in one place and kernel has it under control. I am open to discuss it. But we would need to come up with some clever solution. Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Fri 2018-10-19 09:36:04, Josh Poimboeuf wrote: > On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > > > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > > > much more sense than "enable" now. > > > > > > > > It might be used also for the reverse operation the same way as > > > > "enable" was used before. I think that standalone "reverse" might > > > > be confusing when we allow to reverse the operation in both > > > > directions. > > > > > > As long as we're talking about radical changes... how about we just > > > don't allow disabling patches at all? Instead a patch can be replaced > > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > > code simpler and also ensure there's an audit trail. > > > > > > (Apologies if we've already talked about this. My brain is still mushy > > > thanks to Spectre and friends.) > > > > I think we talked about it last year in Prague and I think we convinced > > you that it was not a good idea (...not to allow disabling patches at > > all). > > > > BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. > > I definitely remember talking about it in Prague, but I don't remember > any conclusions. The revert operation allows to remove a livepatch stuck in the transition without forcing. Also implementing empty cumulative patch might be tricky because of the callbacks. The current proposal is to call callbacks only from the new livepatch. It helps tp keep the interactions easy and under control. The way how to take over some change between an old and new patch depends on the particular functionality. It would mean that the empty patch might need to be custom. Users probably would need to ask and wait for it. > My livepatch-related brain cache lines have been > flushed thanks to the aforementioned CVEs and my rapidly advancing > senility. Uff, I am not the only one. > > > The amount of flexibility we allow is kind of crazy, considering how > > > delicate of an operation live patching is. That reminds me that I > > > should bring up my other favorite idea at LPC: require modules to be > > > loaded before we "patch" them. > > > > We talked about this as well and if I remember correctly we came to a > > conclusion that it is all about a distribution and maintenance. We cannot > > ask customers to load modules they do not need just because we need to > > patch them. > > Fair enough. > > > One cumulative patch is not that great in this case. I remember you > > had a crazy idea how to solve it, but I don't remember details. My > > notes from the event say... > > > > - livepatch code complexity > > - make it synchronous with respect to modules loading > > - Josh's crazy idea > > > > That's not much :D > > > > So yes, we can talk about it and hopefully make proper notes this time. > > Heh, better notes would be good, otherwise I'll just keep complaining > about the same things every year :-) I'll try to remember what my crazy > idea was, or maybe come up with some new ones to keep it fresh. If we do not want to force users to load all patched modules then we would need to create a livepatch-per-module. This just moves the complexity somewhere else. One big problem would be how to keep the system consistent. You would need to solve races between loading modules and livepatches anyway. For example, you could not load fixed/patched modules when the system is not fully patched yet. You would need to load the module and the related livepatch at the same time and follow the consistency model as we do now. OK, there was the idea to refuse loading modules when livepatch transition is in progress. But it might not be acceptable, especially when the transition gets blocked infinitely and manual intervention would be needed. I agree that the current solution adds complexity to the livepatching code but it is not that complicated. Races with loading modules and livepatches in parallel are solved by mod->klp_active flag. There are no other races because all other operations are done on code that is not actively used. One good thing is that everything is in one place and kernel has it under control. I am open to discuss it. But we would need to come up with some clever solution. Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > > On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > > > > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > > > > > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > > > > Also the API and logic is much easier. It is enough to call > > > > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > > > > disabled > > > > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then > > > > > > > the module > > > > > > > can be removed once the transition finishes and sysfs interface > > > > > > > is freed. > > > > > > > > > > > > I think it would be good to discuss our sysfs interface here as > > > > > > well. > > > > > > > > > > > > Writing '1' to enabled attribute now makes sense only when you need > > > > > > to > > > > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > > > > reversion again. > > > > > > > > > > > > Wouldn't be better to split it to two different attributes? > > > > > > Something like > > > > > > "disable" and "reverse"? It could be more intuitive. > > > > > > > > > > > > Maybe we'd also find out that even patch->enabled member is not > > > > > > useful > > > > > > anymore in such case. > > > > > > > > > > I though about this as well. I kept "enabled" because: > > > > > > > > > > + It keeps the public interface the same as before. Most people > > > > > would not notice any change in the behavior except maybe that > > > > > the interface disappears when the patch gets disabled. > > > > > > > > Well our sysfs interface is still in a testing phase as far as ABI is > > > > involved. Moreover, each live patch is bound to its base kernel by > > > > definition anyway. So we can change this without remorse, I think. > > > > But it would break tooling, which is not kernel specific. I'm not sure > > whether it would be worth the headache. After all I think the livepatch > > sysfs interface is designed for tools, not humans. > > You're right. It's probably not worth it. Oh well. > > > > > > + The reverse operation makes most sense when the transition > > > > > cannot get finished. In theory, it might be problem to > > > > > finish even the reversed one. People might want to > > > > > reverse once again and force it. Then "reverse" file > > > > > might be confusing. They might not know in which direction > > > > > they do the reverse. > > > > > > > > I still think it would be better to have a less confusing interface and > > > > it > > > > would outweigh the second remark. > > > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > > much more sense than "enable" now. > > > > > > It might be used also for the reverse operation the same way as > > > "enable" was used before. I think that standalone "reverse" might > > > be confusing when we allow to reverse the operation in both > > > directions. > > > > As long as we're talking about radical changes... how about we just > > don't allow disabling patches at all? Instead a patch can be replaced > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > code simpler and also ensure there's an audit trail. > > > > (Apologies if we've already talked about this. My brain is still mushy > > thanks to Spectre and friends.) > > I think we talked about it last year in Prague and I think we convinced > you that it was not a good idea (...not to allow disabling patches at > all). > > BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. I definitely remember talking about it in Prague, but I don't remember any conclusions. My livepatch-related brain cache lines have been flushed thanks to the aforementioned CVEs and my rapidly advancing senility. > > The amount of flexibility we allow is kind of crazy, considering how > > delicate of an operation live patching is. That reminds me that I > > should bring up my other favorite idea at LPC: require modules to be > > loaded before we "patch" them. > > We talked about this as well and if I remember correctly we came to a > conclusion that it is all about a distribution and maintenance. We cannot > ask customers to load modules they do not need just because we need to > patch them. Fair enough. > One cumulative patch is not that great in this case. I remember you > had a crazy idea how to solve it, but I don't remember details. My > notes from the event say... > > - livepatch code complexity > - make it synchronous with respect to modules loading > - Josh's crazy idea > > That's not much :D > > So yes, we can talk about it and hopefully make proper notes this time. Heh, better notes would be good, otherwise I'll just keep complaining about the same things
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Fri, Oct 19, 2018 at 02:16:19PM +0200, Miroslav Benes wrote: > On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > > > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > > On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > > > > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > > > > > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > > > > Also the API and logic is much easier. It is enough to call > > > > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > > > > disabled > > > > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then > > > > > > > the module > > > > > > > can be removed once the transition finishes and sysfs interface > > > > > > > is freed. > > > > > > > > > > > > I think it would be good to discuss our sysfs interface here as > > > > > > well. > > > > > > > > > > > > Writing '1' to enabled attribute now makes sense only when you need > > > > > > to > > > > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > > > > reversion again. > > > > > > > > > > > > Wouldn't be better to split it to two different attributes? > > > > > > Something like > > > > > > "disable" and "reverse"? It could be more intuitive. > > > > > > > > > > > > Maybe we'd also find out that even patch->enabled member is not > > > > > > useful > > > > > > anymore in such case. > > > > > > > > > > I though about this as well. I kept "enabled" because: > > > > > > > > > > + It keeps the public interface the same as before. Most people > > > > > would not notice any change in the behavior except maybe that > > > > > the interface disappears when the patch gets disabled. > > > > > > > > Well our sysfs interface is still in a testing phase as far as ABI is > > > > involved. Moreover, each live patch is bound to its base kernel by > > > > definition anyway. So we can change this without remorse, I think. > > > > But it would break tooling, which is not kernel specific. I'm not sure > > whether it would be worth the headache. After all I think the livepatch > > sysfs interface is designed for tools, not humans. > > You're right. It's probably not worth it. Oh well. > > > > > > + The reverse operation makes most sense when the transition > > > > > cannot get finished. In theory, it might be problem to > > > > > finish even the reversed one. People might want to > > > > > reverse once again and force it. Then "reverse" file > > > > > might be confusing. They might not know in which direction > > > > > they do the reverse. > > > > > > > > I still think it would be better to have a less confusing interface and > > > > it > > > > would outweigh the second remark. > > > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > > much more sense than "enable" now. > > > > > > It might be used also for the reverse operation the same way as > > > "enable" was used before. I think that standalone "reverse" might > > > be confusing when we allow to reverse the operation in both > > > directions. > > > > As long as we're talking about radical changes... how about we just > > don't allow disabling patches at all? Instead a patch can be replaced > > with a 'revert' patch, or an empty 'nop' patch. That would make our > > code simpler and also ensure there's an audit trail. > > > > (Apologies if we've already talked about this. My brain is still mushy > > thanks to Spectre and friends.) > > I think we talked about it last year in Prague and I think we convinced > you that it was not a good idea (...not to allow disabling patches at > all). > > BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. I definitely remember talking about it in Prague, but I don't remember any conclusions. My livepatch-related brain cache lines have been flushed thanks to the aforementioned CVEs and my rapidly advancing senility. > > The amount of flexibility we allow is kind of crazy, considering how > > delicate of an operation live patching is. That reminds me that I > > should bring up my other favorite idea at LPC: require modules to be > > loaded before we "patch" them. > > We talked about this as well and if I remember correctly we came to a > conclusion that it is all about a distribution and maintenance. We cannot > ask customers to load modules they do not need just because we need to > patch them. Fair enough. > One cumulative patch is not that great in this case. I remember you > had a crazy idea how to solve it, but I don't remember details. My > notes from the event say... > > - livepatch code complexity > - make it synchronous with respect to modules loading > - Josh's crazy idea > > That's not much :D > > So yes, we can talk about it and hopefully make proper notes this time. Heh, better notes would be good, otherwise I'll just keep complaining about the same things
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > > > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > > > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > > > Also the API and logic is much easier. It is enough to call > > > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > > > disabled > > > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the > > > > > > module > > > > > > can be removed once the transition finishes and sysfs interface is > > > > > > freed. > > > > > > > > > > I think it would be good to discuss our sysfs interface here as well. > > > > > > > > > > Writing '1' to enabled attribute now makes sense only when you need > > > > > to > > > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > > > reversion again. > > > > > > > > > > Wouldn't be better to split it to two different attributes? Something > > > > > like > > > > > "disable" and "reverse"? It could be more intuitive. > > > > > > > > > > Maybe we'd also find out that even patch->enabled member is not > > > > > useful > > > > > anymore in such case. > > > > > > > > I though about this as well. I kept "enabled" because: > > > > > > > > + It keeps the public interface the same as before. Most people > > > > would not notice any change in the behavior except maybe that > > > > the interface disappears when the patch gets disabled. > > > > > > Well our sysfs interface is still in a testing phase as far as ABI is > > > involved. Moreover, each live patch is bound to its base kernel by > > > definition anyway. So we can change this without remorse, I think. > > But it would break tooling, which is not kernel specific. I'm not sure > whether it would be worth the headache. After all I think the livepatch > sysfs interface is designed for tools, not humans. You're right. It's probably not worth it. Oh well. > > > > + The reverse operation makes most sense when the transition > > > > cannot get finished. In theory, it might be problem to > > > > finish even the reversed one. People might want to > > > > reverse once again and force it. Then "reverse" file > > > > might be confusing. They might not know in which direction > > > > they do the reverse. > > > > > > I still think it would be better to have a less confusing interface and > > > it > > > would outweigh the second remark. > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > much more sense than "enable" now. > > > > It might be used also for the reverse operation the same way as > > "enable" was used before. I think that standalone "reverse" might > > be confusing when we allow to reverse the operation in both > > directions. > > As long as we're talking about radical changes... how about we just > don't allow disabling patches at all? Instead a patch can be replaced > with a 'revert' patch, or an empty 'nop' patch. That would make our > code simpler and also ensure there's an audit trail. > > (Apologies if we've already talked about this. My brain is still mushy > thanks to Spectre and friends.) I think we talked about it last year in Prague and I think we convinced you that it was not a good idea (...not to allow disabling patches at all). BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. > The amount of flexibility we allow is kind of crazy, considering how > delicate of an operation live patching is. That reminds me that I > should bring up my other favorite idea at LPC: require modules to be > loaded before we "patch" them. We talked about this as well and if I remember correctly we came to a conclusion that it is all about a distribution and maintenance. We cannot ask customers to load modules they do not need just because we need to patch them. One cumulative patch is not that great in this case. I remember you had a crazy idea how to solve it, but I don't remember details. My notes from the event say... - livepatch code complexity - make it synchronous with respect to modules loading - Josh's crazy idea That's not much :D So yes, we can talk about it and hopefully make proper notes this time. Miroslav
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Thu, 18 Oct 2018, Josh Poimboeuf wrote: > On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > > On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > > > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > > > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > > > Also the API and logic is much easier. It is enough to call > > > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > > > disabled > > > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the > > > > > > module > > > > > > can be removed once the transition finishes and sysfs interface is > > > > > > freed. > > > > > > > > > > I think it would be good to discuss our sysfs interface here as well. > > > > > > > > > > Writing '1' to enabled attribute now makes sense only when you need > > > > > to > > > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > > > reversion again. > > > > > > > > > > Wouldn't be better to split it to two different attributes? Something > > > > > like > > > > > "disable" and "reverse"? It could be more intuitive. > > > > > > > > > > Maybe we'd also find out that even patch->enabled member is not > > > > > useful > > > > > anymore in such case. > > > > > > > > I though about this as well. I kept "enabled" because: > > > > > > > > + It keeps the public interface the same as before. Most people > > > > would not notice any change in the behavior except maybe that > > > > the interface disappears when the patch gets disabled. > > > > > > Well our sysfs interface is still in a testing phase as far as ABI is > > > involved. Moreover, each live patch is bound to its base kernel by > > > definition anyway. So we can change this without remorse, I think. > > But it would break tooling, which is not kernel specific. I'm not sure > whether it would be worth the headache. After all I think the livepatch > sysfs interface is designed for tools, not humans. You're right. It's probably not worth it. Oh well. > > > > + The reverse operation makes most sense when the transition > > > > cannot get finished. In theory, it might be problem to > > > > finish even the reversed one. People might want to > > > > reverse once again and force it. Then "reverse" file > > > > might be confusing. They might not know in which direction > > > > they do the reverse. > > > > > > I still think it would be better to have a less confusing interface and > > > it > > > would outweigh the second remark. > > > > OK, what about having just "disable" in sysfs. I agree that it makes > > much more sense than "enable" now. > > > > It might be used also for the reverse operation the same way as > > "enable" was used before. I think that standalone "reverse" might > > be confusing when we allow to reverse the operation in both > > directions. > > As long as we're talking about radical changes... how about we just > don't allow disabling patches at all? Instead a patch can be replaced > with a 'revert' patch, or an empty 'nop' patch. That would make our > code simpler and also ensure there's an audit trail. > > (Apologies if we've already talked about this. My brain is still mushy > thanks to Spectre and friends.) I think we talked about it last year in Prague and I think we convinced you that it was not a good idea (...not to allow disabling patches at all). BUT! Empty 'nop' patch is a new idea and we may certainly discuss it. > The amount of flexibility we allow is kind of crazy, considering how > delicate of an operation live patching is. That reminds me that I > should bring up my other favorite idea at LPC: require modules to be > loaded before we "patch" them. We talked about this as well and if I remember correctly we came to a conclusion that it is all about a distribution and maintenance. We cannot ask customers to load modules they do not need just because we need to patch them. One cumulative patch is not that great in this case. I remember you had a crazy idea how to solve it, but I don't remember details. My notes from the event say... - livepatch code complexity - make it synchronous with respect to modules loading - Josh's crazy idea That's not much :D So yes, we can talk about it and hopefully make proper notes this time. Miroslav
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > > Also the API and logic is much easier. It is enough to call > > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > > disabled > > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the > > > > > module > > > > > can be removed once the transition finishes and sysfs interface is > > > > > freed. > > > > > > > > I think it would be good to discuss our sysfs interface here as well. > > > > > > > > Writing '1' to enabled attribute now makes sense only when you need to > > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > > reversion again. > > > > > > > > Wouldn't be better to split it to two different attributes? Something > > > > like > > > > "disable" and "reverse"? It could be more intuitive. > > > > > > > > Maybe we'd also find out that even patch->enabled member is not useful > > > > anymore in such case. > > > > > > I though about this as well. I kept "enabled" because: > > > > > > + It keeps the public interface the same as before. Most people > > > would not notice any change in the behavior except maybe that > > > the interface disappears when the patch gets disabled. > > > > Well our sysfs interface is still in a testing phase as far as ABI is > > involved. Moreover, each live patch is bound to its base kernel by > > definition anyway. So we can change this without remorse, I think. But it would break tooling, which is not kernel specific. I'm not sure whether it would be worth the headache. After all I think the livepatch sysfs interface is designed for tools, not humans. > > > + The reverse operation makes most sense when the transition > > > cannot get finished. In theory, it might be problem to > > > finish even the reversed one. People might want to > > > reverse once again and force it. Then "reverse" file > > > might be confusing. They might not know in which direction > > > they do the reverse. > > > > I still think it would be better to have a less confusing interface and it > > would outweigh the second remark. > > OK, what about having just "disable" in sysfs. I agree that it makes > much more sense than "enable" now. > > It might be used also for the reverse operation the same way as > "enable" was used before. I think that standalone "reverse" might > be confusing when we allow to reverse the operation in both > directions. As long as we're talking about radical changes... how about we just don't allow disabling patches at all? Instead a patch can be replaced with a 'revert' patch, or an empty 'nop' patch. That would make our code simpler and also ensure there's an audit trail. (Apologies if we've already talked about this. My brain is still mushy thanks to Spectre and friends.) The amount of flexibility we allow is kind of crazy, considering how delicate of an operation live patching is. That reminds me that I should bring up my other favorite idea at LPC: require modules to be loaded before we "patch" them. -- Josh
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Thu, Oct 18, 2018 at 04:54:56PM +0200, Petr Mladek wrote: > On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > > Also the API and logic is much easier. It is enough to call > > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > > disabled > > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the > > > > > module > > > > > can be removed once the transition finishes and sysfs interface is > > > > > freed. > > > > > > > > I think it would be good to discuss our sysfs interface here as well. > > > > > > > > Writing '1' to enabled attribute now makes sense only when you need to > > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > > reversion again. > > > > > > > > Wouldn't be better to split it to two different attributes? Something > > > > like > > > > "disable" and "reverse"? It could be more intuitive. > > > > > > > > Maybe we'd also find out that even patch->enabled member is not useful > > > > anymore in such case. > > > > > > I though about this as well. I kept "enabled" because: > > > > > > + It keeps the public interface the same as before. Most people > > > would not notice any change in the behavior except maybe that > > > the interface disappears when the patch gets disabled. > > > > Well our sysfs interface is still in a testing phase as far as ABI is > > involved. Moreover, each live patch is bound to its base kernel by > > definition anyway. So we can change this without remorse, I think. But it would break tooling, which is not kernel specific. I'm not sure whether it would be worth the headache. After all I think the livepatch sysfs interface is designed for tools, not humans. > > > + The reverse operation makes most sense when the transition > > > cannot get finished. In theory, it might be problem to > > > finish even the reversed one. People might want to > > > reverse once again and force it. Then "reverse" file > > > might be confusing. They might not know in which direction > > > they do the reverse. > > > > I still think it would be better to have a less confusing interface and it > > would outweigh the second remark. > > OK, what about having just "disable" in sysfs. I agree that it makes > much more sense than "enable" now. > > It might be used also for the reverse operation the same way as > "enable" was used before. I think that standalone "reverse" might > be confusing when we allow to reverse the operation in both > directions. As long as we're talking about radical changes... how about we just don't allow disabling patches at all? Instead a patch can be replaced with a 'revert' patch, or an empty 'nop' patch. That would make our code simpler and also ensure there's an audit trail. (Apologies if we've already talked about this. My brain is still mushy thanks to Spectre and friends.) The amount of flexibility we allow is kind of crazy, considering how delicate of an operation live patching is. That reminds me that I should bring up my other favorite idea at LPC: require modules to be loaded before we "patch" them. -- Josh
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > Also the API and logic is much easier. It is enough to call > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > disabled > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the > > > > module > > > > can be removed once the transition finishes and sysfs interface is > > > > freed. > > > > > > I think it would be good to discuss our sysfs interface here as well. > > > > > > Writing '1' to enabled attribute now makes sense only when you need to > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > reversion again. > > > > > > Wouldn't be better to split it to two different attributes? Something > > > like > > > "disable" and "reverse"? It could be more intuitive. > > > > > > Maybe we'd also find out that even patch->enabled member is not useful > > > anymore in such case. > > > > I though about this as well. I kept "enabled" because: > > > > + It keeps the public interface the same as before. Most people > > would not notice any change in the behavior except maybe that > > the interface disappears when the patch gets disabled. > > Well our sysfs interface is still in a testing phase as far as ABI is > involved. Moreover, each live patch is bound to its base kernel by > definition anyway. So we can change this without remorse, I think. > > > + The reverse operation makes most sense when the transition > > cannot get finished. In theory, it might be problem to > > finish even the reversed one. People might want to > > reverse once again and force it. Then "reverse" file > > might be confusing. They might not know in which direction > > they do the reverse. > > I still think it would be better to have a less confusing interface and it > would outweigh the second remark. OK, what about having just "disable" in sysfs. I agree that it makes much more sense than "enable" now. It might be used also for the reverse operation the same way as "enable" was used before. I think that standalone "reverse" might be confusing when we allow to reverse the operation in both directions. Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Mon 2018-10-15 18:01:43, Miroslav Benes wrote: > On Fri, 12 Oct 2018, Petr Mladek wrote: > > > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > > Also the API and logic is much easier. It is enough to call > > > > klp_enable_patch() in module_init() call. The patch patch can be > > > > disabled > > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the > > > > module > > > > can be removed once the transition finishes and sysfs interface is > > > > freed. > > > > > > I think it would be good to discuss our sysfs interface here as well. > > > > > > Writing '1' to enabled attribute now makes sense only when you need to > > > reverse an unpatching transition. Writing '0' means "disable" or a > > > reversion again. > > > > > > Wouldn't be better to split it to two different attributes? Something > > > like > > > "disable" and "reverse"? It could be more intuitive. > > > > > > Maybe we'd also find out that even patch->enabled member is not useful > > > anymore in such case. > > > > I though about this as well. I kept "enabled" because: > > > > + It keeps the public interface the same as before. Most people > > would not notice any change in the behavior except maybe that > > the interface disappears when the patch gets disabled. > > Well our sysfs interface is still in a testing phase as far as ABI is > involved. Moreover, each live patch is bound to its base kernel by > definition anyway. So we can change this without remorse, I think. > > > + The reverse operation makes most sense when the transition > > cannot get finished. In theory, it might be problem to > > finish even the reversed one. People might want to > > reverse once again and force it. Then "reverse" file > > might be confusing. They might not know in which direction > > they do the reverse. > > I still think it would be better to have a less confusing interface and it > would outweigh the second remark. OK, what about having just "disable" in sysfs. I agree that it makes much more sense than "enable" now. It might be used also for the reverse operation the same way as "enable" was used before. I think that standalone "reverse" might be confusing when we allow to reverse the operation in both directions. Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Fri, 12 Oct 2018, Petr Mladek wrote: > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > Also the API and logic is much easier. It is enough to call > > > klp_enable_patch() in module_init() call. The patch patch can be disabled > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the module > > > can be removed once the transition finishes and sysfs interface is freed. > > > > I think it would be good to discuss our sysfs interface here as well. > > > > Writing '1' to enabled attribute now makes sense only when you need to > > reverse an unpatching transition. Writing '0' means "disable" or a > > reversion again. > > > > Wouldn't be better to split it to two different attributes? Something like > > "disable" and "reverse"? It could be more intuitive. > > > > Maybe we'd also find out that even patch->enabled member is not useful > > anymore in such case. > > I though about this as well. I kept "enabled" because: > > + It keeps the public interface the same as before. Most people > would not notice any change in the behavior except maybe that > the interface disappears when the patch gets disabled. Well our sysfs interface is still in a testing phase as far as ABI is involved. Moreover, each live patch is bound to its base kernel by definition anyway. So we can change this without remorse, I think. > + The reverse operation makes most sense when the transition > cannot get finished. In theory, it might be problem to > finish even the reversed one. People might want to > reverse once again and force it. Then "reverse" file > might be confusing. They might not know in which direction > they do the reverse. I still think it would be better to have a less confusing interface and it would outweigh the second remark. > > > @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch > > > *patch) > > > if (WARN_ON(patch->enabled)) > > > return -EINVAL; > > > > > > - /* enforce stacking: only the first disabled patch can be enabled */ > > > - if (patch->list.prev != _patches && > > > - !list_prev_entry(patch, list)->enabled) > > > - return -EBUSY; > > > - > > > - /* > > > - * A reference is taken on the patch module to prevent it from being > > > - * unloaded. > > > - */ > > > - if (!try_module_get(patch->mod)) > > > - return -ENODEV; > > > + if (!patch->kobj.state_initialized) > > > + return -EINVAL; > > > > I think the check is not needed here. __klp_enable_patch() is called right > > after > > klp_init_patch() in klp_enable_patch(). > > I would keep it. Someone might want to call this also from other > location. Even we used to do it from enable_store() ;-) Ok, I don't mind in the end. Miroslav
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Fri, 12 Oct 2018, Petr Mladek wrote: > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > Also the API and logic is much easier. It is enough to call > > > klp_enable_patch() in module_init() call. The patch patch can be disabled > > > by writing '0' into /sys/kernel/livepatch//enabled. Then the module > > > can be removed once the transition finishes and sysfs interface is freed. > > > > I think it would be good to discuss our sysfs interface here as well. > > > > Writing '1' to enabled attribute now makes sense only when you need to > > reverse an unpatching transition. Writing '0' means "disable" or a > > reversion again. > > > > Wouldn't be better to split it to two different attributes? Something like > > "disable" and "reverse"? It could be more intuitive. > > > > Maybe we'd also find out that even patch->enabled member is not useful > > anymore in such case. > > I though about this as well. I kept "enabled" because: > > + It keeps the public interface the same as before. Most people > would not notice any change in the behavior except maybe that > the interface disappears when the patch gets disabled. Well our sysfs interface is still in a testing phase as far as ABI is involved. Moreover, each live patch is bound to its base kernel by definition anyway. So we can change this without remorse, I think. > + The reverse operation makes most sense when the transition > cannot get finished. In theory, it might be problem to > finish even the reversed one. People might want to > reverse once again and force it. Then "reverse" file > might be confusing. They might not know in which direction > they do the reverse. I still think it would be better to have a less confusing interface and it would outweigh the second remark. > > > @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch > > > *patch) > > > if (WARN_ON(patch->enabled)) > > > return -EINVAL; > > > > > > - /* enforce stacking: only the first disabled patch can be enabled */ > > > - if (patch->list.prev != _patches && > > > - !list_prev_entry(patch, list)->enabled) > > > - return -EBUSY; > > > - > > > - /* > > > - * A reference is taken on the patch module to prevent it from being > > > - * unloaded. > > > - */ > > > - if (!try_module_get(patch->mod)) > > > - return -ENODEV; > > > + if (!patch->kobj.state_initialized) > > > + return -EINVAL; > > > > I think the check is not needed here. __klp_enable_patch() is called right > > after > > klp_init_patch() in klp_enable_patch(). > > I would keep it. Someone might want to call this also from other > location. Even we used to do it from enable_store() ;-) Ok, I don't mind in the end. Miroslav
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > On Tue, 28 Aug 2018, Petr Mladek wrote: > > Also the API and logic is much easier. It is enough to call > > klp_enable_patch() in module_init() call. The patch patch can be disabled > > by writing '0' into /sys/kernel/livepatch//enabled. Then the module > > can be removed once the transition finishes and sysfs interface is freed. > > I think it would be good to discuss our sysfs interface here as well. > > Writing '1' to enabled attribute now makes sense only when you need to > reverse an unpatching transition. Writing '0' means "disable" or a > reversion again. > > Wouldn't be better to split it to two different attributes? Something like > "disable" and "reverse"? It could be more intuitive. > > Maybe we'd also find out that even patch->enabled member is not useful > anymore in such case. I though about this as well. I kept "enabled" because: + It keeps the public interface the same as before. Most people would not notice any change in the behavior except maybe that the interface disappears when the patch gets disabled. + The reverse operation makes most sense when the transition cannot get finished. In theory, it might be problem to finish even the reversed one. People might want to reverse once again and force it. Then "reverse" file might be confusing. They might not know in which direction they do the reverse. > > @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch *patch) > > if (WARN_ON(patch->enabled)) > > return -EINVAL; > > > > - /* enforce stacking: only the first disabled patch can be enabled */ > > - if (patch->list.prev != _patches && > > - !list_prev_entry(patch, list)->enabled) > > - return -EBUSY; > > - > > - /* > > -* A reference is taken on the patch module to prevent it from being > > -* unloaded. > > -*/ > > - if (!try_module_get(patch->mod)) > > - return -ENODEV; > > + if (!patch->kobj.state_initialized) > > + return -EINVAL; > > I think the check is not needed here. __klp_enable_patch() is called right > after > klp_init_patch() in klp_enable_patch(). I would keep it. Someone might want to call this also from other location. Even we used to do it from enable_store() ;-) Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > On Tue, 28 Aug 2018, Petr Mladek wrote: > > Also the API and logic is much easier. It is enough to call > > klp_enable_patch() in module_init() call. The patch patch can be disabled > > by writing '0' into /sys/kernel/livepatch//enabled. Then the module > > can be removed once the transition finishes and sysfs interface is freed. > > I think it would be good to discuss our sysfs interface here as well. > > Writing '1' to enabled attribute now makes sense only when you need to > reverse an unpatching transition. Writing '0' means "disable" or a > reversion again. > > Wouldn't be better to split it to two different attributes? Something like > "disable" and "reverse"? It could be more intuitive. > > Maybe we'd also find out that even patch->enabled member is not useful > anymore in such case. I though about this as well. I kept "enabled" because: + It keeps the public interface the same as before. Most people would not notice any change in the behavior except maybe that the interface disappears when the patch gets disabled. + The reverse operation makes most sense when the transition cannot get finished. In theory, it might be problem to finish even the reversed one. People might want to reverse once again and force it. Then "reverse" file might be confusing. They might not know in which direction they do the reverse. > > @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch *patch) > > if (WARN_ON(patch->enabled)) > > return -EINVAL; > > > > - /* enforce stacking: only the first disabled patch can be enabled */ > > - if (patch->list.prev != _patches && > > - !list_prev_entry(patch, list)->enabled) > > - return -EBUSY; > > - > > - /* > > -* A reference is taken on the patch module to prevent it from being > > -* unloaded. > > -*/ > > - if (!try_module_get(patch->mod)) > > - return -ENODEV; > > + if (!patch->kobj.state_initialized) > > + return -EINVAL; > > I think the check is not needed here. __klp_enable_patch() is called right > after > klp_init_patch() in klp_enable_patch(). I would keep it. Someone might want to call this also from other location. Even we used to do it from enable_store() ;-) Best Regards, Petr
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Tue, 28 Aug 2018, Petr Mladek wrote:
> The possibility to re-enable a registered patch was useful for immediate
> patches where the livepatch module had to stay until the system reboot.
> The improved consistency model allows to achieve the same result by
> unloading and loading the livepatch module again.
>
> Also we are going to add a feature called atomic replace. It will allow
> to create a patch that would replace all already registered patches. The
> aim is to handle dependent patches a more secure way. It will obsolete
"in a more secure way", or "more securely" is maybe even better.
> the stack of patches that helped to handle the dependencies so far.
> Then it might be unclear when a cumulative patch re-enabling is safe.
>
> It would be complicated to support the many modes. Instead we could
> actually make the API and code easier.
s/easier/simpler/ ?
or "easier to understand" ?
> This patch removes the two step public API. All the checks and init calls
> are moved from klp_register_patch() to klp_enabled_patch(). Also the patch
> is automatically freed, including the sysfs interface when the transition
> to the disabled state is completed.
>
> As a result, there is newer a disabled patch on the top of the stack.
s/newer/never/
> Therefore we do not need to check the stack in __klp_enable_patch().
> And we could simplify the check in __klp_disable_patch().
>
> Also the API and logic is much easier. It is enough to call
> klp_enable_patch() in module_init() call. The patch patch can be disabled
> by writing '0' into /sys/kernel/livepatch//enabled. Then the module
> can be removed once the transition finishes and sysfs interface is freed.
I think it would be good to discuss our sysfs interface here as well.
Writing '1' to enabled attribute now makes sense only when you need to
reverse an unpatching transition. Writing '0' means "disable" or a
reversion again.
Wouldn't be better to split it to two different attributes? Something like
"disable" and "reverse"? It could be more intuitive.
Maybe we'd also find out that even patch->enabled member is not useful
anymore in such case.
> diff --git a/Documentation/livepatch/livepatch.txt
> b/Documentation/livepatch/livepatch.txt
> index 2d7ed09dbd59..7fb01d27d81d 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -14,10 +14,8 @@ Table of Contents:
[...]
> -5.2. Enabling
> +5.1. Enabling
> -
>
> -Registered patches might be enabled either by calling klp_enable_patch() or
> -by writing '1' to /sys/kernel/livepatch//enabled. The system will
> -start using the new implementation of the patched functions at this stage.
> +Livepatch modules have to call klp_enable_patch() in module_init() callback.
> +This function is rather complex and might even fail in the early phase.
>
> -When a patch is enabled, livepatch enters into a transition state where
> -tasks are converging to the patched state. This is indicated by a value
> -of '1' in /sys/kernel/livepatch//transition. Once all tasks have
> -been patched, the 'transition' value changes to '0'. For more
> -information about this process, see the "Consistency model" section.
> +First, the addresses of the patched functions are found according to their
> +names. The special relocations, mentioned in the section "New functions",
> +are applied. The relevant entries are created under
> +/sys/kernel/livepatch/. The patch is rejected when any above
> +operation fails.
>
> -If an original function is patched for the first time, a function
> -specific struct klp_ops is created and an universal ftrace handler is
> -registered.
> +Third, livepatch enters into a transition state where tasks are converging
s/Third/Second/ ?
[...]
> @@ -655,116 +660,38 @@ static int klp_init_patch(struct klp_patch *patch)
> struct klp_object *obj;
> int ret;
>
> - if (!patch->objs)
> - return -EINVAL;
> -
> - mutex_lock(_mutex);
> -
> patch->enabled = false;
> - patch->forced = false;
> + patch->module_put = false;
> INIT_LIST_HEAD(>list);
> init_completion(>finish);
>
> + if (!patch->objs)
> + return -EINVAL;
> +
> + /*
> + * A reference is taken on the patch module to prevent it from being
> + * unloaded.
> + */
> + if (!try_module_get(patch->mod))
> + return -ENODEV;
> + patch->module_put = true;
> +
> ret = kobject_init_and_add(>kobj, _ktype_patch,
> klp_root_kobj, "%s", patch->mod->name);
> if (ret) {
> - mutex_unlock(_mutex);
> return ret;
> }
{ } are not necessary after the change.
[...]
> static int __klp_enable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (WARN_ON(patch->enabled))
> return -EINVAL;
>
> - /*
Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
On Tue, 28 Aug 2018, Petr Mladek wrote:
> The possibility to re-enable a registered patch was useful for immediate
> patches where the livepatch module had to stay until the system reboot.
> The improved consistency model allows to achieve the same result by
> unloading and loading the livepatch module again.
>
> Also we are going to add a feature called atomic replace. It will allow
> to create a patch that would replace all already registered patches. The
> aim is to handle dependent patches a more secure way. It will obsolete
"in a more secure way", or "more securely" is maybe even better.
> the stack of patches that helped to handle the dependencies so far.
> Then it might be unclear when a cumulative patch re-enabling is safe.
>
> It would be complicated to support the many modes. Instead we could
> actually make the API and code easier.
s/easier/simpler/ ?
or "easier to understand" ?
> This patch removes the two step public API. All the checks and init calls
> are moved from klp_register_patch() to klp_enabled_patch(). Also the patch
> is automatically freed, including the sysfs interface when the transition
> to the disabled state is completed.
>
> As a result, there is newer a disabled patch on the top of the stack.
s/newer/never/
> Therefore we do not need to check the stack in __klp_enable_patch().
> And we could simplify the check in __klp_disable_patch().
>
> Also the API and logic is much easier. It is enough to call
> klp_enable_patch() in module_init() call. The patch patch can be disabled
> by writing '0' into /sys/kernel/livepatch//enabled. Then the module
> can be removed once the transition finishes and sysfs interface is freed.
I think it would be good to discuss our sysfs interface here as well.
Writing '1' to enabled attribute now makes sense only when you need to
reverse an unpatching transition. Writing '0' means "disable" or a
reversion again.
Wouldn't be better to split it to two different attributes? Something like
"disable" and "reverse"? It could be more intuitive.
Maybe we'd also find out that even patch->enabled member is not useful
anymore in such case.
> diff --git a/Documentation/livepatch/livepatch.txt
> b/Documentation/livepatch/livepatch.txt
> index 2d7ed09dbd59..7fb01d27d81d 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -14,10 +14,8 @@ Table of Contents:
[...]
> -5.2. Enabling
> +5.1. Enabling
> -
>
> -Registered patches might be enabled either by calling klp_enable_patch() or
> -by writing '1' to /sys/kernel/livepatch//enabled. The system will
> -start using the new implementation of the patched functions at this stage.
> +Livepatch modules have to call klp_enable_patch() in module_init() callback.
> +This function is rather complex and might even fail in the early phase.
>
> -When a patch is enabled, livepatch enters into a transition state where
> -tasks are converging to the patched state. This is indicated by a value
> -of '1' in /sys/kernel/livepatch//transition. Once all tasks have
> -been patched, the 'transition' value changes to '0'. For more
> -information about this process, see the "Consistency model" section.
> +First, the addresses of the patched functions are found according to their
> +names. The special relocations, mentioned in the section "New functions",
> +are applied. The relevant entries are created under
> +/sys/kernel/livepatch/. The patch is rejected when any above
> +operation fails.
>
> -If an original function is patched for the first time, a function
> -specific struct klp_ops is created and an universal ftrace handler is
> -registered.
> +Third, livepatch enters into a transition state where tasks are converging
s/Third/Second/ ?
[...]
> @@ -655,116 +660,38 @@ static int klp_init_patch(struct klp_patch *patch)
> struct klp_object *obj;
> int ret;
>
> - if (!patch->objs)
> - return -EINVAL;
> -
> - mutex_lock(_mutex);
> -
> patch->enabled = false;
> - patch->forced = false;
> + patch->module_put = false;
> INIT_LIST_HEAD(>list);
> init_completion(>finish);
>
> + if (!patch->objs)
> + return -EINVAL;
> +
> + /*
> + * A reference is taken on the patch module to prevent it from being
> + * unloaded.
> + */
> + if (!try_module_get(patch->mod))
> + return -ENODEV;
> + patch->module_put = true;
> +
> ret = kobject_init_and_add(>kobj, _ktype_patch,
> klp_root_kobj, "%s", patch->mod->name);
> if (ret) {
> - mutex_unlock(_mutex);
> return ret;
> }
{ } are not necessary after the change.
[...]
> static int __klp_enable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (WARN_ON(patch->enabled))
> return -EINVAL;
>
> - /*
