Re: [PATCH v2 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Josh Poimboeuf <jpoimboe@kernel.org>]

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