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;
Subject: Re: [PATCH v5 0/3] livepatch: introduce atomic replace
To:     Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
        linux-kernel@vger.kernel.org, live-patching@vger.kernel.org
Cc:     jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org,
        mbenes@suse.cz, pmladek@suse.com, joe.lawrence@redhat.com
References: <cover.1515786085.git.jbaron@akamai.com>
 <c9064c38-9f04-f6d3-c28b-593a0bb74488@virtuozzo.com>
From:   Jason Baron <jbaron@akamai.com>
Message-ID: <86cac2eb-0de4-bae7-f633-5ad03297880d@akamai.com>
Date:   Fri, 19 Jan 2018 16:10:42 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <c9064c38-9f04-f6d3-c28b-593a0bb74488@virtuozzo.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
> On 12.01.2018 22:55, Jason Baron wrote:
>> Hi,
>>   While using 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 introduced 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
>>
>> Thanks,
>>
>> -Jason
> 
> Thanks a lot! Atomic replace is really crucial when using cumulative
> patches.
> 
> There is one more thing that might need attention here. In my
> experiments with this patch series, I saw that unpatch callbacks are not
> called for the older binary patch (the one being replaced).

Thanks for testing and pointing this out.

> 
> That is, I have prepared 2 binary patches, each has all 4 patch/unpatch
> callbacks.
> 
> When I load the first patch, its pre-patch and post-patch callbacks are
> called as expected.
> 
> Then I replace it with the second patch. Replacement is successful, the
> pre-patch and post-patch callbacks are called for the second patch,
> However, pre-unpatch and post-unpatch callbacks do not run for the first
> one. This makes it more difficult to clean up what its pre/post-patch
> callbacks have done.
> 
> It would be nice if pre-/post- unpatch callbacks were called for the
> first patch, perhaps, before/after the patch is actually disabled during
> replacement. I cannot see right now though, which way is the best to
> implement that.
So I think the pre_unpatch() can be called for any prior livepatch
modules from __klp_enable_patch(). Perhaps in reverse order of loading
(if there is more than one), and *before* the pre_patch() for the
livepatch module being loaded. Then, if it sucessfully patches in
klp_complete_transition() the post_unpatch() can be called for any prior
livepatch modules as well. I think again it makes sense to call the
post_unpatch() for prior modules *before* the post_patch() for the
current livepatch modules.

Thanks,

-Jason

>>>> v4-v5
>> -re-base onto remove-immediate branch (removing immediate dependencies)
>> -replaced modules can be re-enabled by doing rmmod and then insmod
>>
>> v3-v4:
>> -add static patch, objects, funcs to linked lists to simplify iterator
>> -break-out pure function movement as patch 2/3
>>   v2-v3:
>> -refactor how the dynamic nops are calculated (Petr Mladek)
>> -move the creation of dynamic nops to enable/disable paths
>> -add klp_replaced_patches list to indicate patches that can be re-enabled
>> -dropped 'replaced' field
>> -renamed dynamic fields in klp_func, object and patch
>> -moved iterator implementation to kernel/livepatch/core.c
>> -'inherit' nop immediate flag
>> -update kobject_put free'ing logic (Petr Mladek)
>>
>> v1-v2:
>> -removed the func_iter and obj_iter (Petr Mladek)
>> -initialiing kobject structure for no_op functions using:
>>   klp_init_object() and klp_init_func()
>> -added a 'replace' field to klp_patch, similar to the immediate field
>> -a 'replace' patch now disables all previous patches
>> -tried to shorten klp_init_patch_no_ops()...
>> -Simplified logic klp_complete_transition (Petr Mladek)
>>
>> Jason Baron (3):
>>    livepatch: use lists to manage patches, objects and functions
>>    livepatch: shuffle core.c function order
>>    livepatch: add atomic replace
>>
>>   include/linux/livepatch.h     |  25 +-
>>   kernel/livepatch/core.c       | 626
>> ++++++++++++++++++++++++++++++------------
>>   kernel/livepatch/core.h       |   6 +
>>   kernel/livepatch/patch.c      |  22 +-
>>   kernel/livepatch/patch.h      |   4 +-
>>   kernel/livepatch/transition.c |  49 +++-
>>   6 files changed, 537 insertions(+), 195 deletions(-)
>>
> 
> Regards,
> Evgenii