Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
On 03/02/2018 06:11 AM, Petr Mladek wrote: > On Tue 2018-02-27 09:58:40, Joe Lawrence wrote: >> In my mind, atomic replace is the mechanism that forces patching to be >> cumulative. Perhaps this is too strict? Are there other use-cases for >> atomic-replace? > > Jason talked about using the atomic replace to get rid of any > existing livepatches and adding another changes instead. The changes > in the old and the new patch might be unrelated. They simply do > not want to mind what was there before. The term "atomic replace" > fits perfectly for this usecase. > > My understanding is that cumulative patches do similar thing. > But the old and new patches should be related. In particular, > any new patch should include most changes from the older one. > The only exception is when an old change was wrong and we do > not want it anymore. Yes, I can see the semantic difference between these cases. In my mind, I am tainted by an understanding of the implementation... so I lazily optimized both cases under a common terminology. That said, you're right about potential confusion, so I'll update the example and docs to remove references to "cumulative" and just call it "atomic-replace" :) > PS: I did not added these patches to v9 of the atomic replace > patchset. It was already big enough. And I hope that v9 might > be final. In addition, there are no conflicts on the touched > files side. I can continue to update as a separate patchset if that helps the the other patchset reach a quicker conclusion. As far as licensing, I don't mind modifying for SPDX tags if that's the way we want to go. Thanks, -- Joe
Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
On Tue 2018-02-27 09:58:40, Joe Lawrence wrote: > On 02/27/2018 07:36 AM, Miroslav Benes wrote: > > On Fri, 23 Feb 2018, Joe Lawrence wrote: > > > >> [ ... snip ... ] > >> > >> +If a livepatch is replaced by a cumulative patch, then only the > >> +callbacks belonging to the cumulative patch will be executed. This > >> +simplifies the livepatching core for it is the responsibility of the > >> +cumulative patch to safely revert whatever needs to be reverted. See > >> +Documentation/livepatch/cumulative.txt for more information on such > >> +patches. > > > > s/cumulative/atomic replace/ almost everywhere? > > > > 'Documentation/livepatch/cumulative.txt' should be > > 'Documentation/livepatch/cumulative-patches.txt' and we may rename it > > atomic-replace-patches.txt. I don't know. Cumulative patches forms a > > subset of atomic replace patches in my understanding. The feature itself > > is more general. Even if practically used for cumulative patches only. But > > it is for you and Petr to decide. > > Hi Miroslav, > > Thanks for reviewing! > > I guess I'm a little confused about the distinction here. > > I understood a "cumulative-patch" to mean that it would contain the sum > of all changes. So instead of this: > > patch 1 = A > + patch 2 = B > + patch 3 = C > --- > net = A + B + C > > We can group all of the changes together into a single cumulative-patch > for the same net effect: > > patch 1 = A -replaced by- > patch 2 = A + B -replaced by- > patch 3 = A + B + C > > I assumed this would also mean to include any reverted changes as well. > So in the example above, if change C needed to be reverted, then: > > patch 4 = A + B > > and that would still be considered a "cumulative-patch". Yes, I would consider this a cumulative patch. > In my mind, atomic replace is the mechanism that forces patching to be > cumulative. Perhaps this is too strict? Are there other use-cases for > atomic-replace? Jason talked about using the atomic replace to get rid of any existing livepatches and adding another changes instead. The changes in the old and the new patch might be unrelated. They simply do not want to mind what was there before. The term "atomic replace" fits perfectly for this usecase. My understanding is that cumulative patches do similar thing. But the old and new patches should be related. In particular, any new patch should include most changes from the older one. The only exception is when an old change was wrong and we do not want it anymore. Now, your examples are close the the Jason's use case. They do: patch1 = A -replaced by- patch2 =B -- result B I mean that one change is replaced by an "unrelated" one. It might confuse people. They might ask why the new patch is called cumulative when all the older changes are lost. I would suggest to rename the sample patch to livepatch-test-replace or so. Also I would try to avoid the world cumulative in the example to avoid confusion. I still would prefer to keep the documentation for the feature called cumulative-patches.txt. From my point of view, the atomic replace is rather a technical detail. It might be dangerous when used for non-related patches. On the other, cumulative patches seem to be a promising way how to keep livepatches maintainable and safe. Best Regards, Petr PS: I did not added these patches to v9 of the atomic replace patchset. It was already big enough. And I hope that v9 might be final. In addition, there are no conflicts on the touched files side. In each case, thanks a lot for these nice examples and for finding the bug.
Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
On Tue, 27 Feb 2018, Joe Lawrence wrote: > On 02/27/2018 07:36 AM, Miroslav Benes wrote: > > On Fri, 23 Feb 2018, Joe Lawrence wrote: > > > >> [ ... snip ... ] > >> > >> +If a livepatch is replaced by a cumulative patch, then only the > >> +callbacks belonging to the cumulative patch will be executed. This > >> +simplifies the livepatching core for it is the responsibility of the > >> +cumulative patch to safely revert whatever needs to be reverted. See > >> +Documentation/livepatch/cumulative.txt for more information on such > >> +patches. > > > > s/cumulative/atomic replace/ almost everywhere? > > > > 'Documentation/livepatch/cumulative.txt' should be > > 'Documentation/livepatch/cumulative-patches.txt' and we may rename it > > atomic-replace-patches.txt. I don't know. Cumulative patches forms a > > subset of atomic replace patches in my understanding. The feature itself > > is more general. Even if practically used for cumulative patches only. But > > it is for you and Petr to decide. > > Hi Miroslav, > > Thanks for reviewing! > > I guess I'm a little confused about the distinction here. > > I understood a "cumulative-patch" to mean that it would contain the sum > of all changes. So instead of this: > > patch 1 = A > + patch 2 = B > + patch 3 = C > --- > net = A + B + C > > We can group all of the changes together into a single cumulative-patch > for the same net effect: > > patch 1 = A -replaced by- > patch 2 = A + B -replaced by- > patch 3 = A + B + C Yes. > I assumed this would also mean to include any reverted changes as well. > So in the example above, if change C needed to be reverted, then: > > patch 4 = A + B > > and that would still be considered a "cumulative-patch". Ah, ok. This is where we differ. I didn't consider this to be a cumulative patch. But I understand your reasoning. Miroslav
Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
On 02/27/2018 07:36 AM, Miroslav Benes wrote: > On Fri, 23 Feb 2018, Joe Lawrence wrote: > >> [ ... snip ... ] >> >> +If a livepatch is replaced by a cumulative patch, then only the >> +callbacks belonging to the cumulative patch will be executed. This >> +simplifies the livepatching core for it is the responsibility of the >> +cumulative patch to safely revert whatever needs to be reverted. See >> +Documentation/livepatch/cumulative.txt for more information on such >> +patches. > > s/cumulative/atomic replace/ almost everywhere? > > 'Documentation/livepatch/cumulative.txt' should be > 'Documentation/livepatch/cumulative-patches.txt' and we may rename it > atomic-replace-patches.txt. I don't know. Cumulative patches forms a > subset of atomic replace patches in my understanding. The feature itself > is more general. Even if practically used for cumulative patches only. But > it is for you and Petr to decide. Hi Miroslav, Thanks for reviewing! I guess I'm a little confused about the distinction here. I understood a "cumulative-patch" to mean that it would contain the sum of all changes. So instead of this: patch 1 = A + patch 2 = B + patch 3 = C --- net = A + B + C We can group all of the changes together into a single cumulative-patch for the same net effect: patch 1 = A -replaced by- patch 2 = A + B -replaced by- patch 3 = A + B + C I assumed this would also mean to include any reverted changes as well. So in the example above, if change C needed to be reverted, then: patch 4 = A + B and that would still be considered a "cumulative-patch". In my mind, atomic replace is the mechanism that forces patching to be cumulative. Perhaps this is too strict? Are there other use-cases for atomic-replace? >> Example Use-cases >> = >> >> [ ... snip ... ] >> >> +Test 11 >> +--- >> + >> +A similar test as the previous one, except this time load the second >> +callback demo module as a cumulative (ie, replacement) patch. The >> +livepatching core will only execute klp_object callbacks for the latest >> +cumulative patch on the patch stack. >> + >> +- load livepatch >> +- load second livepatch (atomic replace) >> +- disable livepatch > > Not needed. Good catch. -- Joe
Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
On Fri, 23 Feb 2018, Joe Lawrence wrote: > Update livepatch callback documentation and samples with respect to new > atomic replace / cumulative patch functionality. > > Signed-off-by: Joe Lawrence > --- > Documentation/livepatch/callbacks.txt | 102 > samples/livepatch/Makefile| 1 + > samples/livepatch/livepatch-callbacks-demo2.c | 162 > ++ > 3 files changed, 265 insertions(+) > create mode 100644 samples/livepatch/livepatch-callbacks-demo2.c > > diff --git a/Documentation/livepatch/callbacks.txt > b/Documentation/livepatch/callbacks.txt > index c9776f48e458..b5e67975c5a9 100644 > --- a/Documentation/livepatch/callbacks.txt > +++ b/Documentation/livepatch/callbacks.txt > @@ -86,6 +86,13 @@ If the object did successfully patch, but the patch > transition never > started for some reason (e.g., if another object failed to patch), > only the post-unpatch callback will be called. > > +If a livepatch is replaced by a cumulative patch, then only the > +callbacks belonging to the cumulative patch will be executed. This > +simplifies the livepatching core for it is the responsibility of the > +cumulative patch to safely revert whatever needs to be reverted. See > +Documentation/livepatch/cumulative.txt for more information on such > +patches. s/cumulative/atomic replace/ almost everywhere? 'Documentation/livepatch/cumulative.txt' should be 'Documentation/livepatch/cumulative-patches.txt' and we may rename it atomic-replace-patches.txt. I don't know. Cumulative patches forms a subset of atomic replace patches in my understanding. The feature itself is more general. Even if practically used for cumulative patches only. But it is for you and Petr to decide. > Example Use-cases > = > @@ -603,3 +610,98 @@ pre-unpatch callbacks are skipped: >% rmmod samples/livepatch/livepatch-callbacks-busymod.ko >[ 141.279111] livepatch_callbacks_busymod: busymod_work_func exit >[ 141.279760] livepatch_callbacks_busymod: livepatch_callbacks_mod_exit > + > + > +Test 10 > +--- > + > +Test loading multiple livepatch modules containing callback routines. > +The livepatching core executes callbacks for all modules. > + > +- load livepatch > +- load second livepatch > +- disable livepatch > +- disable second livepatch > +- unload livepatch > +- unload second livepatch > + > + % insmod samples/livepatch/livepatch-callbacks-demo.ko > + [ 216.448208] livepatch: enabling patch 'livepatch_callbacks_demo' > + [ 216.448211] livepatch: 'livepatch_callbacks_demo': initializing > patching transition > + [ 216.448330] livepatch_callbacks_demo: pre_patch_callback: vmlinux > + [ 216.448341] livepatch: 'livepatch_callbacks_demo': starting patching > transition > + [ 218.720099] livepatch: 'livepatch_callbacks_demo': completing patching > transition > + [ 218.720179] livepatch_callbacks_demo: post_patch_callback: vmlinux > + [ 218.720180] livepatch: 'livepatch_callbacks_demo': patching complete > + > + % insmod samples/livepatch/livepatch-callbacks-demo2.ko > + [ 220.126552] livepatch: enabling patch 'livepatch_callbacks_demo2' > + [ 220.126554] livepatch: 'livepatch_callbacks_demo2': initializing > patching transition > + [ 220.126592] livepatch_callbacks_demo2: pre_patch_callback: vmlinux > + [ 220.126593] livepatch: 'livepatch_callbacks_demo2': starting patching > transition > + [ 221.728091] livepatch: 'livepatch_callbacks_demo2': completing patching > transition > + [ 221.728254] livepatch_callbacks_demo2: post_patch_callback: vmlinux > + [ 221.728255] livepatch: 'livepatch_callbacks_demo2': patching complete > + > + % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled > + [ 223.434556] livepatch: 'livepatch_callbacks_demo2': initializing > unpatching transition > + [ 223.434616] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux > + [ 223.434617] livepatch: 'livepatch_callbacks_demo2': starting unpatching > transition > + [ 224.736159] livepatch: 'livepatch_callbacks_demo2': completing > unpatching transition > + [ 224.736660] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux > + [ 224.736662] livepatch: 'livepatch_callbacks_demo2': unpatching complete > + > + % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled > + [ 227.284070] livepatch: 'livepatch_callbacks_demo': initializing > unpatching transition > + [ 227.284111] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux > + [ 227.284112] livepatch: 'livepatch_callbacks_demo': starting unpatching > transition > + [ 228.704142] livepatch: 'livepatch_callbacks_demo': completing > unpatching transition > + [ 228.704215] livepatch_callbacks_demo: post_unpatch_callback: vmlinux > + [ 228.704216] livepatch: 'livepatch_callbacks_demo': unpatching complete > + > + % rmmod samples/livepatch/livepatch-callbacks-demo2.ko > + % rmmod samples/livepatch/livepatch-callb
