From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753952AbdGUJNy (ORCPT ); Fri, 21 Jul 2017 05:13:54 -0400 Received: from mx2.suse.de ([195.135.220.15]:33276 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753388AbdGUJNw (ORCPT ); Fri, 21 Jul 2017 05:13:52 -0400 Date: Fri, 21 Jul 2017 11:13:50 +0200 From: Petr Mladek To: Joe Lawrence Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Miroslav Benes 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 . 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 to the hash, > duplicates return NULL and a WARN Sounds good. > 2 - klp_get_or_attach = return if it already exists, > otherwise allocate a new one Sounds good. > 3 - klp_get_or_update = update and return 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