On Wed, Nov 09, 2022 at 03:36:23PM +0100, Petr Mladek wrote: > > Right. Though, thinking about it more, this isn't even needed. Each > > klp_shadow would have a pointer to its owning module. We already have a > > global hash of klp_shadows which can be iterated when the module gets > > unloaded or replaced. > > > > Assuming atomic replace, the new patch is almost always a superset of > > the old patch. We can optimize for that case. > > I see. But I do not agree with this assumption. The new livepatch > might also remove code that caused problems. > > Also we allow downgrades. I mean that there is no versioning > of livepatches. Older livepatches might replace new ones. > > We really want to support downgrades or upgrades that remove > problematic code. Or upgrades that start fixing the problem > a better way using another shadow variable. So I've been thinking about this too... I think it's good to support downgrades. But even with this patch set, they still aren't properly supported because of the callback design. Unpatch callbacks aren't called for the replaced patch. Any cleanup it needs to do (e.g., for a downgrade) is skipped. That can cause all kinds of problems including leaks and conflicts with future patches. And actually, that seems directly related to shadow variable garbage collection. Typically you would call 'klp_shadow_free_all(id, dtor)' in the post_unpatch callback, *unless* that data is going to be used by the replacement patch. That "unless" is the problem. Without that problem, garbage collection of shadow variables would be trivial. Thing is, that "unless" exists not only for the freeing of shadow variables, but also for the allocation of some shadow variables in pre/post-patch callbacks, and many other types of initializations and cleanups done by patch/unpatch callbacks. Correct me if I'm wrong, but it seems like downgrades (and similar scenarios like upgrades with partial revert) are the primary real-world motivation for this patch set. These patches feel like a partial solution to a bigger problem. If we really want to support downgrade use cases, the callbacks will need to be improved. The main issue is that callbacks aren't called at the right times, and they're not granular enough to support different levels of upgrade/downgrade so that we can arbitrarily replace any patch with any other version of the patch without consequence. If the callbacks were split up and called only when they're needed, it would be trivial to free the shadow variables in a post_unpatch callback. To support this we could have some kind of callback versioning, where a klp_object can have an array of klp_callbacks, and each set of callbacks has a version associated with it. For example, consider a sequence of patches P1-P4, each of which is an upgrade from the last. (Note: I'm assuming only one klp_object here to keep it simple.) - P1 has [ callbacks.version=1 ] - P2 has [ callbacks.version=1 ] - P3 has [ callbacks.version=1, callbacks.version=2 ] - P4 has [ callbacks.version=1, callbacks.version=3 ] Usually a patch will have the same callbacks (with same versions) as its predecessor. P1 and P2 have the same callbacks (v1). If needed, a patch can then add an additional set of callbacks (with bumped version) like P3. Or it can remove its predecessor's callbacks. Or it can do both, like P4. When deciding which callbacks to call for a replace operation, it's basically an XOR. If a callback version exists in one patch but not the other, its callbacks should be called. For example, if P3 gets upgraded to P4, call P4's v3 patch callbacks and P3's v2 unpatch callbacks. If P4 gets downgraded to P1, call P4's v3 unpatch callbacks. BTW, "version" may not be the right name, as there's no concept of newer or older: any patch can be arbitrarily replaced by any other patch. The version is just a unique ID for a set of callbacks. -- Josh
