Re: [PATCH v2 4/4] livepatch/shadow: Add garbage collection of shadow variables

From: Petr Mladek <pmladek@suse.com>
To: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Nicolai Stange <nstange@suse.de>,
	Marcos Paulo de Souza <mpdesouza@suse.com>,
	linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
	jpoimboe@redhat.com, joe.lawrence@redhat.com
Subject: Re: [PATCH v2 4/4] livepatch/shadow: Add garbage collection of shadow variables
Date: Wed, 9 Nov 2022 15:36:23 +0100	[thread overview]
Message-ID: <Y2u659H1tZQfG0uQ@alley> (raw)
In-Reply-To: <20221108184410.qhpxhtbfryzeh6eq@treble>

On Tue 2022-11-08 10:44:10, Josh Poimboeuf wrote:
> On Tue, Nov 08, 2022 at 10:14:18AM +0100, Petr Mladek wrote:
> > On Mon 2022-11-07 17:32:09, Josh Poimboeuf wrote:
> > > On Fri, Nov 04, 2022 at 11:25:38AM +0100, Petr Mladek wrote:
> > > > > I get the feeling the latter would be easier to implement (no reference
> > > > > counting; also maybe can be auto-detected with THIS_MODULE?) and harder
> > > > > for the patch author to mess up (by accidentally omitting an object
> > > > > which uses it).
> > > > 
> > > > I am not sure how you mean it. I guess that you suggest to store
> > > > the name of the livepatch module into the shadow variable.
> > > > And use the variable only when the livepatch module is still loaded.
> > > 
> > > Actually I was thinking the klp_patch could have references to all the
> > > shadow variables (or shadow variable types?) it owns.
> > 
> > In short, you suggest to move the array of used klp_shadow_types from
> > struct klp_object to struct klp_patch. Do I get it correctly?
> 
> Right.  Though, thinking about it more, this isn't even needed.  Each
> klp_shadow would have a pointer to its owning module.  We already have a
> global hash of klp_shadows which can be iterated when the module gets
> unloaded or replaced.
>
> Assuming atomic replace, the new patch is almost always a superset of
> the old patch.  We can optimize for that case.

I see. But I do not agree with this assumption. The new livepatch
might also remove code that caused problems.

Also we allow downgrades. I mean that there is no versioning
of livepatches. Older livepatches might replace new ones.

We really want to support downgrades or upgrades that remove
problematic code. Or upgrades that start fixing the problem
a better way using another shadow variable.

I personally think that registering the supported klp_shadow_types
is worth the effort. It allows to do various changes a safe way.

Also it should be easy to make sure that all klp_shadow_types
are registered:

1. Grep might be used to find declarations in the source code.
   IMHO, it should work even with kPatch.

2. The klp_shadow_*() API will warn when a non-registered shadow
   variable is used. IMHO, this might be useful and catch some bugs
   on its own.

Best Regards,
Petr