Re: [PATCH] livepatch: Allow to handle lifetime of shadow variables using the livepatch state

> Will this callback run in an atomic context, unlike the other state callbacks?

> All existing state callbacks execute in standard process context and are
> permitted to sleep. However, looking at klp_shadow_free_all(), it invokes
> the destructor while holding klp_shadow_lock:

> kernel/livepatch/shadow.c:klp_shadow_free_all() {
> 	 ...
>	 spin_lock_irqsave(&klp_shadow_lock, flags);
>
>	 hash_for_each(klp_shadow_hash, i, shadow, node) {
>	 	if (klp_shadow_match(shadow, shadow->obj, id))
>	 		klp_shadow_free_struct(shadow, dtor);
>	 }
>
>	 spin_unlock_irqrestore(&klp_shadow_lock, flags);
> }

> If a user assumes this callback runs in process context like the other
> state callbacks and includes sleeping operations, couldn't this trigger
> a scheduling while atomic panic?

This problem wasn't introduced by this change; it already existed beforehand.
There shouldn't be any sleeping operations in klp_shadow_dtor_t. Maybe we
should rename it to klp_shadow_atomic_dtor_t.

[...]

> Does tying shadow variable lifetime to the global patch state cause leaks when
> dealing with loadable modules?

> If a livepatch applies to a loadable module, it may allocate shadow variables
> keyed to the module's dynamically allocated objects.

> When the target module is unloaded, klp_module_going() unpatches the module's
> objects. However, because the state is a global property of the patch, no
> state callbacks are invoked and the state is not considered obsolete. This
> leaves the shadow variables for the unloaded module orphaned in the global
> hash table.

> If the module is subsequently reloaded and allocates new objects at the same
> memory addresses, wouldn't klp_shadow_alloc() fail with a duplicate warning,
> or klp_shadow_get() incorrectly return stale data?

You're right, this is an issue. We need to ensure the associated shadow
variables are freed during klp_module_going().

[...]

> Can this permanently leak shadow variables and state during an atomic replace?

> Livepatch supports stacking, allowing multiple active patches (e.g., Patch A
> and Patch B) to define the same state. If an atomic replace patch (Patch C)
> is loaded, it replaces both A and B.

With the introduction of replace_set, it's no longer possible for different
patches to define the same state within the same set. So, I don't think this
is an issue.