On Tue, 17 Oct 2017, Miroslav Benes wrote: > On Tue, 10 Oct 2017, Jason Baron wrote: > > > > > > > On 10/06/2017 06:32 PM, Josh Poimboeuf wrote: > > > On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote: > > >> Since 'atomic replace' has completely replaced all previous livepatch > > >> modules, it explicitly disables all previous livepatch modules. However, > > >> previous livepatch modules that have been replaced, can be re-enabled > > >> if they have the 'replace' flag set. In this case the set of 'nop' > > >> functions is re-calculated, such that it replaces all others. > > >> > > >> For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set > > >> then: > > >> > > >> # insmod kpatch-a.ko > > >> # insmod kpatch-b.ko > > >> > > >> At this point we have: > > >> > > >> # cat /sys/kernel/livepatch/kpatch-a/enabled > > >> 0 > > >> > > >> # cat /sys/kernel/livepatch/kpatch-b/enabled > > >> 1 > > >> > > >> To revert to the kpatch-a state we can do: > > >> > > >> echo 1 > /sys/kernel/livepatch/kpatch-a/enabled > > >> > > >> And now we have: > > >> > > >> # cat /sys/kernel/livepatch/kpatch-a/enabled > > >> 1 > > >> > > >> # cat /sys/kernel/livepatch/kpatch-b/enabled > > >> 0 > > > > > > I don't really like allowing a previously replaced patch to replace the > > > current patch. It's just more unnecessary complexity. If the user > > > wants to atomically revert back to kpatch-a, they should be able to: > > > > > > rmmod kpatch-a > > > insmod kpatch-a.ko > > > > > I agree. > > > Right - that's how I sent v1 (using rmmod/insmod to revert), but it > > didn't account for the fact the patch or some functions may be marked > > 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in > > some cases 'rmmod' was not feasible, I thought it would be simpler from > > an operational pov to just say we always revert by re-enabling a > > previously replaced patch as opposed to rmmod/insmod. > > > > > > >> Note that it may be possible to unload (rmmod) replaced patches in some > > >> cases based on the consistency model, when we know that all the functions > > >> that are contained in the patch may no longer be in used, however its > > >> left as future work, if this functionality is desired. > > > > > > If you don't allow a previously replaced patch to be enabled again, I > > > think it would be trivial to let it be unloaded. > > > > > > > The concern is around being replaced by 'immediate' functions and thus > > not knowing if the code is still in use. > > Hm. Would it make sense to remove immediate and rely only on the > consistency model? At least for the architectures where the model is > implemented (x86_64)? > > If not, then I'd keep such modules there without a possibility to remove > them ever. If its functionality was required again, it would of course > mean to insmod a new module with it. Petr pointed out (offline) that immediate could still be useful. So let me describe how I envision the whole atomic replace semantics. Let's have three functions - a, b, c. Patch 1 is immediate and patches a(). func a b c patches 1i Patch 2 is not immediate and patches b() func a b c patches 1i 2 Patch 3 is atomic replace, which patches a() and c(). func a b c patches 1i 2 3r 3r With 3 applied, 3versions of a() and c() are called, original b() is called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2 cannot be reenabled. 3 can be disabled. Original a() and c() will be called in such case. If one wants to have a() from patch 1 there, he would need to apply patch 4. func a b c patches 1i 2 3r 3r 4i Does it make sense? This is what I would expect and I think it is easier to implement. Whole initialization of nop functions could be done in klp_init_ functions, if I am not mistaken. Thoughts? Miroslav -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
