Re: [PATCH 4/4] livepatch/shadow: Add garbage collection of shadow variables

[Petr Mladek <pmladek@suse.com>]

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@suse.com>

Best Regards,
Petr