Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence <joe.lawrence@redhat.com>]

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