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. For example, in my test_klp_convert1.c test case, I have a livepatch module with a sysfs interface (print_debug_set()) that allows the test bash script to force the module to references through its klp-relocations (print_static_strings()): ... static void print_string(const char *label, const char *str) { if (str) pr_info("%s: %s\n", label, str); } ... static noinline void print_static_strings(void) { print_string("klp_string.12345", klp_string_a); print_string("klp_string.67890", klp_string_b); } /* provide a sysfs handle to invoke debug functions */ static int print_debug; static int print_debug_set(const char *val, const struct kernel_param *kp) { print_static_strings(); return 0; } static const struct kernel_param_ops print_debug_ops = { .set = print_debug_set, .get = param_get_int, }; When I first wrote test_klp_convert1.c, I did not have wrappers like print_string(), I simply referenced the symbols directly and send them to pr_info as "%s" string formatting options. You can probably see where this is going when I unloaded the module that provided klp_string_a, klp_string_b, etc. and invoked the sysfs handle. The stale relocation values point to ... somewhere we shouldn't try to reference any more. Perhaps I'm too paranoid about that possibility, but by actually clearing the values in the relocations on module removal, one could check them and try to guard against dangling pointer (dangling relocation?) references. > [...] > >>> 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. Alternatively, we could roll this into the klp-convert-tree as an early patch (or separate branch), where development could continue on the ppc64le and s390x arches as needed. I'll caution that progress is rather slow on the entire patchset, so it may remain out of tree for quite a while. :( -- Joe
