On 07/21/2017 05:13 AM, Petr Mladek wrote:
> On Thu 2017-07-20 16:30:37, Joe Lawrence wrote:
>> 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 am not sure if you want to explain why you did not care. Or if
> you want to suggest that we should not care :-)
We knew that in our limited use-cases for in-flight shadow variables,
concurrency was not an issue. Josh has a better historical perspective,
but I think this particular use-case appeared way after the initial
kpatch implementation of shadow variables. Now that we know we can use
them in this way, I agree that it's important to hash out the
implications while designing the livepatch counterpart.
> I agree that if the API is used in simple/clear situations then
> this might look like an overkill. But I am afraid that the API users
> do not have this in hands. They usually have to create a livepatch
> based on an upstream secutity fix. The fix need not be always simple.
> Then it is handy to have an API that helps to catch mistakes
> and keeps the patched system in a sane state.
Very true, though I wonder what interesting state that will be when
running patched code and partially shadowed variables. :) At the very
least, I think this protection would be valuable during patch sanity
testing.
>>> 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
>
> Sounds good.
>
>> 2 - klp_get_or_attach = return <obj, id> if it already exists,
>> otherwise allocate a new one
>
> Sounds good.
>
>> 3 - klp_get_or_update = update and return <obj, id> if it already
>> exists, otherwise allocate a new one
>
> I am not sure where this behavior would make sense. See below.
>
>
>> 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?
>
> I do not agree. Let's look at the example with the missing lock.
> The patch adds the lock if it did not exist. Then the lock can
> be used to synchronize all further operations.
>
> klp_get_or_update() would always replace the existing lock
> with a freshly initialized one. We would loss the information
> if it was locked or not.
Ah good point, perhaps we have two situations here:
A - A shadow variable that's pointing to some object, like a lock,
where the original object is required. (Your example above.)
B - A shadow variable that's storing the data itself. In other words,
instead of attaching a pointer, the whole object was attached:
void patched_function()
{
...
klp_get_or_attach(obj, id, &jiffies, sizeof(jiffies), ...)
...
in which case the caller is only interested in pushing in the
latest version of jiffies.
For these I suggest klp_get_or_attach() for case A and
klp_get_or_update() for case B.
-- Joe
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
