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:   Fri, 4 Nov 2022 11:25:38 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Josh Poimboeuf <jpoimboe@kernel.org>,
        Nicolai Stange <nstange@suse.de>
Cc:     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: <Y2TooogxxLTIkBcj@alley>
References: <20221026194122.11761-1-mpdesouza@suse.com>
 <20221026194122.11761-5-mpdesouza@suse.com>
 <20221104010327.wa256pos75dczt4x@treble>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221104010327.wa256pos75dczt4x@treble>
Precedence: bulk

On Thu 2022-11-03 18:03:27, Josh Poimboeuf wrote:
> On Wed, Oct 26, 2022 at 04:41:22PM -0300, Marcos Paulo de Souza wrote:
> > The life of shadow variables is not completely trivial to maintain.
> > They might be used by more livepatches and more livepatched objects.
> > They should stay as long as there is any user.
> > 
> > In practice, it requires to implement reference counting in callbacks
> > of all users. They should register all the user and remove the shadow
> > variables only when there is no user left.
> > 
> > This patch hides the reference counting into the klp_shadow API.
> > The counter is connected with the shadow variable @id. It requires
> > an API to take and release the reference. The release function also
> > calls the related dtor() when defined.
> > 
> > An easy solution would be to add some get_ref()/put_ref() API.
> > But it would need to get called from pre()/post_un() callbacks.
> > It might be easy to forget a callback and make it wrong.
> > 
> > A more safe approach is to associate the klp_shadow_type with
> > klp_objects that use the shadow variables. The livepatch core
> > code might then handle the reference counters on background.
> > 
> > The shadow variable type might then be added into a new @shadow_types
> > member of struct klp_object. They will get then automatically registered
> > and unregistered when the object is being livepatched. The registration
> > increments the reference count. Unregistration decreases the reference
> > count. All shadow variables of the given type are freed when the reference
> > count reaches zero.
> > 
> > All klp_shadow_alloc/get/free functions also checks whether the requested
> > type is registered. It will help to catch missing registration and might
> > also help to catch eventual races.
> 
> Is there a reason the shadow variable lifetime is tied to klp_object
> rather than klp_patch?

Good question!

My thinking was that shadow variables might be tight to variables
from a particular livepatched module. It would make sense to free them
when the only user (livepatched module) is removed.

The question is if it is really important. I did re-read the original
issue and the real problem was when the livepatch was reloaded.
The related variables were handled using livepatched code, then
without the livepatched code, and then with the livepatched code
again.

The variable was modified with the original code that was not aware of
the shadow variable. As a result the state of the shadow variable was
not in sync when it was later used again.

Nicolai, your have the practical experience. Should the reference
counting be per-livepatched object or per-livepatch, please?

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

This would not help.

We use the reference counting because the shadow variables should
survive an atomic replace of the lifepatch. In this case, the related
variables are still handled using a livepatched code that is aware
of the shadow variables.

Also it does not solve the problem that the shadow variable might
get out of sync with the related variable when the same
livepatch gets reloaded.

Best Regards,
Petr