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.
> >
>
> 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?
The klp_shadow_*alloc() and klp_shdow_free*() APIs pass
struct *klp_shadow_type instead of the ID. The structure is
defined in each livepatch that is using this shadow variable.
As a result, each livepatch code is using @ctor/@dtor defined
in its own livepatch module. It actually behaves the same as before.
I though about storing pointers to @ctors/@dtors of all
registered klp_shadow_types from all livepatches. But it
would hard to choose which one should be used.
Note that some implementation might disappear when the livepatch
is replaced and the module removed.
So, it is easier when each caller passes its own structure
with its own implementation. They should be compatible anyway.
>
> > 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.
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.
> 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.
I think that I would need to get more familiar with kPatch...
> 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.
I think that we do this as well. AFAIK, we are going to associate them
with the klp_object for "vmlinux".
Best Regards,
Petr
