DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 699F5552F1
Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
To: Petr Mladek <pmladek@suse.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
References: <1498664247-12296-1-git-send-email-joe.lawrence@redhat.com>
 <1498664247-12296-2-git-send-email-joe.lawrence@redhat.com>
 <20170718124500.GF3393@pathway.suse.cz>
 <79756d67-0f6e-4d7d-1827-f1e275e65f27@redhat.com>
 <20170721091350.GB26370@pathway.suse.cz>
From: Joe Lawrence <joe.lawrence@redhat.com>
Organization: Red Hat
Message-ID: <f903b90c-70a9-5e29-2f62-b409bc66cad7@redhat.com>
Date: Fri, 21 Jul 2017 09:55:59 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <20170721091350.GB26370@pathway.suse.cz>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4005
Lines: 91

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