Re: [PATCHv4 0/3] Kernel Live Patching

Petr Mladek
2014-11-27T10:52:37Z

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.

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

> > 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