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:     Jason Baron <jbaron@akamai.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
References: <cover.1515786085.git.jbaron@akamai.com>
From:   Evgenii Shatokhin <eshatokhin@virtuozzo.com>
Message-ID: <c9064c38-9f04-f6d3-c28b-593a0bb74488@virtuozzo.com>
Date:   Fri, 19 Jan 2018 22:20:27 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <cover.1515786085.git.jbaron@akamai.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: virtuozzo.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2018 19:20:32.5267 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 60aa5d30-aa65-4df3-8290-08d55f71b5fa
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2440
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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

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