Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

On Wed, 7 Aug 2024 00:19:20 +0000
Song Liu [off-list ref] wrote:

quoted

On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu [off-list ref] wrote:

On Tue, 6 Aug 2024 20:12:55 +0000
Song Liu [off-list ref] wrote:

quoted

quoted

On Aug 6, 2024, at 1:01 PM, Steven Rostedt [off-list ref] wrote:

On Tue, 6 Aug 2024 16:00:49 -0400
Steven Rostedt [off-list ref] wrote:

quoted

quoted

quoted

quoted

+ if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr)
+ addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk));
+    

So you do the lookup twice if this is enabled?

Why not just use "kallsyms_lookup_name_without_suffix()" the entire time,
and it should work just the same as "kallsyms_lookup_name()" if it's not
needed?    

We still want to give priority to full match. For example, we have:

[root@~]# grep c_next /proc/kallsyms
ffffffff81419dc0 t c_next.llvm.7567888411731313343
ffffffff81680600 t c_next
ffffffff81854380 t c_next.llvm.14337844803752139461

If the goal is to explicitly trace c_next.llvm.7567888411731313343, the
user can provide the full name. If we always match _without_suffix, all
of the 3 will match to the first one. 

Does this make sense?  

Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked
like you did the command twice.

But that said, does this only have to be for llvm? Or should we do this for
even gcc? As I believe gcc can give strange symbols too.

I think most of the issue comes with LTO, as LTO promotes local static
functions to global functions. IIUC, we don't have GCC built, LTO enabled
kernel yet.

In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", 
and ".isra.0.cold". We didn't do anything about these before this set. So I 
think we are OK not handling them now. We sure can enable it for GCC built
kernel in the future.

Hmm, I think it should be handled as it is. This means it should do as
livepatch does. Since I expected user will check kallsyms if gets error,
we should keep this as it is. (if a symbol has suffix, it should accept
symbol with suffix, or user will get confused because they can not find
which symbol is kprobed.)

Sorry about the conclusion (so I NAK this), but this is a good discussion. 

Do you mean we do not want patch 3/3, but would like to keep 1/3 and part 
of 2/3 (remove the _without_suffix APIs)? If this is the case, we are 
undoing the change by Sami in [1], and thus may break some tracing tools. 

BTW, I confirmed that the PATCH 1/3 and 2/3 fixes kprobes to probe on suffixed
symbols correctly. (because 1/3 allows to search suffixed symbols) 

/sys/kernel/tracing # cat dynamic_events 
p:kprobes/p_c_stop_llvm_17132674095431275852_0 c_stop.llvm.17132674095431275852
p:kprobes/p_c_stop_llvm_8011538628216713357_0 c_stop.llvm.8011538628216713357
p:kprobes/p_c_stop_0 c_stop

Thank you,

Sami, could you please share your thoughts on this? 

If this works, I will send next version with 1/3 and part of 2/3. 

Thanks,
Song

[1] https://lore.kernel.org/all/20210408182843.1754385-8-samitolvanen@google.com/ (local)

-- 
Masami Hiramatsu (Google) [off-list ref]