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

Josh Poimboeuf
2022-11-08T18:44:16Z

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