Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
From: Joe Lawrence
Date: Thu Jul 20 2017 - 16:30:44 EST
On 07/18/2017 08:45 AM, Petr Mladek wrote:
> On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
>> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
>> new file mode 100644
>> index 000000000000..7f28982e6b1c
>> --- /dev/null
>> +++ b/Documentation/livepatch/shadow-vars.txt
>> +Use cases
>> +---------
>> +
>> +See the example shadow variable livepatch modules in samples/livepatch
>> +for full working demonstrations.
>> +
>> +Example 1: Commit 1d147bfa6429 ("mac80211: fix AP powersave TX vs.
>> +wakeup race") added a spinlock to net/mac80211/sta_info.h :: struct
>> +sta_info. Implementing this change with a shadow variable is
>> +straightforward.
>> +
>> +Allocation - when a host sta_info structure is allocated, attach a
>> +shadow variable copy of the ps_lock:
>> +
>> +#define PS_LOCK 1
>> +struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>> + const u8 *addr, gfp_t gfp)
>> +{
>> + struct sta_info *sta;
>> + spinlock_t *ps_lock;
>> + ...
>> + sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);
>
> klp_shadow_attach() does the allocation as well now.
> Note that we could pass already initialized spin_lock.
>
>> + ...
>> + ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
>> + if (!ps_lock)
>> + goto shadow_fail;
>> + spin_lock_init(ps_lock);
>> + ...
>> +
>> +Usage - when using the shadow spinlock, query the shadow variable API to
>> +retrieve it:
>> +
>> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>> +{
>> + spinlock_t *ps_lock;
>> + ...
>> + /* sync with ieee80211_tx_h_unicast_ps_buf */
>> + ps_lock = klp_shadow_get(sta, "ps_lock");
>
> s/"ps_lock"/PS_LOCK/
>
> The same problem is repeated many times below (also in the 2nd
> example).
>
> Also this is a nice example, where klp_shadow_get_or_attach()
> would be useful. It would fix even already existing instances.
>
> So, the code might look like:
>
> void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> {
> DEFINE_SPINLOCK(ps_lock_fallback)
> spinlock_t *ps_lock;
> ...
> /* sync with ieee80211_tx_h_unicast_ps_buf */
> ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> &ps_lock_fallback, sizeof(ps_lock_fallback),
> GFP_ATOMIC);
>
> It is a bit ugly that we always initialize ps_lock_fallback
> even when it is not used. But it helps to avoid a custom
> callback that would create the fallback variable. I think
> that it is an acceptable deal.
Yup, it's a tradeoff for the caller. If they want a shadow variable
added and considered "live", they better have previously initialized it :)
>
>> + if (ps_lock)
>> + spin_lock(ps_lock);
>> + ...
>> + if (ps_lock)
>> + spin_unlock(ps_lock);
>> + ...
>> +
>> +Release - when the host sta_info structure is freed, first detach the
>> +shadow variable and then free the shadow spinlock:
>> +
>> +void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
>> +{
>> + spinlock_t *ps_lock;
>> + ...
>> + ps_lock = klp_shadow_get(sta, "ps_lock");
>> + if (ps_lock)
>> + klp_shadow_detach(sta, "ps_lock");
>
> Isn't klp_shadow_detach() enough? If it an optimization,
> klp_shadow_detach() might get optimized the same way.
> But I am not sure if it is worth it.
Let me go back and review these inline examples for v3... I didn't
update them carefully enough when drafting v2.
>> + kfree(sta);
>> +
>> +
>
>
>> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
>> new file mode 100644
>> index 000000000000..d37a61c57e72
>> --- /dev/null
>> +++ b/kernel/livepatch/shadow.c
>> +/**
>> + * _klp_shadow_attach() - allocate and add a new shadow variable
>> + * @obj: pointer to original data
>> + * @num: numerical description of new data
>> + * @new_data: pointer to new data
>> + * @new_size: size of new data
>> + * @gfp_flags: GFP mask for allocation
>> + * @lock: take klp_shadow_lock during klp_shadow_hash operations
>> + *
>> + * Note: allocates @new_size space for shadow variable data and copies
>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
>> + * space. If @new_data is NULL, @new_size is still allocated, but no
>> + * copy is performed.
>> + *
>> + * Return: the shadow variable new_data element, NULL on failure.
>> + */
>> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
>> + size_t new_size, gfp_t gfp_flags,
>> + bool lock)
>
> Nested implementation is usually prefixed by two underlines __.
> It is more visible and helps to distinguish it from the normal function.
Noted for v3.
>> +{
>> + struct klp_shadow *shadow;
>> + unsigned long flags;
>> +
>> + shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
>> + if (!shadow)
>> + return NULL;
>> +
>> + shadow->obj = obj;
>> + shadow->num = num;
>> + if (new_data)
>> + memcpy(shadow->new_data, new_data, new_size);
>> +
>> + if (lock)
>> + spin_lock_irqsave(&klp_shadow_lock, flags);
>> + hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
>
> We should check if the shadow variable already existed. Otherwise,
> it would be possible to silently create many duplicates.
>
> It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> to behave the same.
They would be almost exactly the same, except one version would bounce a
redundant entry while the other would return the existing one. I could
envision callers wanting any of the following behavior:
If a shadow <obj, id> already exists:
0 - add a second shadow variable (??? why)
1 - return NULL, WARN
2 - return the existing one
3 - update the existing one with the new data and return it
* v2 klp_shadow_attach() currently implements #0, can be made to do #1
* v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
makes more sense
Going back to existing kpatch use-cases, since we paired shadow variable
creation to their parent object creation, -EEXIST was never an issue. I
think we concocted one proof-of-concept kpatch where we created shadow
variables "in-flight", that is, we patched a routine that operated on
the parent object and created a shadow variable if one did not already
exist. The in-flight patch was for single function and we knew that it
would never be called concurrently for the same parent object. tl;dr =
kpatch never worried about existing shadow <obj, id>.
> I would do WARN() in klp_shadow_attach() when the variable
> already existed are return NULL. Of course it might be inoncent
> duplication. But it might mean that someone else is using another
> variable of the same name but with different content. klp_shadow_get()
> would then return the same variable for two different purposes.
> Then the whole system might end like a glass on a stony floor.
What do you think of expanding the API to include each the cases
outlined above? Something like:
1 - klp_attach = allocate and add a unique <obj, id> to the hash,
duplicates return NULL and a WARN
2 - klp_get_or_attach = return <obj, id> if it already exists,
otherwise allocate a new one
3 - klp_get_or_update = update and return <obj, id> if it already
exists, otherwise allocate a new one
IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
be dropped. Since you suggested adding klp_get_or_attach(), what do you
think?
>
>> + if (lock)
>> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +
>> + return shadow->new_data;
>> +}
>
> Otherwise, I rather like the API. Thanks a lot for adding
> klp_shadow_get_or_attach().
>
> I did not comment things that were already discussed in
> other threads.
klp_shadow_get_or_attach() looks to be really useful in concurrent
situations, especially cases where we'd like to do in-flight shadow
variable creation.
Appreciate the comments as always.
-- Joe