Date: Fri, 21 Jul 2017 11:27:17 +0200
From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Joe Lawrence <joe.lawrence@redhat.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
Message-ID: <20170721092717.GA28857@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>
 <alpine.LSU.2.20.1707211105350.6086@pobox.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LSU.2.20.1707211105350.6086@pobox.suse.cz>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2182
Lines: 56

On Fri 2017-07-21 11:12:18, Miroslav Benes wrote:
> 
> > >> +{
> > >> +	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.

The shadow id is an integer. This prevents also from using the same
id by two patches for a different purpose. The two livepatches might
be crated months after each other. There might be many fixes
accumulated in the livepatch. Is it really almost impossible
to make mistakes so this rather small change is not worth it?

Another motivation is that the author of the livepatch usually
is not familiar with the patched code. It makes it more prone
to mistakes.

Best Regards,
Petr