Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Petr Mladek <pmladek@suse.com>]

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