On 11/1/22 7:02 AM, Petr Mladek wrote:
> On Mon 2022-10-24 17:09:08, Petr Mladek wrote:
>> On Thu 2022-08-25 12:26:25, Joe Lawrence wrote:
>>> On 7/1/22 3:48 PM, 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
>>>> 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
>>>> @@ -100,11 +100,14 @@ struct klp_callbacks {
>>>> bool post_unpatch_enabled;
>>>> };
>>>>
>>>> +struct klp_shadow_type;
>>>> +
>>>> /**
>>>> * struct klp_object - kernel object structure for live patching
>>>> * @name: module name (or NULL for vmlinux)
>>>> * @funcs: function entries for functions to be patched in the object
>>>> * @callbacks: functions to be executed pre/post (un)patching
>>>> + * @shadow_types: shadow variable types used by the livepatch for the klp_object
>>>> * @kobj: kobject for sysfs resources
>>>> * @func_list: dynamic list of the function entries
>>>> * @node: list node for klp_patch obj_list
>>>> @@ -118,6 +121,7 @@ struct klp_object {
>>>> const char *name;
>>>> struct klp_func *funcs;
>>>> struct klp_callbacks callbacks;
>>>> + struct klp_shadow_type **shadow_types;
>>>>
>>>
>>> Hmm. The implementation of shadow_types inside klp_object might be
>>> difficult to integrate into kpatch-build. For kpatches, we do utilize
>>> the kernel's shadow variable API directly, but at kpatch author time we
>>> don't have any of klp_patch objects in hand -- those are generated by
>>> template after the binary before/after comparison is completed.
>>
>> I am sorry but I am not much familiar with kPatch. But I am surprised.
>> It should be similar to klp_callbacks. If it was possible to define
>> struct klp_callbacks for a particular struct klp_object then it
>> should be possible to define struct klp_shadow_types ** similar way.
Right. For klp_callbacks, the kpatch author would logically provide the
callback code right in the compilation unit for said klp_object.
Additionally, kpatch uses callback macros to provide hints in the ELF
file so that kpatch-build can collect up the callbacks for each
klp_object to plug its the livepatch template.
In the case of shadow variables, we might use a similar hint mechanism.
Also, we could implement additional sanity checking to detect if the
kpatch author is trying to access klp_object A's shadow_types from
klp_object B. If there is a legit use case for that, then I would think
the klp_object better be vmlinux, else the kpatch may introduce
use-after-frees.
> Note that adding the used klp_shadow_types into struct klp_object
> is not strictly required.
>
> Alternative solution is to register/unregister the used types using
> klp_callbacks or module init()/exit() callbacks. This approach
> is used in lib/livepatch/test_klp_shadow_vars.c.
>
> I believe that this would be usable for kpatch-build.
> You needed to remove not-longer used shadow variables
> using these callbacks even before this patchset. I would
> consider it a bug if you did not remove them. The new API
> just allows to do this a safe way.
>
Ah, let me dig into that example for alternative usage. At first
glance, it looks like you're right -- kpatch already supports callbacks,
so just (un)register the shadow variables here. I'll be back with more
info hopefully later this week.
Thanks,
--
Joe
