Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 17:32:09 -0800
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Petr Mladek <pmladek@suse.com>
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
Message-ID: <20221108013209.eqrxs3xqtat6kksm@treble>
References: <20221026194122.11761-1-mpdesouza@suse.com>
 <20221026194122.11761-5-mpdesouza@suse.com>
 <20221104010327.wa256pos75dczt4x@treble>
 <Y2TooogxxLTIkBcj@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Y2TooogxxLTIkBcj@alley>
Precedence: bulk

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.

Instead of reference counting, the livepatch atomic replace code could
just migrate any previously owned shadow variables to the new klp_patch.

This of course adds the restriction that such garbage-collected shadow
variables couldn't be shared across non-replace livepatches.  But I
wouldn't expect that to be much of a problem.

Additionally, I was wondering if "which klp_patch owns which shadow
variables" could be auto-detected somehow.

For example:

1) add 'struct module *owner' or 'struct klp_patch *owner' to klp_shadow

2) add klp_shadow_alloc_gc() and klp_shadow_get_or_alloc_gc(), which are
   similar to their non-gc counterparts, with a few additional
   arguments: the klp module owner (THIS_MODULE for the caller); and a
   destructor to be used later for the garbage collection

3) When atomic replacing a patch, iterate through the klp_shadow_hash
   and, for each klp_shadow which previously had an owner, change it to
   be owned by the new patch

4) When unloading/freeing a patch, free all its associated klp_shadows
   (also calling destructors where applicable)


I'm thinking this would be easier for the patch author, and also simpler
overall.  I could work up a patch.

-- 
Josh