On Fri 2022-07-01 16:48:17, 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
s/all the user/all users/
> 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.
>
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -222,13 +226,30 @@ typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
> * @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)
> + * @registered: flag indicating if the variable was successfully registered
> + *
> + * All shadow variables used by the livepatch for the related klp_object
> + * must be listed here so that they are registered when the livepatch
> + * and the module is loaded. Otherwise, it will not be possible to
> + * allocated them.
s/allocated/allocate/
> */
> struct klp_shadow_type {
> unsigned long id;
> klp_shadow_ctor_t ctor;
> klp_shadow_dtor_t dtor;
> +
> + /* internal */
> + bool registered;
> };
>
> +#define klp_for_each_shadow_type(obj, shadow_type, i) \
> + for (shadow_type = obj->shadow_types ? obj->shadow_types[0] : NULL, i = 1; \
> + shadow_type; \
> + shadow_type = obj->shadow_types[i++])
> +
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -307,3 +331,103 @@ void klp_shadow_free_all(struct klp_shadow_type *shadow_type)
> spin_unlock_irqrestore(&klp_shadow_lock, flags);
> }
> EXPORT_SYMBOL_GPL(klp_shadow_free_all);
> +
> +static struct klp_shadow_type_reg *
> +klp_shadow_type_get_reg(struct klp_shadow_type *shadow_type)
> +{
> + struct klp_shadow_type_reg *shadow_type_reg;
> + lockdep_assert_held(&klp_shadow_lock);
> +
> + list_for_each_entry(shadow_type_reg, &klp_shadow_types, list) {
> + if (shadow_type_reg->id == shadow_type->id)
> + return shadow_type_reg;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * klp_shadow_register() - register self for using a given data identifier
This is a relic from a former version. It should be something like:
* klp_shadow_register() - register the given shadow variable type
> + * @shadow_type: shadow type to be registered
> + *
> + * Tell the system that the related module (livepatch) is going to use a given
> + * shadow variable ID. It allows to check and maintain lifetime of shadow
> + * variables.
Same here. It should be:
* Tell the system that the given shadow variable ID is going to be used by
* the caller (livepatch module). It allows to check and maintain the lifetime
* of shadow variables.
> + *
> + * Return: 0 on suceess, -ENOMEM when there is not enough memory.
> + */
> +int klp_shadow_register(struct klp_shadow_type *shadow_type)
> +{
> + struct klp_shadow_type_reg *shadow_type_reg;
> + struct klp_shadow_type_reg *new_shadow_type_reg;
> +
The code looks good to me. With the above changes:
Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
Best Regards,
Petr
