On Thu 2022-08-25 10:54:18, Joe Lawrence wrote:
> On 8/25/22 10:50 AM, Joe Lawrence wrote:
> > On 7/1/22 3:48 PM, Marcos Paulo de Souza wrote:
> >> The shadow variable type will be used in klp_shadow_alloc/get/free
> >> functions instead of id/ctor/dtor parameters. As a result, all callers
> >> use the same callbacks consistently[*][**].
> >>
> >> The structure will be used in the next patch that will manage the
> >> lifetime of shadow variables and execute garbage collection automatically.
> >>
> >> [*] From the user POV, it might have been easier to pass $id instead
> >> of pointer to struct klp_shadow_type.
> >>
> >> The problem is that each livepatch registers its own struct
> >> klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
> >> be unclear what callback should be used. They should be compatible.
> >>
> >> This problem is gone when each livepatch explicitly uses its
> >> own struct klp_shadow_type pointing to its own callbacks.
> >>
> >> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
> >> The message must be disabled when called via klp_shadow_free_all()
> >> because the ordering of freed variables is not well defined there.
> >> It has to be done using another hack after switching to
> >> klp_shadow_types.
> >>
> >
> > Is the ordering problem new to this patchset? Shadow variables are
> > still saved in klp_shadow_hash and I think the only change in this patch
> > is that we need to compare through shadow_type and not id directly. Or
> > does patch 4/4 change behavior here? Just curious, otherwise this patch
> > is pretty straightforward.
> >
> >> --- a/include/linux/livepatch.h
> >> +++ b/include/linux/livepatch.h
> >> @@ -216,15 +216,26 @@ typedef int (*klp_shadow_ctor_t)(void *obj,
> >> void *ctor_data);
> >> typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
> >>
> >> -void *klp_shadow_get(void *obj, unsigned long id);
> >> -void *klp_shadow_alloc(void *obj, unsigned long id,
> >> - size_t size, gfp_t gfp_flags,
> >> - klp_shadow_ctor_t ctor, void *ctor_data);
> >> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> >> - size_t size, gfp_t gfp_flags,
> >> - klp_shadow_ctor_t ctor, void *ctor_data);
> >> -void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> >> -void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
> >> +/**
> >> + * struct klp_shadow_type - shadow variable type used by the klp_object
> >> + * @id: shadow variable type indentifier
> >> + * @ctor: custom constructor to initialize the shadow data (optional)
> >> + * @dtor: custom callback that can be used to unregister the variable
> >> + * and/or free data that the shadow variable points to (optional)
> >> + */
> >> +struct klp_shadow_type {
> >> + unsigned long id;
> >> + klp_shadow_ctor_t ctor;
> >> + klp_shadow_dtor_t dtor;
> >> +};
> >> +
> >> +void *klp_shadow_get(void *obj, struct klp_shadow_type *shadow_type);
> >> +void *klp_shadow_alloc(void *obj, struct klp_shadow_type *shadow_type,
> >> + size_t size, gfp_t gfp_flags, void *ctor_data);
> >> +void *klp_shadow_get_or_alloc(void *obj, struct klp_shadow_type *shadow_type,
> >> + size_t size, gfp_t gfp_flags, void *ctor_data);
> >> +void klp_shadow_free(void *obj, struct klp_shadow_type *shadow_type);
> >> +void klp_shadow_free_all(struct klp_shadow_type *shadow_type);
> >>
> >> struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
> >> struct klp_state *klp_get_prev_state(unsigned long id);
> >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> >> index 79b8646b1d4c..9dcbb626046e 100644
> >> --- a/kernel/livepatch/shadow.c
> >> +++ b/kernel/livepatch/shadow.c
> >> @@ -63,24 +63,24 @@ struct klp_shadow {
> >> * klp_shadow_match() - verify a shadow variable matches given <obj, id>
> >> * @shadow: shadow variable to match
> >> * @obj: pointer to parent object
> >> - * @id: data identifier
> >> + * @shadow_type: type of the wanted shadow variable
> >> *
> >> * Return: true if the shadow variable matches.
> >> */
> >> static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> >> - unsigned long id)
> >> + struct klp_shadow_type *shadow_type)
> >> {
> >> - return shadow->obj == obj && shadow->id == id;
> >> + return shadow->obj == obj && shadow->id == shadow_type->id;
> >
> > Not sure if I'm being paranoid, but is there any problem if the user
> > registers two klp_shadow_types with the same id? I can't find any
> > obvious logic problems with that, but I don't think the API prevents
> > this confusing possibility.
Great question!
> Ah n/m, I think I see now that I'm reading patch 4/4, it's
> klp_shadow_type_get_reg() is going to look for an existing
> shadow_type_reg->id first.
The purpose of klp_shadow_type_get_reg() is a bit different.
It decides whether the given klp_shadow_type is registered
for the first time or we just need to increase the refcount.
It means that it is actually possible to register two
different klp_shadow_types using the same ID.
Well, I am not sure if we could do better. We could not compare
pointers of @ctor and @dtor callbacks. The very same klp_shadow_type
can be registered from more livepatches and each livepatch need
to have its own implementation of @ctor and @dtor. It is the purpose
of the refcounting.
In each case, it does not make things worse. The previous API
allowed to combine any ID with any @ctor or @dtor.
One idea. We could define the type by "name" and "id". The @name
might be created by stringification of the structure that defines
the klp_shadow_type. It might be checked during registration
and unregistration. But I am not sure if it is worth the effort.
Best Regards,
Petr
