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

[Joe Lawrence <joe.lawrence@redhat.com>]

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@suse.com>
>> Signed-off-by: Petr Mladek <pmladek@suse.com>
>> ---
>>  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