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