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:     Petr Mladek <pmladek@suse.com>
Cc:     Jason Baron <jbaron@akamai.com>, 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
References: <cover.1515786085.git.jbaron@akamai.com>
 <c9064c38-9f04-f6d3-c28b-593a0bb74488@virtuozzo.com>
 <86cac2eb-0de4-bae7-f633-5ad03297880d@akamai.com>
 <20180126102326.u5jscbbgburrzatp@pathway.suse.cz>
 <adf9b91f-e9e9-9d9a-8b28-5754c16c0782@virtuozzo.com>
 <20180130140303.6xmjgnbdemovzif5@pathway.suse.cz>
From:   Evgenii Shatokhin <eshatokhin@virtuozzo.com>
Message-ID: <54c06e8c-2749-7c53-5a44-575c0ba69551@virtuozzo.com>
Date:   Tue, 30 Jan 2018 18:06:39 +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: <20180130140303.6xmjgnbdemovzif5@pathway.suse.cz>
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: 30 Jan 2018 15:06:43.3065 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: e50c190f-1d3c-4b84-b811-08d567f3133c
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0802MB2436
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 30.01.2018 17:03, Petr Mladek wrote:
> On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote:
>> On 26.01.2018 13:23, Petr Mladek wrote:
>>> 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.
>>>
>>> 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.
>>
>> In my experience, it was quite convenient sometimes to just "replace all
>> binary patches the user currently has loaded with this single one". No
>> matter what these original binary patches did and where they came from.
> 
> To be honest, I would feel better if the livepatch framework is
> more safe. It should prevent breaking the system by a patch
> that atomically replaces other random patches that rely on callbacks.
> 
> Well, combining random livepatches from random vendors is a call
> for troubles. It might easily fail when two patches add
> different changes to the same function.
> 
> I wonder if we should introduce some tags, keys, vendors. I mean
> to define an identification or dependencies that would say that some
> patches are compatible or incompatible.
> 
> We could allow to livepatch one function by two livepatches
> only if they are from the same vendor. But then the order still
> might be important. Also I am not sure if this condition is safe
> enough.
> 
> One the other hand, we could handle callbacks like the shadow
> variables. Every shadow variable has an ID. We have an API to
> add/read/update/remove them. We might allow to have more
> callbacks with different IDs. Then we could run callbacks
> only for IDs that are newly added or removed. Sigh, this would
> be very complex implementation as well. But it might make
> these features easier to use and maintain.
> 
> 
> Alternatively, I thought about having two modes. One is
> stack of "random" patches. Second is a cumulative mode.
> IMHO, the combination of the two modes makes things very
> complex. It might be much easier if we allow to load
> patch with the replace flag enabled only on top of
> another patch with this flag enabled.
> 
> 
>> Another problematic situation is when you need to actually downgrade a
>> cumulative patch. Should be rare, but...
> 
> Good point. Well, especially the callbacks should be rare.

Yes, we will probably use them for the most important fixes only and 
only if there are no other suitable options. The patches with could be 
more difficult to maintain anyway.

> 
> I would like to hear from people that have some experience
> or plans with using callbacks and cumulative patches.
> 
> I wonder how they plan to technically reuse a feature
> in the cummulative patches. IMHO, it should be very
> rare to add/remove callbacks. Then it might be safe
> to downgrade a cummulative patch if the callbacks
> are exactly the same.
> 
>> Well, I think we will disable the old patches explicitly in these cases,
>> before loading of the new one. May be fragile but easier to maintain.
> 
> I am afraid that we will not be able to support all use cases
> and keep the code sane.

Thats is OK.

I agree with you that the current behaviour of the 'replace' operation 
w.r.t. patch callbacks should stay as is. Making kernel code more 
complex than it should be is definitely a bad thing.

I cannot say much about generic policies for cumulative/non-cumulative 
patches here. In our particular case (Virtuozzo ReadyKernel), the 
cumulative patches are easily recognizable by their names, they have 
'cumulative' substring there. Patch version is also included in the name 
and is easily obtainable.

So I think, I'll update our userspace tools (based on kpatch) so that 
their 'replace' command worked as follows:

* Explicitly disable all loaded non-cumulative patches first.
* If the new patch is non-cumulative, just disable all loaded patches 
explicitly and then load the new one.
* If the new patch is cumulative, explicitly disable the loaded 
cumulative patches that have smaller version numbers than the new one 
(if any). Then run replace as usual.

This way, the in-kernel 'replace' functionality will only be used when 
upgrading cumulative patches - and we can design their callbacks (if the 
callbacks are needed at some point) according to the current behaviour.

Patch downgrade operations should be very rare and could only happen if 
something goes wrong at the users' side. I think, it is acceptable to 
disable the loaded patch first in that case. Its callbacks, if present, 
will do proper cleanup. Then the older, "good" patch can be loaded 
normally. If such things happen, we'll have to investigate the affected 
nodes anyway.

Same for the custom (non-cumulative) patches. We sometimes use them to 
"patch things up" quickly before the cumulative patch with the 
appropriate fixes is officially released. It should be reasonably safe 
to disable these patches explicitly when loading a new cumulative patch.

So, I think, the current situation with 'replace' & callbacks is 
acceptable for us.

Just my 2 cents.

> 
> Best Regards,
> Petr
> .
> 

Regards,
Evgenii