From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>, Jiri Kosina <jikos@kernel.org>,
Jason Baron <jbaron@akamai.com>,
Joe Lawrence <joe.lawrence@redhat.com>,
Jessica Yu <jeyu@kernel.org>,
Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.
Date: Wed, 11 Apr 2018 07:32:14 -0500 [thread overview]
Message-ID: <20180411123214.mfpkbrirze32phrb@treble> (raw)
In-Reply-To: <alpine.LSU.2.21.1804110956430.28885@pobox.suse.cz>
On Wed, Apr 11, 2018 at 10:07:31AM +0200, Miroslav Benes wrote:
> > > I was confused by wording "in the middle". It suggested that there
> > > might had been enabled patches on the top and the bottom of the stack
> > > and some disabled patches in between at the same time (or vice versa).
> > > This was not true.
> >
> > That *was* what I meant. Consider the following sequence of events:
> >
> > - Register patch 1
> > - Enable patch 1
> > - Register patch 2
> > - Enable patch 2
> > - Disable patch 2
> > - Register patch 3
> > - Enable patch 3
> >
> > Notice that patch 2 (in the middle) is disabled, whereas patch 1 (on the
> > bottom) and patch 3 (on the top) are enabled.
>
> This should not be possible at all.
>
> __klp_enable_patch:
>
> if (patch->list.prev != &klp_patches &&
> !list_prev_entry(patch, list)->enabled)
> return -EBUSY;
>
> When patch 3 is enabled, list_prev_entry() returns patch 2 and its
> ->enabled is false.
Hm, you're right. I'm not sure how I got that idea...
I still agree with my original conclusion that enforcing stack order no
longer makes sense though.
> > > Another possibility would be to get rid of the enable/disable states.
> > > I mean that the patch will be automatically enabled during
> > > registration and removed during unregistration.
> >
> > I don't see how disabling during unregistration would be possible, since
> > the unregister is called from the patch module exit function, which
> > can only be called *after* the patch is disabled.
> >
> > However, we could unregister immediately after disabling (i.e., in
> > enabled_store context).
>
> I think this is what Petr meant. So there would be nothing in the patch
> module exit function. Well, not exactly. We'd need to remove sysfs dir and
> maybe something more.
Sounds good to me, though aren't the livepatch sysfs entries removed by
klp during unregister?
> > > The question is what is acceptable to others
> >
> > If there are any objections, this is their chance to speak up :-)
> >
> > > and if it needs to be done as part of this patch set.
> >
> > Maybe so, for at least a few reasons:
> >
> > - This patch set makes the 'stack' obsolete, so it makes sense to remove
> > the 'stack' with it.
>
> Not necessarily. I like Petr's rebase explanation here.
I'm not sure what you mean. IIRC, his rebase explanation referred to
how we handle 'replace' patches, for which there is no stacking (as I
meant the term: enforcement of stack order for registration and
enablement).
--
Josh
next prev parent reply other threads:[~2018-04-11 12:32 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-07 8:20 [PATCH v10 00/10] livepatch: Atomic replace feature Petr Mladek
2018-03-07 8:20 ` [PATCH v10 01/10] livepatch: Use lists to manage patches, objects and functions Petr Mladek
2018-03-13 22:38 ` Josh Poimboeuf
2018-03-07 8:20 ` [PATCH v10 02/10] livepatch: Free only structures with initialized kobject Petr Mladek
2018-03-13 22:38 ` Josh Poimboeuf
2018-03-14 15:50 ` Petr Mladek
2018-03-07 8:20 ` [PATCH v10 03/10] livepatch: Initial support for dynamic structures Petr Mladek
2018-03-13 22:44 ` Josh Poimboeuf
2018-03-19 13:10 ` Petr Mladek
2018-03-07 8:20 ` [PATCH v10 04/10] livepatch: Allow to unpatch only functions of the given type Petr Mladek
2018-03-07 8:20 ` [PATCH v10 05/10] livepatch: Support separate list for replaced patches Petr Mladek
2018-03-13 22:46 ` Josh Poimboeuf
2018-03-19 15:02 ` Petr Mladek
2018-03-19 21:43 ` Josh Poimboeuf
2018-03-20 12:25 ` Petr Mladek
2018-03-20 12:48 ` Evgenii Shatokhin
2018-03-20 13:30 ` Miroslav Benes
2018-03-20 20:15 ` Josh Poimboeuf
2018-03-23 9:45 ` Petr Mladek
2018-03-23 22:44 ` Josh Poimboeuf
2018-03-26 10:11 ` Petr Mladek
2018-04-06 19:50 ` Josh Poimboeuf
2018-04-10 8:34 ` Petr Mladek
2018-04-10 13:21 ` Miroslav Benes
2018-04-10 13:56 ` Evgenii Shatokhin
2018-04-10 17:47 ` Josh Poimboeuf
2018-04-11 7:56 ` Miroslav Benes
2018-04-10 17:42 ` Josh Poimboeuf
2018-04-11 8:07 ` Miroslav Benes
2018-04-11 12:32 ` Josh Poimboeuf [this message]
2018-04-11 13:39 ` Miroslav Benes
2018-04-11 14:17 ` Petr Mladek
2018-04-11 15:48 ` Josh Poimboeuf
2018-04-16 14:58 ` Petr Mladek
2018-04-16 19:04 ` Josh Poimboeuf
2018-04-17 8:23 ` Miroslav Benes
2018-04-17 15:37 ` Petr Mladek
2018-04-17 17:57 ` Josh Poimboeuf
2018-03-07 8:20 ` [PATCH v10 06/10] livepatch: Add atomic replace Petr Mladek
2018-03-13 22:48 ` Josh Poimboeuf
2018-03-20 14:35 ` Petr Mladek
2018-03-20 21:26 ` Josh Poimboeuf
2018-03-22 15:43 ` Petr Mladek
2018-03-07 8:20 ` [PATCH v10 07/10] livepatch: Correctly handle atomic replace for not yet loaded modules Petr Mladek
2018-03-13 14:55 ` Petr Mladek
2018-03-07 8:20 ` [PATCH v10 08/10] livepatch: Improve dynamic struct klp_object detection and manipulation Petr Mladek
2018-03-07 8:20 ` [PATCH v10 09/10] livepatch: Allow to replace even disabled patches Petr Mladek
2018-03-07 8:20 ` [PATCH v10 10/10] livepatch: Atomic replace and cumulative patches documentation Petr Mladek
2018-03-07 21:55 ` [PATCH v10 00/10] livepatch: Atomic replace feature Joe Lawrence
2018-03-08 15:01 ` Petr Mladek
2018-03-08 15:09 ` Joe Lawrence
2018-03-12 18:57 ` Joe Lawrence
2018-03-20 13:16 ` Miroslav Benes
2018-03-26 10:56 ` Petr Mladek
2018-03-26 18:12 ` Joe Lawrence
2018-03-27 8:22 ` Petr Mladek
2018-08-17 10:17 ` Evgenii Shatokhin
2018-08-17 14:53 ` Petr Mladek
2018-08-17 15:33 ` Evgenii Shatokhin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180411123214.mfpkbrirze32phrb@treble \
--to=jpoimboe@redhat.com \
--cc=eshatokhin@virtuozzo.com \
--cc=jbaron@akamai.com \
--cc=jeyu@kernel.org \
--cc=jikos@kernel.org \
--cc=joe.lawrence@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=pmladek@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).