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

[Joe Lawrence <joe.lawrence@redhat.com>]

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

If I understand the patchset correctly, the last patch consolidated
id/ctor/dtor into klp_shadow_type structure, then this one formally
associates the new structure with a klp_object so that it bumps up /
down a simple reference count automatically.  That count is now checked
before calling the dtor/removal of any matching shadow variable.  So far
so good.

How does this play out for the following scenario:

- atomic replace livepatches, accumulative versions
- livepatch v1 : registers a klp_shadow_type (id=1), creates a few
shadow vars
- livepatch v2 : also uses klp_shadow_type (id=1) and creates a few
shadow vars

Since the first livepatch registered the klp_shadow_type, its ctor/dtor
pair will be used for all id=1 shadow variables, including those created
by the subsequent livepatches, right?


> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/livepatch.h                 |  21 ++++
>  kernel/livepatch/core.c                   |  39 +++++++
>  kernel/livepatch/core.h                   |   1 +
>  kernel/livepatch/shadow.c                 | 124 ++++++++++++++++++++++
>  kernel/livepatch/transition.c             |   4 +-
>  lib/livepatch/test_klp_shadow_vars.c      |  18 +++-
>  samples/livepatch/livepatch-shadow-fix1.c |   8 +-
>  samples/livepatch/livepatch-shadow-fix2.c |   9 +-
>  8 files changed, 214 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 79e7bf3b35f6..cb65de831684 100644
> --- 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.

My first thought is that kpatch-build might associate any shadow
variables in foo.c with its parent object (vmlinux, foo.ko, etc.),
however that may not always be 100% accurate.

In fact, we have occasionally used shadow variables with <id, 0> to
create one off variables not associated with specific code or data
structures, but rather the presence of livepatch.  This is (too) briefly
mentioned in "Other use-cases" section of the shadow variable Documentation.

-- 
Joe