Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 26 Jan 2018 10:09:01 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Jason Baron <jbaron@akamai.com>
Cc:     jpoimboe@redhat.com, jikos@kernel.org, mbenes@suse.cz,
        jeyu@kernel.org, Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
        linux-kernel@vger.kernel.org, live-patching@vger.kernel.org
Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace
Message-ID: <20180126090901.z36n2tn67b2nz75m@pathway.suse.cz>
References: <20180125160203.28959-1-pmladek@suse.com>
 <20180125160203.28959-7-pmladek@suse.com>
 <693db35b-3bc8-d5de-207e-6321226d7f58@akamai.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <693db35b-3bc8-d5de-207e-6321226d7f58@akamai.com>
User-Agent: NeoMutt/20170421 (1.8.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu 2018-01-25 23:27:57, Jason Baron wrote:
> On 01/25/2018 11:02 AM, Petr Mladek wrote:
> > From: Jason Baron <jbaron@akamai.com>
> > A better solution would be to create cumulative patch and say that
> > it replaces all older ones.
> > 
> > Signed-off-by: Jason Baron <jbaron@akamai.com>
> > [pmladek@suse.com: Split, reuse existing code, simplified]
> 
> Hi Petr,
> 
> Thanks for cleaning this up - it looks good.

Uff, I feel relief :-)

> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 6ad3195d995a..c606b291dfcd 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > +/*
> > + * This function removes replaced patches from both func_stack
> > + * and klp_patches stack.
> > + *
> > + * We could be pretty aggressive here. It is called in situation
> > + * when these structures are not longer accessible. All functions
> > + * are redirected using the klp_transition_patch. They use either
> > + * a new code or they in the original code because of the special
> > + * nop function patches.
> > + */
> > +void klp_throw_away_replaced_patches(struct klp_patch *new_patch,
> > +				     bool keep_module)
> > +{
> > +	struct klp_patch *old_patch, *tmp_patch;
> > +
> > +	list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) {
> > +		if (old_patch == new_patch)
> > +			return;
> > +
> > +		klp_unpatch_objects(old_patch, KLP_FUNC_ANY);
> > +		old_patch->enabled = false;
> > +
> > +		/*
> > +		 * Replaced patches could not get re-enabled to keep
> > +		 * the code sane.
> > +		 */
> > +		list_del_init(&old_patch->list);
> 
> I'm wondering if this should be:
> 
> list_move(&old_patch->list, &klp_replaced_patches);

Yes, great catch!

The list_del() comes from one iteration where I got rid of the extra
list. I though that it might be enough to check
patch->kobj.state_initialized. But then I realized that this
kobject state was modified outside klp_mutex.

To be honest, I did not only minimal testing with my changes.
Mirek promised to port a battery of his kGraft-based tests and
run it.

Thanks a lot for review.

Best Regards,
Petr