On Mon, Jun 24, 2024 at 5:39 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > On Mon, 24 Jun 2024 13:32:35 -0700 > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > On Mon, Jun 17, 2024 at 3:37 PM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > On Tue, Jun 4, 2024 at 10:16 AM Andrii Nakryiko > > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > > > On Tue, Jun 4, 2024 at 7:13 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > > > > > > > > > On Tue, 21 May 2024 18:38:43 -0700 > > > > > Andrii Nakryiko <andrii@xxxxxxxxxx> wrote: > > > > > > > > > > > When kernel has pending uretprobes installed, it hijacks original user > > > > > > function return address on the stack with a uretprobe trampoline > > > > > > address. There could be multiple such pending uretprobes (either on > > > > > > different user functions or on the same recursive one) at any given > > > > > > time within the same task. > > > > > > > > > > > > This approach interferes with the user stack trace capture logic, which > > > > > > would report suprising addresses (like 0x7fffffffe000) that correspond > > > > > > to a special "[uprobes]" section that kernel installs in the target > > > > > > process address space for uretprobe trampoline code, while logically it > > > > > > should be an address somewhere within the calling function of another > > > > > > traced user function. > > > > > > > > > > > > This is easy to correct for, though. Uprobes subsystem keeps track of > > > > > > pending uretprobes and records original return addresses. This patch is > > > > > > using this to do a post-processing step and restore each trampoline > > > > > > address entries with correct original return address. This is done only > > > > > > if there are pending uretprobes for current task. > > > > > > > > > > > > This is a similar approach to what fprobe/kretprobe infrastructure is > > > > > > doing when capturing kernel stack traces in the presence of pending > > > > > > return probes. > > > > > > > > > > > > > > > > This looks good to me because this trampoline information is only > > > > > managed in uprobes. And it should be provided when unwinding user > > > > > stack. > > > > > > > > > > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> > > > > > > > > > > Thank you! > > > > > > > > Great, thanks for reviewing, Masami! > > > > > > > > Would you take this fix through your tree, or where should it be routed to? > > > > > > > > > > Ping! What would you like me to do with this patch set? Should I > > > resend it without patch 3 (the one that tries to guess whether we are > > > at the entry to the function?), or did I manage to convince you that > > > this heuristic is OK, given perf's stack trace capturing logic already > > > makes heavy assumption of rbp register being used for frame pointer? > > > > > > Please let me know your preference, I could drop patch 3 and send it > > > separately, if that helps move the main fix forward. Thanks! > > > > Masami, > > > > Another week went by with absolutely no action or reaction from you. > > Is there any way I can help improve the collaboration here? > > OK, if there is no change without [3/4], let me pick the others on Thanks, Masami! Selftest is probably failing (as it expects correct stack trace), but that's ok, we can fix it up once linux-trace-kernel and bpf-next trees converge. > probes/for-next directly. [3/4] I need other x86 maintainer's > comments. And it should be handled by PMU maintainers. Sounds good, I'll repost it separately. Do I need to CC anyone else besides people on this thread already? > > Thanks, > > > > > > I'm preparing more patches for uprobes and about to submit them. If > > each reviewed and ready to be applied patch set has to sit idle for > > multiple weeks for no good reason, we all will soon be lost just plain > > forgetting the context in which the patch was prepared. > > > > Please, prioritize handling patches that are meant to be routed > > through your tree in a more timely fashion. Or propose some > > alternative acceptable arrangement. > > > > Thank you. > > > > > > > > > > > > > > > > Reported-by: Riham Selim <rihams@xxxxxxxx> > > > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > > > > > --- > > > > > > kernel/events/callchain.c | 43 ++++++++++++++++++++++++++++++++++++++- > > > > > > kernel/events/uprobes.c | 9 ++++++++ > > > > > > 2 files changed, 51 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > [...] > > > -- > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
