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

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Tue, Nov 08, 2022 at 10:14:18AM +0100, Petr Mladek wrote:
> On Mon 2022-11-07 17:32:09, Josh Poimboeuf wrote:
> > On Fri, Nov 04, 2022 at 11:25:38AM +0100, Petr Mladek wrote:
> > > > I get the feeling the latter would be easier to implement (no reference
> > > > counting; also maybe can be auto-detected with THIS_MODULE?) and harder
> > > > for the patch author to mess up (by accidentally omitting an object
> > > > which uses it).
> > > 
> > > I am not sure how you mean it. I guess that you suggest to store
> > > the name of the livepatch module into the shadow variable.
> > > And use the variable only when the livepatch module is still loaded.
> > 
> > Actually I was thinking the klp_patch could have references to all the
> > shadow variables (or shadow variable types?) it owns.
> 
> In short, you suggest to move the array of used klp_shadow_types from
> struct klp_object to struct klp_patch. Do I get it correctly?

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.

> > 1) add 'struct module *owner' or 'struct klp_patch *owner' to klp_shadow
> > 
> > 2) add klp_shadow_alloc_gc() and klp_shadow_get_or_alloc_gc(), which are
> >    similar to their non-gc counterparts, with a few additional
> >    arguments: the klp module owner (THIS_MODULE for the caller); and a
> >    destructor to be used later for the garbage collection
> > 
> > 3) When atomic replacing a patch, iterate through the klp_shadow_hash
> >    and, for each klp_shadow which previously had an owner, change it to
> >    be owned by the new patch
> 
> This is not clear to me. The new livepatch might also use less shadow
> variables. It must not blindly take over all shadow variables which
> were owned by the previous livepatch.

Assuming atomic replace, the new patch is almost always a superset of
the old patch.  We can optimize for that case.

If the new patch needs to remove any old shadow variables, it can do so
in its post-patch callback.

> > 4) When unloading/freeing a patch, free all its associated klp_shadows
> >    (also calling destructors where applicable)
> > 
> > 
> > I'm thinking this would be easier for the patch author, and also simpler
> > overall.  I could work up a patch.
> 
> From the patch author POV:
> 
> If the autodetection did not work then the patch author would still
> need to provide the array of used shadow types. I agree that only
> one array in struct klp_patch might be enough.
> 
> 
> From the implementation POV:
> 
> I agree that the code might be easier if we support only atomic
> replace. We would not need the reference counter in this case.
> 
> But I am not sure if this is acceptable for users that do not use
> the atomic replace. They suffer from the same problem. Do we really
> want to make this mode a 2nd citizen? IMHO, all applicable features
> have been implemented for both modes so far.

Non-replace patches would still be supported.  Just with the restriction
that garbage-collected shadow variables are by definition owned by a
single patch module and thus can't be shared across patch modules.

-- 
Josh