Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden <zhangwarden@gmail.com>]

Hi, Petr

> On Sep 7, 2024, at 00:39, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Fri 2024-09-06 17:39:46, zhang warden wrote:
>> Hi, John & Miroslav
>> 
>>>> 
>>>> Would it be possible to just use klp_transition_patch and implement the 
>>>> logic just in using_show()?
>>> 
>>> Yes, containing the logic to the sysfs file sounds a lot better.
>> 
>> Maybe I can try to use the state of klp_transition_patch to update the function's state instead of changing code in klp_complete_transition?
>> 
>>> 
>>>> I have not thought through it completely but 
>>>> klp_transition_patch is also an indicator that there is a transition going 
>>>> on. It is set to NULL only after all func->transition are false. So if you 
>>>> check that, you can assign -1 in using_show() immediately and then just 
>>>> look at the top of func_stack.
>>> 
>>> sysfs already has per-patch 'transition' and 'enabled' files so I don't
>>> like duplicating that information.
>>> 
>>> The only thing missing is the patch stack order.  How about a simple
>>> per-patch file which indicates that?
>>> 
>>> /sys/kernel/livepatch/<patchA>/order => 1
>>> /sys/kernel/livepatch/<patchB>/order => 2
>>> 
>>> The implementation should be trivial with the use of
>>> klp_for_each_patch() to count the patches.
>>> 
>> I think this is the second solution. It seems that adding an
>> interface to patch level is an acceptable way.
> 
> It seems to be the only acceptable idea at the moment.
> 
>> And if patch order
>> is provided in /sys/kernel/livepatch/<patchA>/order, we should
>> make a user space tool to calculate the function that
>> is activate in the system. From my point to the original
>> problem, it is more look like a workaround.
> 
> It is always a compromise between the complexity and the benefit.
> Upstream will accept only changes which are worth it.
> 
> Here, the main problem is that you do not have coordinated developement
> and installation of livepatches. This is dangerous and you should
> not do it! Upstream will never support such a wild approach.
> 
> You could get upstream some features which would make your life
> easier. But the features must be worth the effort.

Yep, I have the same idea as you here. The problem we face now as Josh describes, livepatch now missing the information of patch order. If we can have it, the rest information user need can be processed my the original information from the kernel.

My intention to introduce "using" feature is to solve the missing info of livepatch, facing the original problem I met. But as livepatch becomes more and more widely used, this is a real problem that may be widely encountered.

If maintainer thinks the way changing klp_transition_patch is not worth it. Maybe I can try another way to fix this problem. ( For example, the patch-level order interface?) :) 

From my point of view, this information have its worth it provide, what we need to consider is how to implement this function specifically. Do you agree with me?

Best Regards,
Wardenjohn.