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

[Petr Mladek]

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?


> Instead of reference counting, the livepatch atomic replace code could
> just migrate any previously owned shadow variables to the new klp_patch.

I see. We would not need to explicitly register the shadow type using
klp_shadow_register() and allocate struct klp_shadow_type_reg
for the reference counter.


> This of course adds the restriction that such garbage-collected shadow
> variables couldn't be shared across non-replace livepatches.  But I
> wouldn't expect that to be much of a problem.

>From my POV, this is similar to "func_stack" in struct "klp_ops".
The list was originally designed for non-replace livepatches because
the same function might be livepatched many times.

It might theoretically get "simplified" when only the atomic replace is
supported. Then there would be the maximum of two livepatches
and the (infinite) list would be an overhead.

I say theoretically because the list is actually quite elegant
solution.


> Additionally, I was wondering if "which klp_patch owns which shadow
> variables" could be auto-detected somehow.
> 
> For example:
> 
> 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.

> 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.

Best Regards,
Petr