Re: Re: [PATCHv4 0/3] Kernel Live Patching

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2014/11/27 19:52), Petr Mladek wrote:
> On Thu 2014-11-27 19:06:37, Masami Hiramatsu wrote:
>> (2014/11/27 0:27), Josh Poimboeuf wrote:
>>> On Wed, Nov 26, 2014 at 10:18:24AM +0100, Jiri Kosina wrote:
>>>> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
>>>>
>>>>>> Note to Steve:
>>>>>> Masami's IPMODIFY patch is heading for -next via your tree.  Once it arrives,
>>>>>> I'll rebase and make the change to set IPMODIFY.  Do not pull this for -next
>>>>>> yet.  This version (v4) is for review and gathering acks.
>>>>>
>>>>> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate 
>>>>> ftrace_ops for each function in each patch, it could be conflict each 
>>>>> other.
>>>>
>>>> Yup, this corresponds to what Petr brought up yesterday. There are cases 
>>>> where all solutions (kpatch, kgraft, klp) would allocate multiple 
>>>> ftrace_ops for a single function entry (think of patching one function 
>>>> multiple times in a row).
>>>>
>>>> So it's not as easy as just setting the flag.
>>>>
>>>>> Maybe we need to have another ops hashtable to find such conflict and 
>>>>> new handler to handle it.
>>>>
>>>> If I understand your proposal correctly, that would sound like a hackish 
>>>> workaround, trying to basically trick the IPMODIFY flag semantics you just 
>>>> implemented :)
>>>
>>> I think Masami may be proposing something similar to what we do in
>>> kpatch today.  We have a single ftrace_ops and handler which is used for
>>> all functions.  The handler accesses a global hash of kpatch_func
>>> structs which is indexed by the original function's IP address.
>>
>> Hmm, I think both is OK. kpatch method is less memory consuming and
>> will have a bigger overhead. However, as Steven talked at Plumbers Conf.,
>> he will introduce a direct code modifying interface for ftrace. After
>> that is introduced, we don't need to care about performance degradation
>> by patching :)
> 
> Yup, I would prefer to have ftrace_ops per (original) function entry. I mean
> that new patches will reuse the existing ftrace_ops for already
> patched functions. They will just create new ftrace_ops from the
> not-yet-patched symbols.

However, too many ftrace_ops will get bigger overhead if conflicts
happened on any entry, since on such entry ftrace walks through
all registered ftrace_ops and checks its filter. It's a downside.
Perhaps, ftrace needs to have 2 different ftrace_ops lists, one
is for managing, and one is for walk through. And the latter list
drops the ftrace_ops which has trampoline and whose all filtered ip
is exclusively used (iow, such ftrace_ops never be hit in the walk
through).

> Using a single ftrace_ops everywhere would kill the win from
> Steven's direct ftrace optimization.

It depends on the implementation and interface. E.g. an explicit
path-change optimization interface for each address with ftrace_ops,
like as
  ftrace_change_path_ip(ftrace_ops *ops, unsigned long old_addr,
			unsigned long new_addr);

This checks ftrace_ops is already registered and has given old_addr
in filter, ensures no other ftrace_ops are registered on given address,
and then optimizes the fentry call to jump to the new_addr.

Anyway, at this point it is not a major discussion point, it's a
kind of minor implementation issue for performance and memory consuming.
We can switch it without changing API.

Thank you,

>>> It actually works out pretty well because it nicely encapsulates the
>>> knowledge about which functions are patched in a single place.  And it
>>> makes it easy to track function versions (for incremental patching and
>>> rollback).
>>>
>>>> What I'd propose instead is to make sure that we always have 
>>>> just a ftrace_ops per function entry, and only update the pointers there 
>>>> as necessary. Fortunately we can do the switch atomically, by making use 
>>>> of ->private.
>>>
>>> But how would you update multiple functions atomically, to enforce
>>> per-thread consistency?
>>
>> At this point, both can do it atomically. We just need an atomic flag
>> for applying patches.
> 
> By other words, we would need something like the "kgr_immutable" flag from
> kGraft. It will make sure that everybody stays with the current code until
> all function entries are updated.
> 
> Best Regards,
> Petr


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com