Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On 12/06/2018 07:37 AM, Petr Mladek wrote: > On Thu 2018-12-06 11:15:55, Petr Mladek wrote: >> On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: >>> > I don't have many code comments as the changes appear to safely and > correctly do what the say. (We are at v14 after all :) I mainly > compared the text and comments to the implementation and noted typos > (marked by substitution s/old/new) and awkward wordings (marked by > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > want these to hold up the patchset. Thanks a lot both you and Miroslav for the review. I'll give it some more days before I prepare v15. I wonder if Josh could find some cycle to look at it at least from the top level. >>> >>> For what is worth, I'm fine with all the changes Joe proposed and you can >>> preserve my acks there. >> >> We need enough time for bikeshedding about comments and documentation. >> I need some time to recover. And I still hope that Josh can give it >> a spin. > > Please, forget the above paragraph. I did not get Mirek's one correctly. > I understood it as "Why do we need to wait? You have two acks. > Jump on it and send us v15 immediately." > No worries :) I usually interpret language bikeshedding as an indication that the code is just about done. BTW, I think Josh will be back online next week, as he is out of the office this week. -- Joe
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On 12/06/2018 07:37 AM, Petr Mladek wrote: > On Thu 2018-12-06 11:15:55, Petr Mladek wrote: >> On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: >>> > I don't have many code comments as the changes appear to safely and > correctly do what the say. (We are at v14 after all :) I mainly > compared the text and comments to the implementation and noted typos > (marked by substitution s/old/new) and awkward wordings (marked by > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > want these to hold up the patchset. Thanks a lot both you and Miroslav for the review. I'll give it some more days before I prepare v15. I wonder if Josh could find some cycle to look at it at least from the top level. >>> >>> For what is worth, I'm fine with all the changes Joe proposed and you can >>> preserve my acks there. >> >> We need enough time for bikeshedding about comments and documentation. >> I need some time to recover. And I still hope that Josh can give it >> a spin. > > Please, forget the above paragraph. I did not get Mirek's one correctly. > I understood it as "Why do we need to wait? You have two acks. > Jump on it and send us v15 immediately." > No worries :) I usually interpret language bikeshedding as an indication that the code is just about done. BTW, I think Josh will be back online next week, as he is out of the office this week. -- Joe
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On Thu 2018-12-06 11:15:55, Petr Mladek wrote: > On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: > > > > > > I don't have many code comments as the changes appear to safely and > > > > correctly do what the say. (We are at v14 after all :) I mainly > > > > compared the text and comments to the implementation and noted typos > > > > (marked by substitution s/old/new) and awkward wordings (marked by > > > > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > > > > want these to hold up the patchset. > > > > > > Thanks a lot both you and Miroslav for the review. > > > > > > I'll give it some more days before I prepare v15. I wonder if Josh > > > could find some cycle to look at it at least from the top level. > > > > For what is worth, I'm fine with all the changes Joe proposed and you can > > preserve my acks there. > > We need enough time for bikeshedding about comments and documentation. > I need some time to recover. And I still hope that Josh can give it > a spin. Please, forget the above paragraph. I did not get Mirek's one correctly. I understood it as "Why do we need to wait? You have two acks. Jump on it and send us v15 immediately." Best Reards, Petr
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On Thu 2018-12-06 11:15:55, Petr Mladek wrote: > On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: > > > > > > I don't have many code comments as the changes appear to safely and > > > > correctly do what the say. (We are at v14 after all :) I mainly > > > > compared the text and comments to the implementation and noted typos > > > > (marked by substitution s/old/new) and awkward wordings (marked by > > > > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > > > > want these to hold up the patchset. > > > > > > Thanks a lot both you and Miroslav for the review. > > > > > > I'll give it some more days before I prepare v15. I wonder if Josh > > > could find some cycle to look at it at least from the top level. > > > > For what is worth, I'm fine with all the changes Joe proposed and you can > > preserve my acks there. > > We need enough time for bikeshedding about comments and documentation. > I need some time to recover. And I still hope that Josh can give it > a spin. Please, forget the above paragraph. I did not get Mirek's one correctly. I understood it as "Why do we need to wait? You have two acks. Jump on it and send us v15 immediately." Best Reards, Petr
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: > > > > I don't have many code comments as the changes appear to safely and > > > correctly do what the say. (We are at v14 after all :) I mainly > > > compared the text and comments to the implementation and noted typos > > > (marked by substitution s/old/new) and awkward wordings (marked by > > > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > > > want these to hold up the patchset. > > > > Thanks a lot both you and Miroslav for the review. > > > > I'll give it some more days before I prepare v15. I wonder if Josh > > could find some cycle to look at it at least from the top level. > > For what is worth, I'm fine with all the changes Joe proposed and you can > preserve my acks there. We need enough time for bikeshedding about comments and documentation. I need some time to recover. And I still hope that Josh can give it a spin. Best Regards, Petr
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: > > > > I don't have many code comments as the changes appear to safely and > > > correctly do what the say. (We are at v14 after all :) I mainly > > > compared the text and comments to the implementation and noted typos > > > (marked by substitution s/old/new) and awkward wordings (marked by > > > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > > > want these to hold up the patchset. > > > > Thanks a lot both you and Miroslav for the review. > > > > I'll give it some more days before I prepare v15. I wonder if Josh > > could find some cycle to look at it at least from the top level. > > For what is worth, I'm fine with all the changes Joe proposed and you can > preserve my acks there. We need enough time for bikeshedding about comments and documentation. I need some time to recover. And I still hope that Josh can give it a spin. Best Regards, Petr
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
> > I don't have many code comments as the changes appear to safely and > > correctly do what the say. (We are at v14 after all :) I mainly > > compared the text and comments to the implementation and noted typos > > (marked by substitution s/old/new) and awkward wordings (marked by > > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > > want these to hold up the patchset. > > Thanks a lot both you and Miroslav for the review. > > I'll give it some more days before I prepare v15. I wonder if Josh > could find some cycle to look at it at least from the top level. For what is worth, I'm fine with all the changes Joe proposed and you can preserve my acks there. Miroslav
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
> > I don't have many code comments as the changes appear to safely and > > correctly do what the say. (We are at v14 after all :) I mainly > > compared the text and comments to the implementation and noted typos > > (marked by substitution s/old/new) and awkward wordings (marked by > > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > > want these to hold up the patchset. > > Thanks a lot both you and Miroslav for the review. > > I'll give it some more days before I prepare v15. I wonder if Josh > could find some cycle to look at it at least from the top level. For what is worth, I'm fine with all the changes Joe proposed and you can preserve my acks there. Miroslav
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On Wed 2018-12-05 15:49:14, Joe Lawrence wrote: > On 11/29/2018 04:44 AM, Petr Mladek wrote: > > The atomic replace allows to create cumulative patches. They > > are useful when you maintain many livepatches and want to remove > > one that is lower on the stack. In addition it is very useful when > > more patches touch the same function and there are dependencies > > between them. > > > > Changes against v13: > > > > + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] > > > > Aside: I don't suppose that this could ever be folded into the kobject > code/data structure itself? This seems like a common problem that > kobj-users will need to solve like this. I am afraid that it does not have much chance to get solved in kobject. They are not designed to be used in static objets and there is pushback against this usecase. > I don't have many code comments as the changes appear to safely and > correctly do what the say. (We are at v14 after all :) I mainly > compared the text and comments to the implementation and noted typos > (marked by substitution s/old/new) and awkward wordings (marked by > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > want these to hold up the patchset. Thanks a lot both you and Miroslav for the review. I'll give it some more days before I prepare v15. I wonder if Josh could find some cycle to look at it at least from the top level. Best Regards, Petr
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On Wed 2018-12-05 15:49:14, Joe Lawrence wrote: > On 11/29/2018 04:44 AM, Petr Mladek wrote: > > The atomic replace allows to create cumulative patches. They > > are useful when you maintain many livepatches and want to remove > > one that is lower on the stack. In addition it is very useful when > > more patches touch the same function and there are dependencies > > between them. > > > > Changes against v13: > > > > + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] > > > > Aside: I don't suppose that this could ever be folded into the kobject > code/data structure itself? This seems like a common problem that > kobj-users will need to solve like this. I am afraid that it does not have much chance to get solved in kobject. They are not designed to be used in static objets and there is pushback against this usecase. > I don't have many code comments as the changes appear to safely and > correctly do what the say. (We are at v14 after all :) I mainly > compared the text and comments to the implementation and noted typos > (marked by substitution s/old/new) and awkward wordings (marked by > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > want these to hold up the patchset. Thanks a lot both you and Miroslav for the review. I'll give it some more days before I prepare v15. I wonder if Josh could find some cycle to look at it at least from the top level. Best Regards, Petr
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On 11/29/2018 04:44 AM, Petr Mladek wrote: > Hi, > > I have an updated present for your mailboxes. > > The atomic replace allows to create cumulative patches. They > are useful when you maintain many livepatches and want to remove > one that is lower on the stack. In addition it is very useful when > more patches touch the same function and there are dependencies > between them. > > All the changes were simple in principle but they required quite > some refactoring again :-( IMHO, the biggest change is renaming > klp_init_lists() ->klp_init_patch_before_free(). It does all > init actions that need to succeed before klp_free() functions > can be safely called. The main motivation was the need to > initialize also the new .kobj_alive flags. > > > Changes against v13: > > + Rename old_addr -> old_func instead of new_func -> new_addr. [Josh] > > + Do not add the helper macros to define structures. [Miroslav, Josh] > > + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] > Aside: I don't suppose that this could ever be folded into the kobject code/data structure itself? This seems like a common problem that kobj-users will need to solve like this. > + Avoid renaming .forced flag to .module_put by calling klp_free > functions only with taken module reference. [Josh] > > + Use list_add_tail() instead of list_add() when updating the dynamic > lists of klp_object and klp_func structures. Note that this > required also updating the order of messages from the pre/post > callbacks in the selftest. [Josh, Miroslav] > Updated self-tests ran fine for me, thanks for updating. > + Do not unnecessarily initialize ret variable in klp_add_nops(). [Miroslav] > > + Got rid of klp_discard_replaced_stuff(). [Josh] > > + Updated commit messages, comments and documentation, especially > the section "Livepatch life-cycle" [Josh, Miroslav] Thank you for adding/revising this part. It was pretty clear and it helped to read this before going through the individual patches. I don't have many code comments as the changes appear to safely and correctly do what the say. (We are at v14 after all :) I mainly compared the text and comments to the implementation and noted typos (marked by substitution s/old/new) and awkward wordings (marked by "re-wording suggestion"). That said, I ack'd each patch as I wouldn't want these to hold up the patchset. Thanks, -- Joe
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On 11/29/2018 04:44 AM, Petr Mladek wrote: > Hi, > > I have an updated present for your mailboxes. > > The atomic replace allows to create cumulative patches. They > are useful when you maintain many livepatches and want to remove > one that is lower on the stack. In addition it is very useful when > more patches touch the same function and there are dependencies > between them. > > All the changes were simple in principle but they required quite > some refactoring again :-( IMHO, the biggest change is renaming > klp_init_lists() ->klp_init_patch_before_free(). It does all > init actions that need to succeed before klp_free() functions > can be safely called. The main motivation was the need to > initialize also the new .kobj_alive flags. > > > Changes against v13: > > + Rename old_addr -> old_func instead of new_func -> new_addr. [Josh] > > + Do not add the helper macros to define structures. [Miroslav, Josh] > > + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] > Aside: I don't suppose that this could ever be folded into the kobject code/data structure itself? This seems like a common problem that kobj-users will need to solve like this. > + Avoid renaming .forced flag to .module_put by calling klp_free > functions only with taken module reference. [Josh] > > + Use list_add_tail() instead of list_add() when updating the dynamic > lists of klp_object and klp_func structures. Note that this > required also updating the order of messages from the pre/post > callbacks in the selftest. [Josh, Miroslav] > Updated self-tests ran fine for me, thanks for updating. > + Do not unnecessarily initialize ret variable in klp_add_nops(). [Miroslav] > > + Got rid of klp_discard_replaced_stuff(). [Josh] > > + Updated commit messages, comments and documentation, especially > the section "Livepatch life-cycle" [Josh, Miroslav] Thank you for adding/revising this part. It was pretty clear and it helped to read this before going through the individual patches. I don't have many code comments as the changes appear to safely and correctly do what the say. (We are at v14 after all :) I mainly compared the text and comments to the implementation and noted typos (marked by substitution s/old/new) and awkward wordings (marked by "re-wording suggestion"). That said, I ack'd each patch as I wouldn't want these to hold up the patchset. Thanks, -- Joe
[PATCH v14 00/11] livepatch: Atomic replace feature
Hi, I have an updated present for your mailboxes. The atomic replace allows to create cumulative patches. They are useful when you maintain many livepatches and want to remove one that is lower on the stack. In addition it is very useful when more patches touch the same function and there are dependencies between them. All the changes were simple in principle but they required quite some refactoring again :-( IMHO, the biggest change is renaming klp_init_lists() ->klp_init_patch_before_free(). It does all init actions that need to succeed before klp_free() functions can be safely called. The main motivation was the need to initialize also the new .kobj_alive flags. Changes against v13: + Rename old_addr -> old_func instead of new_func -> new_addr. [Josh] + Do not add the helper macros to define structures. [Miroslav, Josh] + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] + Avoid renaming .forced flag to .module_put by calling klp_free functions only with taken module reference. [Josh] + Use list_add_tail() instead of list_add() when updating the dynamic lists of klp_object and klp_func structures. Note that this required also updating the order of messages from the pre/post callbacks in the selftest. [Josh, Miroslav] + Do not unnecessarily initialize ret variable in klp_add_nops(). [Miroslav] + Got rid of klp_discard_replaced_stuff(). [Josh] + Updated commit messages, comments and documentation, especially the section "Livepatch life-cycle" [Josh, Miroslav] Changes against v12: + Finish freeing the patch using workqueues to prevent deadlock against kobject code. + Check for valid pointers when initializing the dynamic lists objects and functions. + Mark klp_free_objects_dynamic() static. + Improved documentation and fixed typos Changes against v11: + Functional changes: + Livepatches get automatically unregistered when disabled. Note that the sysfs interface disappears at this point. It simplifies the API and code. The only drawback is that the patch can be enabled again only by reloading the module. + Refuse to load conflicting patches. The same function can be patched again only by a new cumulative patch that replaces all older ones. + Non-conflicting patches can be loaded and disabled in any order. + API related changes: + Change void *new_func -> unsigned long new_addr in struct klp_func. + Several new macros to hide implementation details and avoid casting when defining struct klp-func and klp_object. + Remove obsolete klp_register_patch() klp_unregister_patch() API + Change in selftest against v4: + Use new macros to define struct klp_func and klp_object. + Remove klp_register_patch()/klp_unregister_patch() calls. + Replace load_mod() + wait_for_transition() with three variants load_mod(), load_lp(), load_lp_nowait(). IMHO, it is easier to use because we need to detect the end of transaction another way after disable_lp() now. + Replaced unload_mod() with two variants unload_mod(), unload_lp() to match the above change. + Wait for the end of transition in disable_lp() instead of the unreliable check of the sysfs interface. Note that I did not touch the logs with expected result. They stay exactly the same as in v4 posted by Joe. I hope that it is a good sign ;-) Changes against v10: + Bug fixes and functional changes: + Handle Nops in klp_ftrace_handled() to avoid infinite loop [Mirek] + Really add dynamically allocated klp_object into the list [Petr] + Clear patch->replace when transition finishes [Josh] + Refactoring and clean up [Josh]: + Replace enum types with bools + Avoid using ERR_PTR + Remove too paranoid warnings + Distinguish registered patches by a flag instead of a list + Squash some functions + Update comments, documentation, and commit messages + Squashed and split patches to do more controversial changes later Changes against v9: + Fixed check of valid NOPs for already loaded objects, regression introduced in v9 [Joe, Mirek] + Allow to replace even disabled patches [Evgenii] Changes against v8: + Fixed handling of statically defined struct klp_object with empty array of functions [Joe, Mirek] + Removed redundant func->new_func assignment for NOPs [Mirek] + Improved some wording [Mirek] Changes against v7: + Fixed handling of NOPs for not-yet-loaded modules + Made klp_replaced_patches list static [Mirek] + Made klp_free_object() public later [Mirek] + Fixed several reported typos [Mirek, Joe] + Updated documentation according to the feedback [Joe] + Added some Acks [Mirek] Changes against v6: + used list_move when disabling replaced patches [Jason] + renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek] + used
[PATCH v14 00/11] livepatch: Atomic replace feature
Hi, I have an updated present for your mailboxes. The atomic replace allows to create cumulative patches. They are useful when you maintain many livepatches and want to remove one that is lower on the stack. In addition it is very useful when more patches touch the same function and there are dependencies between them. All the changes were simple in principle but they required quite some refactoring again :-( IMHO, the biggest change is renaming klp_init_lists() ->klp_init_patch_before_free(). It does all init actions that need to succeed before klp_free() functions can be safely called. The main motivation was the need to initialize also the new .kobj_alive flags. Changes against v13: + Rename old_addr -> old_func instead of new_func -> new_addr. [Josh] + Do not add the helper macros to define structures. [Miroslav, Josh] + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] + Avoid renaming .forced flag to .module_put by calling klp_free functions only with taken module reference. [Josh] + Use list_add_tail() instead of list_add() when updating the dynamic lists of klp_object and klp_func structures. Note that this required also updating the order of messages from the pre/post callbacks in the selftest. [Josh, Miroslav] + Do not unnecessarily initialize ret variable in klp_add_nops(). [Miroslav] + Got rid of klp_discard_replaced_stuff(). [Josh] + Updated commit messages, comments and documentation, especially the section "Livepatch life-cycle" [Josh, Miroslav] Changes against v12: + Finish freeing the patch using workqueues to prevent deadlock against kobject code. + Check for valid pointers when initializing the dynamic lists objects and functions. + Mark klp_free_objects_dynamic() static. + Improved documentation and fixed typos Changes against v11: + Functional changes: + Livepatches get automatically unregistered when disabled. Note that the sysfs interface disappears at this point. It simplifies the API and code. The only drawback is that the patch can be enabled again only by reloading the module. + Refuse to load conflicting patches. The same function can be patched again only by a new cumulative patch that replaces all older ones. + Non-conflicting patches can be loaded and disabled in any order. + API related changes: + Change void *new_func -> unsigned long new_addr in struct klp_func. + Several new macros to hide implementation details and avoid casting when defining struct klp-func and klp_object. + Remove obsolete klp_register_patch() klp_unregister_patch() API + Change in selftest against v4: + Use new macros to define struct klp_func and klp_object. + Remove klp_register_patch()/klp_unregister_patch() calls. + Replace load_mod() + wait_for_transition() with three variants load_mod(), load_lp(), load_lp_nowait(). IMHO, it is easier to use because we need to detect the end of transaction another way after disable_lp() now. + Replaced unload_mod() with two variants unload_mod(), unload_lp() to match the above change. + Wait for the end of transition in disable_lp() instead of the unreliable check of the sysfs interface. Note that I did not touch the logs with expected result. They stay exactly the same as in v4 posted by Joe. I hope that it is a good sign ;-) Changes against v10: + Bug fixes and functional changes: + Handle Nops in klp_ftrace_handled() to avoid infinite loop [Mirek] + Really add dynamically allocated klp_object into the list [Petr] + Clear patch->replace when transition finishes [Josh] + Refactoring and clean up [Josh]: + Replace enum types with bools + Avoid using ERR_PTR + Remove too paranoid warnings + Distinguish registered patches by a flag instead of a list + Squash some functions + Update comments, documentation, and commit messages + Squashed and split patches to do more controversial changes later Changes against v9: + Fixed check of valid NOPs for already loaded objects, regression introduced in v9 [Joe, Mirek] + Allow to replace even disabled patches [Evgenii] Changes against v8: + Fixed handling of statically defined struct klp_object with empty array of functions [Joe, Mirek] + Removed redundant func->new_func assignment for NOPs [Mirek] + Improved some wording [Mirek] Changes against v7: + Fixed handling of NOPs for not-yet-loaded modules + Made klp_replaced_patches list static [Mirek] + Made klp_free_object() public later [Mirek] + Fixed several reported typos [Mirek, Joe] + Updated documentation according to the feedback [Joe] + Added some Acks [Mirek] Changes against v6: + used list_move when disabling replaced patches [Jason] + renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek] + used
