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@xxxxxxxx>
> Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
> ---
> 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
