From: Jason Baron <jbaron@akamai.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org,
pmladek@suse.com
Subject: Re: [PATCH 0/3] livepatch: introduce atomic replace
Date: Fri, 21 Jul 2017 13:56:40 -0400 [thread overview]
Message-ID: <2476c7a9-e962-8c20-2cf1-41befdd8ef95@akamai.com> (raw)
In-Reply-To: <alpine.LSU.2.20.1707211459480.6086@pobox.suse.cz>
On 07/21/2017 09:06 AM, Miroslav Benes wrote:
> On Wed, 19 Jul 2017, Jason Baron wrote:
>
>> Hi,
>>
>> In testing livepatch, I found that when doing cumulative patches, if a patched
>> function is completed reverted by a subsequent patch (back to its original state)
>> livepatch does not revert the funtion to its original state. Specifically, if
>> patch A introduces a change to function 1, and patch B reverts the change to
>> function 1 and introduces changes to say function 2 and 3 as well, the change
>> that patch A introducd to function 1 is still present. This could be addressed
>> by first completely removing patch A (disable and then rmmod) and then inserting
>> patch B (insmod and enable), but this leaves an unpatched window. In discussing
>> this issue with Josh on the kpatch mailing list, he mentioned that we could get
>> 'atomic replace working properly', and that is the direction of this patchset:
>> https://www.redhat.com/archives/kpatch/2017-June/msg00005.html
>
> Hi Jason,
>
> this has been on my TODO list for a long time now, so thanks for working
> on this. We have the same feature in kGraft and we use it heavily (in fact
> we distribute our patches as cumulative and "replace_all" how we call it).
>
Hi Miroslav,
Cool - we feel like this is an important feature as well and would like
to have an upstream solution as well.
> The forward port of the feature from kGraft is unfortunately not
> straightforward. We do not have a concept of klp_target_state there, so we
> can freely let functions to be patched or reverted in one go. We cannot do
> the same upstream. At first glance, you used nop function exactly for this
> case. Nice hack :).
>
>> Patches:
>>
>> 1) livepatch: Add klp_object and klp_func iterators
>> Just a prep patch for the 'atomic revert' feature
>>
>> 2) livepatch: add atomic replace
>> Core feature
>>
>> 3) livepatch: Add a sysctl livepatch_mode for atomic replace
>> Introduces a knob for enabling atomic replace. I hate knobs and perhaps
>> its possible to default to cumulative replace? Although I suspect there
>> are workflows relying on the existing behavior - I'm not sure. It may
>> be desirable to associate the knob with the patch itself as in the
>> 'immediate' flag, such that we don't introduce a global sysctl that
>> likely would also need to built-in, if there are patches in the initrd.
>
> Yes. I think it should be associated with the patch itself. This would
> allow more flexible behaviour. You could stack more patches on top of
> "atomic replace" patch.
>
Ok - associating the "atomic replace" with the patch itself makes sense
to me. It would also basically work, I think with the patch I proposed
except for the case where the the "atomic replace" was on top of several
non-"atomic replace" patches. The reason is that the "atomic replace" I
posted looks back 1 patch to see what it needs to replace (assuming all
patches are in atomic replace mode). So instead of just looking back 1
patch, it could 'look back' and make sure it was replacing all
previously loaded patches.
> Anyway, I'm on holiday next week, so I'll take a proper look the week
> after.
>
Ok - have a nice holiday!
Thanks,
-Jason
> Thanks,
> Miroslav
>
next prev parent reply other threads:[~2017-07-21 17:57 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-19 17:18 [PATCH 0/3] livepatch: introduce atomic replace Jason Baron
2017-07-19 17:18 ` [PATCH 1/3] livepatch: Add klp_object and klp_func iterators Jason Baron
2017-08-24 14:25 ` Petr Mladek
2017-08-24 15:39 ` Jason Baron
2017-08-25 11:34 ` Petr Mladek
2017-07-19 17:18 ` [PATCH 2/3] livepatch: add atomic replace Jason Baron
2017-08-25 9:26 ` Petr Mladek
2017-08-25 11:53 ` Petr Mladek
2017-08-30 21:36 ` Jason Baron
2017-07-19 17:18 ` [PATCH 3/3] livepatch: Add a sysctl livepatch_mode for " Jason Baron
2017-07-21 13:06 ` [PATCH 0/3] livepatch: introduce " Miroslav Benes
2017-07-21 17:56 ` Jason Baron [this message]
2017-08-10 11:12 ` Miroslav Benes
2017-08-10 14:56 ` Jason Baron
2017-08-15 9:26 ` Miroslav Benes
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=2476c7a9-e962-8c20-2cf1-41befdd8ef95@akamai.com \
--to=jbaron@akamai.com \
--cc=jeyu@kernel.org \
--cc=jikos@kernel.org \
--cc=jpoimboe@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).