Re: [PATCH v2] tracing/uprobe: Add missing PID filter for uretprobe

[Oleg Nesterov <oleg@xxxxxxxxxx>]

On 08/28, Jiri Olsa wrote:
>
> On Tue, Aug 27, 2024 at 10:19:26PM +0200, Oleg Nesterov wrote:
> > Hmm. Really? In this case these 2 different consumers will have the different
> > trace_event_call's, so
> >
> > 	// consumer for task 1019
> > 	uretprobe_dispatcher
> > 	  uretprobe_perf_func
> > 	    __uprobe_perf_func
> > 	      perf_tp_event
> >
> > won't store the event because this_cpu_ptr(call->perf_events) should be
> > hlist_empty() on this CPU, the perf_event for task 1019 wasn't scheduled in
> > on this CPU...
>
> I'll double check on that,

Yes, please.

> but because there's no filter for uretprobe
> I think it will be stored under 1018 event
...
> I'm working on bpf selftests for above (uprobe and uprobe_multi paths)

Meanwhile, I decided to try to test this case too ;)

test.c:

	#include <unistd.h>

	int func(int i)
	{
		return i;
	}

	int main(void)
	{
		int i;
		for (i = 0;; ++i) {
			sleep(1);
			func(i);
		}
		return 0;
	}

run_probe.c:

	#include "./include/uapi/linux/perf_event.h"
	#define _GNU_SOURCE
	#include <sys/syscall.h>
	#include <sys/ioctl.h>
	#include <assert.h>
	#include <unistd.h>
	#include <stdlib.h>
	#include <stdio.h>

	// cat /sys/bus/event_source/devices/uprobe/type
	#define UPROBE_TYPE	9

	void run_probe(const char *file, unsigned offset, int pid)
	{
		struct perf_event_attr attr = {
			.type		= UPROBE_TYPE,
			.config		= 1, // ret-probe
			.uprobe_path	= (unsigned long)file,
			.probe_offset	= offset,
			.size		= sizeof(struct perf_event_attr),
		};

		int fd = syscall(__NR_perf_event_open, &attr, pid, 0, -1, 0);
		assert(fd >= 0);

		assert(ioctl(fd, PERF_EVENT_IOC_ENABLE, 0) == 0);

		for (;;)
			pause();
	}

	int main(int argc, const char *argv[])
	{
		int pid = atoi(argv[1]);
		run_probe("./test", 0x536, pid);
		return 0;
	}

Now, with the kernel patch below applied, I do

	$ ./test &
	$ PID1=$!
	$ ./test &
	$ PID2=$!

	$ ./run_probe $PID1 &
	$ ./run_probe $PID2 &

and the kernel prints:

	CHAIN
	trace_uprobe: HANDLER pid=46 consumers_target=46 stored=1
	trace_uprobe: HANDLER pid=46 consumers_target=45 stored=0
	CHAIN
	trace_uprobe: HANDLER pid=45 consumers_target=46 stored=0
	trace_uprobe: HANDLER pid=45 consumers_target=45 stored=1
	CHAIN
	trace_uprobe: HANDLER pid=46 consumers_target=46 stored=1
	trace_uprobe: HANDLER pid=46 consumers_target=45 stored=0
	CHAIN
	trace_uprobe: HANDLER pid=45 consumers_target=46 stored=0
	trace_uprobe: HANDLER pid=45 consumers_target=45 stored=1
	CHAIN
	trace_uprobe: HANDLER pid=46 consumers_target=46 stored=1
	trace_uprobe: HANDLER pid=46 consumers_target=45 stored=0
	CHAIN
	trace_uprobe: HANDLER pid=45 consumers_target=46 stored=0
	trace_uprobe: HANDLER pid=45 consumers_target=45 stored=1
	CHAIN
	trace_uprobe: HANDLER pid=46 consumers_target=46 stored=1
	trace_uprobe: HANDLER pid=46 consumers_target=45 stored=0
	CHAIN
	trace_uprobe: HANDLER pid=45 consumers_target=46 stored=0
	trace_uprobe: HANDLER pid=45 consumers_target=45 stored=1

and so on.

As you can see, perf_trace_buf_submit/etc is never called for the "unfiltered"
consumer, so I still think that perf is fine wrt filtering. But I can be easily
wrong, please check.

Oleg.


diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index acc73c1bc54c..14aa92a78d6d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2150,6 +2150,8 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
 	struct uprobe *uprobe = ri->uprobe;
 	struct uprobe_consumer *uc;
 
+	pr_crit("CHAIN\n");
+
 	rcu_read_lock_trace();
 	list_for_each_entry_rcu(uc, &uprobe->consumers, cons_node, rcu_read_lock_trace_held()) {
 		if (uc->ret_handler)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f7443e996b1b..e4eaa0363742 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1364,7 +1364,7 @@ static bool uprobe_perf_filter(struct uprobe_consumer *uc, struct mm_struct *mm)
 	return ret;
 }
 
-static void __uprobe_perf_func(struct trace_uprobe *tu,
+static int __uprobe_perf_func(struct trace_uprobe *tu,
 			       unsigned long func, struct pt_regs *regs,
 			       struct uprobe_cpu_buffer **ucbp)
 {
@@ -1375,6 +1375,7 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
 	void *data;
 	int size, esize;
 	int rctx;
+	int ret = 0;
 
 #ifdef CONFIG_BPF_EVENTS
 	if (bpf_prog_array_valid(call)) {
@@ -1382,7 +1383,7 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
 
 		ret = bpf_prog_run_array_uprobe(call->prog_array, regs, bpf_prog_run);
 		if (!ret)
-			return;
+			return -1;
 	}
 #endif /* CONFIG_BPF_EVENTS */
 
@@ -1392,12 +1393,13 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
 	size = esize + ucb->dsize;
 	size = ALIGN(size + sizeof(u32), sizeof(u64)) - sizeof(u32);
 	if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough"))
-		return;
+		return -1;
 
 	preempt_disable();
 	head = this_cpu_ptr(call->perf_events);
 	if (hlist_empty(head))
 		goto out;
+	ret = 1;
 
 	entry = perf_trace_buf_alloc(size, NULL, &rctx);
 	if (!entry)
@@ -1421,6 +1423,7 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
 			      head, NULL);
  out:
 	preempt_enable();
+	return ret;
 }
 
 /* uprobe profile handler */
@@ -1439,7 +1442,15 @@ static void uretprobe_perf_func(struct trace_uprobe *tu, unsigned long func,
 				struct pt_regs *regs,
 				struct uprobe_cpu_buffer **ucbp)
 {
-	__uprobe_perf_func(tu, func, regs, ucbp);
+	struct trace_uprobe_filter *filter = tu->tp.event->filter;
+	struct perf_event *event = list_first_entry(&filter->perf_events,
+					struct perf_event, hw.tp_list);
+
+	int r = __uprobe_perf_func(tu, func, regs, ucbp);
+
+	pr_crit("HANDLER pid=%d consumers_target=%d stored=%d\n",
+		current->pid, event->hw.target ? event->hw.target->pid : -1, r);
+
 }
 
 int bpf_get_uprobe_info(const struct perf_event *event, u32 *fd_type,