Date: Fri, 21 Jul 2017 11:13:50 +0200
From: Petr Mladek <pmladek@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.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>
Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
Message-ID: <20170721091350.GB26370@pathway.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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <79756d67-0f6e-4d7d-1827-f1e275e65f27@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4734
Lines: 121

On Thu 2017-07-20 16:30:37, Joe Lawrence wrote:
> On 07/18/2017 08:45 AM, Petr Mladek wrote:
> > On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
> >> 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
> >> +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 am not sure if you want to explain why you did not care. Or if
you want to suggest that we should not care :-)

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.


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


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

Thanks a lot for working in the API. It will be handy.

Best Regards,
Petr