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.
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
> ---
> include/linux/livepatch.h | 29 +++--
> kernel/livepatch/shadow.c | 103 ++++++++---------
> lib/livepatch/test_klp_shadow_vars.c | 105 ++++++++++--------
> samples/livepatch/livepatch-shadow-fix1.c | 18 ++-
> samples/livepatch/livepatch-shadow-fix2.c | 27 +++--
> .../selftests/livepatch/test-shadow-vars.sh | 2 +-
> 6 files changed, 163 insertions(+), 121 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 293e29960c6e..79e7bf3b35f6 100644
> --- 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.
> [ ... snip ... ]
>
Regards,
--
Joe
