From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932555AbeASVLa (ORCPT ); Fri, 19 Jan 2018 16:11:30 -0500 Received: from mx0b-00190b01.pphosted.com ([67.231.157.127]:45054 "EHLO mx0b-00190b01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754118AbeASVLX (ORCPT ); Fri, 19 Jan 2018 16:11:23 -0500 Subject: Re: [PATCH v5 0/3] livepatch: introduce atomic replace To: Evgenii Shatokhin , 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: From: Jason Baron 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: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-19_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=1 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801190273 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-19_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801190272 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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