On Fri, Jun 28, 2024 at 02:23:49PM +0200, Miroslav Benes wrote: > On Fri, 7 Jun 2024, Song Liu wrote: > > > Hi Miroslav, > > > > Thanks for reviewing the patch! > > > > On Fri, Jun 7, 2024 at 6:06 AM Miroslav Benes <mbenes@xxxxxxx> wrote: > > > > > > Hi, > > > > > > On Tue, 4 Jun 2024, Song Liu wrote: > > > > > > > With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm.<hash> > > > > to avoid symbol duplication. scripts/kallsyms.c sorted the symbols > > > > without these postfixes. The default symbol lookup also removes these > > > > postfixes before comparing symbols. > > > > > > > > On the other hand, livepatch need to look up symbols with the full names. > > > > However, calling kallsyms_on_each_match_symbol with full name (with the > > > > postfix) cannot find the symbol(s). As a result, we cannot livepatch > > > > kernel functions with .llvm.<hash> postfix or kernel functions that use > > > > relocation information to symbols with .llvm.<hash> postfixes. > > > > > > > > Fix this by calling kallsyms_on_each_match_symbol without the postfix; > > > > and then match the full name (with postfix) in klp_match_callback. > > > > > > > > Signed-off-by: Song Liu <song@xxxxxxxxxx> > > > > --- > > > > include/linux/kallsyms.h | 13 +++++++++++++ > > > > kernel/kallsyms.c | 21 ++++++++++++++++----- > > > > kernel/livepatch/core.c | 32 +++++++++++++++++++++++++++++++- > > > > 3 files changed, 60 insertions(+), 6 deletions(-) > > > > > > I do not like much that something which seems to be kallsyms-internal is > > > leaked out. You need to export cleanup_symbol_name() and there is now a > > > lot of code outside. I would feel much more comfortable if it is all > > > hidden from kallsyms users and kept there. Would it be possible? > > > > I think it is possible. Currently, kallsyms_on_each_match_symbol matches > > symbols without the postfix. We can add a variation or a parameter, so > > that it matches the full name with post fix. > > I think it might be better. > > Luis, what is your take on this? > > If I am not mistaken, there was a patch set to address this. Luis might > remember more. Yeah this is a real issue outside of CONFIG_LTO_CLANG, Rust modules is another example where instead of symbol names they want to use full hashes. So, as I hinted to you Sami, can we knock two birds with one stone here and move CONFIG_LTO_CLANG to use the same strategy as Rust so we have two users instead of just one? Then we resolve this. In fact what I suggested was even to allow even non-Rust, and in this case even with gcc to enable this world. This gives much more wider scope of testing / review / impact of these sorts of changes and world view and it would resolve the Rust case, the live patch CONFIG_LTO_CLANG world too. Thoughts? Luis
