Received: by 10.223.176.46 with SMTP id f43csp506309wra; Fri, 26 Jan 2018 02:24:08 -0800 (PST) X-Google-Smtp-Source: AH8x2278Sd2I4VX9XhVvh+bGpgnHPcsv0BI0ktBp02Dq2uZAXs0W7bYQERfxio5OYsmpa/eQUHPh X-Received: by 10.101.90.71 with SMTP id z7mr15358690pgs.15.1516962248070; Fri, 26 Jan 2018 02:24:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1516962248; cv=none; d=google.com; s=arc-20160816; b=LS80Zw54/E8czSuc0Nvo0yzZ2CHM+00SXEPV/pjazFWKpx1DCMUuuNFe9Z2PoNVp9L 8EK4yiiXTTcegIaZVlURH1HXZosRiyUTeQssmAKn/EdxveoaJGLDzWQWL4ZB0hRUBe4y HCOE5oxq7wGlo3xzAunmXC07yaV738nFMVUarw2Ai00a/r/tAypKfeH9MS612cidH4Lb 8JbwV0KLpAt3rwtuLdNt4YlYgGadm2D0DS+N662IVr5FEyhcxtxSwGRTlc4careaNHSn hM3kHHp6OLYHSgycXzL5y8B/05xRBovtaCo0enjtuUKYO6lRTy6h6j5MscsM83Wrwdeb MkCw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=vvJeGOP1O5Fj732jxKVF0LJ7QgIRF/fKxu+u0uKgN1I=; b=W9Fs/WgeGILYiM8rSyUw73vCtHq2QJReQIOgtC0Tiz2SCdmg66k/nxb9ExZLHyQraa q7/Ea3qh/4nppuEzI73CW4EKqez33m7m3iTYJvedtTcaGbAp/SkefpCHvCmcR5IyECVa fH+SNN0Iwjs1fb/Q4aFZXtIu2avl5S7SBmR+66oZQ4ehPDwQMAqUmlkB4S/mlhuv622m WXhkxxYX+MKIYxgzj1PSK1pJCW8pjanmfj6k1OvllYzamuwWdTkTg6yviD9zVo2rfTtU 3IzkUkTZ0e61ftTLVq9JbpNFELTa96cM/K90Vm35hTzJC71AYQI1TgI4E0qCUnZ6RjL/ gCmg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 86si6169208pfp.347.2018.01.26.02.23.53; Fri, 26 Jan 2018 02:24:08 -0800 (PST) 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; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752386AbeAZKXa (ORCPT + 99 others); Fri, 26 Jan 2018 05:23:30 -0500 Received: from mx2.suse.de ([195.135.220.15]:39296 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752076AbeAZKX2 (ORCPT ); Fri, 26 Jan 2018 05:23:28 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 3AE23B2F7; Fri, 26 Jan 2018 10:23:27 +0000 (UTC) Date: Fri, 26 Jan 2018 11:23:26 +0100 From: Petr Mladek To: Jason Baron Cc: Evgenii Shatokhin , linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org, mbenes@suse.cz, joe.lawrence@redhat.com Subject: Re: [PATCH v5 0/3] livepatch: introduce atomic replace Message-ID: <20180126102326.u5jscbbgburrzatp@pathway.suse.cz> References: <86cac2eb-0de4-bae7-f633-5ad03297880d@akamai.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <86cac2eb-0de4-bae7-f633-5ad03297880d@akamai.com> User-Agent: NeoMutt/20170421 (1.8.2) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 2018-01-19 16:10:42, Jason Baron wrote: > > > On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote: > > On 12.01.2018 22:55, Jason Baron wrote: > > 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). > > 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. I played with this when working on v6. And I am not sure if it is worth it. The main reason is that we are talking about cumulative patches. They are supposed to preserve most of the existing changes and just remove and/or add few changes. The older patches might or might not expect to be replaced this way. If we would decide to run callbacks from the replaced patches then it would make sense to run the one from the new patch first. It is because we might need to do some hacks to preserve the already existing changes. We might need something like this for __klp_enable_patch(): static int klp_run_pre_patch_callbacks(struct klp_patch *patch) { struct klp_patch *old_patch; struct klp_object *old_obj; int ret; list_for_each_entry_reverse(old_patch, &klp_patches, list) { if (!old_patch->enabled && old_patch != patch) continue; klp_for_each_object(old_patch, old_obj) { if (!klp_is_object_loaded()) continue; if (old_patch == patch) { /* pre_patch from new patch */ ret = klp_pre_patch_callback(obj); if (ret) return ret; if (!patch->replace) return; } else { /* preunpatch from replaced patches */ klp_pre_unpatch_callback(obj); } } } return 0; } This was quite hairy. Alternative would be: static void klp_run_pre_unpatch_callbacks_when_replacing(struct klp_patch *patch) { struct klp_patch *old_patch; struct klp_object *old_obj; if (WARN_ON(!patch->replace)) return; list_for_each_entry_reverse(old_patch, &klp_patches, list) { if (!old_patch->enabled || old_patch == patch) continue; klp_for_each_object(old_patch, old_obj) { if (!klp_is_object_loaded()) continue; klp_pre_unpatch_callback(obj); } } } static int klp_run_pre_patch_callbacks(struct klp_patch *patch) { struct klp_object *old_obj; int ret; klp_for_each_object(patch, old_obj) { if (!klp_is_object_loaded()) continue; ret = klp_pre_patch_callback(obj); if (ret) return ret; } if (patch->replace) klp_run_pre_unpatch_callbacks_when_replacing(patch); return 0; } 2nd variant is easier to read but a lot of code. And this is only what we would need for __klp_enable_patch(). But we would need solution also for: klp_cancel_transition(); klp_try_transition(); (2 variants for patching and unpatching) klp_module_coming(); klp_module_going(); So, we are talking about a lot of rather non-trivial code. IMHO, it might be easier to run just the callbacks from the new patch. In reality, the author should always know what it might be replacing and what needs to be done. By other words, it might be much easier to handle all situations in a single script in the new patch. Alternative would be doing crazy hacks to prevent the older scripts from destroying what we would like to keep. We would need to keep in mind interactions between the scripts and the order in which they are called. Or do you plan to use cumulative patches to simply get rid of any other "random" livepatches with something completely different? In this case, it might be much more safe to disable the older patches a normal way. I would suggest to just document the current behavior. We should create Documentation/livepatch/cummulative-patches.txt anyway. Best Regards, Petr