Date: Fri, 21 Jul 2017 11:12:18 +0200 (CEST)
From: Miroslav Benes <mbenes@suse.cz>
To: Joe Lawrence <joe.lawrence@redhat.com>
cc: Petr Mladek <pmladek@suse.com>, 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>
Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
In-Reply-To: <79756d67-0f6e-4d7d-1827-f1e275e65f27@redhat.com>
Message-ID: <alpine.LSU.2.20.1707211105350.6086@pobox.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>
User-Agent: Alpine 2.20 (LSU 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3473
Lines: 84


> >> +{
> >> +	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

I have a feeling that we're becoming overprotective here again. I think 
that klp_shadow_attach() adding a new entry makes sense. 
Although I can imagine #1. I think it is a responsibility of the user to 
know what to call. And that is what klp_shadow_get_or_attach() is for.

klp_shadow_get() and klp_shadow_attach() are two main API functions. 
klp_shadow_get_or_attach() is there to make things safe if needed 
(concurrency).
 
> 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>.

And this makes sense to me too.

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

I don't know. I'd be prudent now. We can always add it later...

Miroslav