Re: [PATCH v7] livepatch: Clear relocation targets on a module removal

[Miroslav Benes <mbenes@suse.cz>]

Hi

On Thu, 5 Jan 2023, Joe Lawrence wrote:

> On 1/5/23 00:59, Song Liu wrote:
> > On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >>
> >>
> >> Stepping back, this feature is definitely foot-gun capable.
> >> Kpatch-build would expect that klp-relocations would only ever be needed
> >> in code that will patch the very same module that provides the
> >> relocation destination -- that is, it was never intended to reference
> >> through one of these klp-relocations unless it resolved to a live
> >> module.
> >>
> >> On the other hand, when writing the selftests, verifying against NULL
> >> [1] provided 1) a quick sanity check that something was "cleared" and 2)
> >> protected the machine against said foot-gun.
> >>
> >> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
> > 
> > I don't quite follow the foot-gun here. What's the failure mode?
> > 
> 
> Kpatch-build, for better or worse, hides the potential problem.  A
> typical kpatch scenario would be:
> 
> 1. A patch modifies module foo's function bar(), which references
> symbols local to module foo
> 
> 2. Kpatch-build creates a livepatch .ko with klp-relocations in the
> modified bar() to foo's symbols
> 
> 3. When loaded, modified bar() code that references through its
> klp-relocations to module foo will only ever be active when foo is
> loaded, i.e. when the original bar() redirects to the livepatch version.
> 
> However, writing source-based livepatches (like the kselftests) offers a
> lot more freedom.  There is no implicit guarantee from (3) that the
> module is loaded.  One could reference klp-relocations from anywhere in
> the livepatch module.

Yes, on the other hand the approach you describe above seems to be the 
only reasonable one in my opinion. The rest might be considered a bug. 
Foot-gun as you say. I am not sure if we can do anything about it.
 
> > [...]
> > 
> >>> These approaches don't look better to me. But I am ok
> >>> with any of them. Please just let me know which one is
> >>> most preferable:
> >>>
> >>> a. current version;
> >>> b. clear_ undo everything of apply_ (the sample code
> >>>    above)
> >>> c. clear_ undo R_PPC_REL24, but _redo_ everything
> >>>    of apply_ for other ELF64_R_TYPEs. (should be
> >>>   clearer code than option b).
> >>>
> >>
> >> This was my attempt at combining and slightly refactoring the power64
> >> version.  There is so much going on here I was tempted to split off it
> >> into separate value assignment and write functions.  Some changes I
> >> liked, but I wasn't all too happy with the result.  Also, as you
> >> mention, completely undoing R_PPC_REL24 is less than trivial... for this
> >> arch, there are basically three major tasks:
> >>
> >>   1) calculate the new value, including range checking
> >>   2) special constructs created by restore_r2 / create_stub
> >>   3) writing out the value
> >>
> >> and many cases are similar, but subtly different enough to avoid easy
> >> code consolidation.
> > 
> > Thanks for exploring this direction. I guess this part won't be perfect
> > anyway.
> > 
> > PS: While we discuss a solution for ppc64, how about we ship the
> > fix for other archs first? I think there are only a few small things to
> > be addressed.
> > 
> 
> Yeah, the x86_64 version looks a lot simpler and closer to being done.
> Though I believe that Petr would prefer a complete solution, but I'll
> let him speak to that.

I cannot speak for Petr, but I think it might be easier to split it given 
the situation. Then we can involve arch maintainers for ppc64le because 
they might have a preference with respect to a, b, c options above.

Petr, what do you think?

Miroslav