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