On Wed, May 03, 2023 at 03:54:47PM -0400, Joe Lawrence wrote: > On 4/11/23 06:06, Nicolai Stange wrote: > > Josh Poimboeuf <jpoimboe@xxxxxxxxxx> writes: > > > >> On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: > >>> Have you tried retrofitting klp-convert into any real-world livepatch? > >>> I'm curious as to your observations on the overall experience, or > >>> thoughts on the sympos annotation style noted above. > >> > >> On a related note, the patch creation process (of which klp-convert > >> would be part of) needs to be documented. > >> > >> If I remember correctly, the proper safe usage of klp-convert requires a > >> kernel built with -flive-patching, plus some scripting and/or manual > >> processes. > > > > Not always, I think: -flive-patching or IPA optimizations in general > > aren't a concern in the context of data symbols. From a quick glance, it > > seems like the selftests introduced as part of this patchset are > > all restricted to this usecase. > > > > IIRC there is nothing currently stopping klp-convert from converting > function symbol relocations. That may be dangerous when taking > optimizations like sibling functions (and their sharing of stack) into > consideration. This is about the point I stopped to turn and see what > the real use cases may be. > > >> If nobody knows how to safely use it then there wouldn't be much value > >> in merging it. > > > > I tend to agree, but would put it a bit differently: the current > > implementation of klp-convert features quite some convenience logic, > > which, until the question of a documented livepatch preparation process > > has been settled, is not known yet to ever be of any use. > > > > Good observation and perhaps something that Marcos could elaborate on > (pros and cons of klp-convert in his experiments). > In my tests, I took the exact code generated by klp-ccp, and adapted it to not rely on kallsyms anymore, removing the symbol lookups. For data symbols, I changed it to be a extern variable instead of a pointer to it. For the function symbols, also removed the pointer lookup, and left only the function prototype, and it worked as expected. I was quite surprised that it worked quite well. But I agree with Nicolai that the tool itself could be shrunk into a smaller version. In our usage, klp-ccp knows all unexported functions and to which modules they belong, as it's currently used for the symbol lookup. I believe that kpatch-build also has similar information so all the symbols.klp and other Kbuild machinery could be avoided, making the tool responsible to only generate the klp relocations based on the undefined symbols, just as proposed by Nicolai.
