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

[Petr Mladek <pmladek@suse.com>]

On Mon 2022-10-24 17:09:08, Petr Mladek wrote:
> On Thu 2022-08-25 12:26:25, Joe Lawrence wrote:
> > On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> > > The life of shadow variables is not completely trivial to maintain.
> > > They might be used by more livepatches and more livepatched objects.
> > > They should stay as long as there is any user.
> > > 
> > > In practice, it requires to implement reference counting in callbacks
> > > of all users. They should register all the user and remove the shadow
> > > variables only when there is no user left.
> > > 
> > > This patch hides the reference counting into the klp_shadow API.
> > > The counter is connected with the shadow variable @id. It requires
> > > an API to take and release the reference. The release function also
> > > calls the related dtor() when defined.
> > > 
> > > An easy solution would be to add some get_ref()/put_ref() API.
> > > But it would need to get called from pre()/post_un() callbacks.
> > > It might be easy to forget a callback and make it wrong.
> > > 
> > > A more safe approach is to associate the klp_shadow_type with
> > > klp_objects that use the shadow variables. The livepatch core
> > > code might then handle the reference counters on background.
> > > 
> > > The shadow variable type might then be added into a new @shadow_types
> > > member of struct klp_object. They will get then automatically registered
> > > and unregistered when the object is being livepatched. The registration
> > > increments the reference count. Unregistration decreases the reference
> > > count. All shadow variables of the given type are freed when the reference
> > > count reaches zero.
> > > 
> > > All klp_shadow_alloc/get/free functions also checks whether the requested
> > > type is registered. It will help to catch missing registration and might
> > > also help to catch eventual races.
> > > 
> > > --- a/include/linux/livepatch.h
> > > +++ b/include/linux/livepatch.h
> > > @@ -100,11 +100,14 @@ struct klp_callbacks {
> > >  	bool post_unpatch_enabled;
> > >  };
> > >  
> > > +struct klp_shadow_type;
> > > +
> > >  /**
> > >   * struct klp_object - kernel object structure for live patching
> > >   * @name:	module name (or NULL for vmlinux)
> > >   * @funcs:	function entries for functions to be patched in the object
> > >   * @callbacks:	functions to be executed pre/post (un)patching
> > > + * @shadow_types: shadow variable types used by the livepatch for the klp_object
> > >   * @kobj:	kobject for sysfs resources
> > >   * @func_list:	dynamic list of the function entries
> > >   * @node:	list node for klp_patch obj_list
> > > @@ -118,6 +121,7 @@ struct klp_object {
> > >  	const char *name;
> > >  	struct klp_func *funcs;
> > >  	struct klp_callbacks callbacks;
> > > +	struct klp_shadow_type **shadow_types;
> > >  
> > 
> > Hmm.  The implementation of shadow_types inside klp_object might be
> > difficult to integrate into kpatch-build.  For kpatches, we do utilize
> > the kernel's shadow variable API directly, but at kpatch author time we
> > don't have any of klp_patch objects in hand -- those are generated by
> > template after the binary before/after comparison is completed.
> 
> I am sorry but I am not much familiar with kPatch. But I am surprised.
> It should be similar to klp_callbacks. If it was possible to define
> struct klp_callbacks for a particular struct klp_object then it
> should be possible to define struct klp_shadow_types ** similar way.

Note that adding the used klp_shadow_types into struct klp_object
is not strictly required.

Alternative solution is to register/unregister the used types using
klp_callbacks or module init()/exit() callbacks. This approach
is used in lib/livepatch/test_klp_shadow_vars.c.

I believe that this would be usable for kpatch-build.
You needed to remove not-longer used shadow variables
using these callbacks even before this patchset. I would
consider it a bug if you did not remove them. The new API
just allows to do this a safe way.

Best Regards,
Petr