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.
>
>> 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.
>
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.
--
Joe
