[PATCHv3 0/7] uprobe, bpf: Add session support

[PATCHv3 0/7] uprobe, bpf: Add session support

[Jiri Olsa]

hi,
this patchset is adding support for session uprobe attachment and
using it through bpf link for bpf programs.

The session means that the uprobe consumer is executed on entry
and return of probed function with additional control:
  - entry callback can control execution of the return callback
  - entry and return callbacks can share data/cookie

On more details please see patch #1.

It's based on Peter's perf/core:
  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core


v3 changes:
  - rebased on top of Andrii's recent changes in Peter's perf/core [2]
  - I kept the session cookie instead of introducing generic session
    data, because it makes the change much more complicated, I think
    it can be added as a followup if it's needed
  - addressed several small review fixes [Andrii]
  - added uprobe.session.s section and test [Andrii]
  - kept acks from previous version on patches that did not change

thanks,
jirka


---
Jiri Olsa (7):
      uprobe: Add support for session consumer
      bpf: Add support for uprobe multi session attach
      bpf: Add support for uprobe multi session context
      libbpf: Add support for uprobe multi session attach
      selftests/bpf: Add uprobe session test
      selftests/bpf: Add uprobe session cookie test
      selftests/bpf: Add uprobe session recursive test

 include/linux/uprobes.h                                            |  17 +++++++++--
 include/uapi/linux/bpf.h                                           |   1 +
 kernel/bpf/syscall.c                                               |   9 ++++--
 kernel/events/uprobes.c                                            | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 kernel/trace/bpf_trace.c                                           |  59 ++++++++++++++++++++++++++-----------
 kernel/trace/trace_uprobe.c                                        |  12 +++++---
 tools/include/uapi/linux/bpf.h                                     |   1 +
 tools/lib/bpf/bpf.c                                                |   1 +
 tools/lib/bpf/libbpf.c                                             |  51 ++++++++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h                                             |   4 ++-
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c              |   2 +-
 tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c         | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/uprobe_multi_session.c           |  71 ++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/uprobe_multi_session_cookie.c    |  48 ++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c |  44 ++++++++++++++++++++++++++++
 15 files changed, 531 insertions(+), 56 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session.c
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session_cookie.c
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c

[PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

Adding support for uprobe consumer to be defined as session and have
new behaviour for consumer's 'handler' and 'ret_handler' callbacks.

The session means that 'handler' and 'ret_handler' callbacks are
connected in a way that allows to:

  - control execution of 'ret_handler' from 'handler' callback
  - share data between 'handler' and 'ret_handler' callbacks

The session is enabled by setting new 'session' bool field to true
in uprobe_consumer object.

We use return_consumer object to keep track of consumers with
'ret_handler'. This object also carries the shared data between
'handler' and and 'ret_handler' callbacks.

The control of 'ret_handler' callback execution is done via return
value of the 'handler' callback. This patch adds new 'ret_handler'
return value (2) which means to ignore ret_handler callback.

Actions on 'handler' callback return values are now:

  0 - execute ret_handler (if it's defined)
  1 - remove uprobe
  2 - do nothing (ignore ret_handler)

The session concept fits to our common use case where we do filtering
on entry uprobe and based on the result we decide to run the return
uprobe (or not).

It's also convenient to share the data between session callbacks.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/uprobes.h                       |  17 ++-
 kernel/events/uprobes.c                       | 132 ++++++++++++++----
 kernel/trace/bpf_trace.c                      |   6 +-
 kernel/trace/trace_uprobe.c                   |  12 +-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   2 +-
 5 files changed, 133 insertions(+), 36 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 2b294bf1881f..557901e04624 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -24,7 +24,7 @@ struct notifier_block;
 struct page;
 
 #define UPROBE_HANDLER_REMOVE		1
-#define UPROBE_HANDLER_MASK		1
+#define UPROBE_HANDLER_IGNORE		2
 
 #define MAX_URETPROBE_DEPTH		64
 
@@ -37,13 +37,16 @@ struct uprobe_consumer {
 	 * for the current process. If filter() is omitted or returns true,
 	 * UPROBE_HANDLER_REMOVE is effectively ignored.
 	 */
-	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
+	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data);
 	int (*ret_handler)(struct uprobe_consumer *self,
 				unsigned long func,
-				struct pt_regs *regs);
+				struct pt_regs *regs, __u64 *data);
 	bool (*filter)(struct uprobe_consumer *self, struct mm_struct *mm);
 
 	struct list_head cons_node;
+	bool session;
+
+	__u64 id;	/* set when uprobe_consumer is registered */
 };
 
 #ifdef CONFIG_UPROBES
@@ -83,14 +86,22 @@ struct uprobe_task {
 	unsigned int			depth;
 };
 
+struct return_consumer {
+	__u64	cookie;
+	__u64	id;
+};
+
 struct return_instance {
 	struct uprobe		*uprobe;
 	unsigned long		func;
 	unsigned long		stack;		/* stack pointer */
 	unsigned long		orig_ret_vaddr; /* original return address */
 	bool			chained;	/* true, if instance is nested */
+	int			consumers_cnt;
 
 	struct return_instance	*next;		/* keep as stack */
+
+	struct return_consumer	consumers[] __counted_by(consumers_cnt);
 };
 
 enum rp_check {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 4b7e590dc428..9e971f86afdf 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -67,6 +67,8 @@ struct uprobe {
 	loff_t			ref_ctr_offset;
 	unsigned long		flags;
 
+	unsigned int		consumers_cnt;
+
 	/*
 	 * The generic code assumes that it has two members of unknown type
 	 * owned by the arch-specific code:
@@ -826,8 +828,12 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
 
 static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
+	static atomic64_t id;
+
 	down_write(&uprobe->consumer_rwsem);
 	list_add_rcu(&uc->cons_node, &uprobe->consumers);
+	uc->id = (__u64) atomic64_inc_return(&id);
+	uprobe->consumers_cnt++;
 	up_write(&uprobe->consumer_rwsem);
 }
 
@@ -839,6 +845,7 @@ static void consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	down_write(&uprobe->consumer_rwsem);
 	list_del_rcu(&uc->cons_node);
+	uprobe->consumers_cnt--;
 	up_write(&uprobe->consumer_rwsem);
 }
 
@@ -1786,6 +1793,30 @@ static struct uprobe_task *get_utask(void)
 	return current->utask;
 }
 
+static size_t ri_size(int consumers_cnt)
+{
+	struct return_instance *ri;
+
+	return sizeof(*ri) + sizeof(ri->consumers[0]) * consumers_cnt;
+}
+
+static struct return_instance *alloc_return_instance(int consumers_cnt)
+{
+	struct return_instance *ri;
+
+	ri = kzalloc(ri_size(consumers_cnt), GFP_KERNEL);
+	if (ri)
+		ri->consumers_cnt = consumers_cnt;
+	return ri;
+}
+
+static struct return_instance *dup_return_instance(struct return_instance *old)
+{
+	size_t size = ri_size(old->consumers_cnt);
+
+	return kmemdup(old, size, GFP_KERNEL);
+}
+
 static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
 {
 	struct uprobe_task *n_utask;
@@ -1798,11 +1829,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
 
 	p = &n_utask->return_instances;
 	for (o = o_utask->return_instances; o; o = o->next) {
-		n = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
+		n = dup_return_instance(o);
 		if (!n)
 			return -ENOMEM;
 
-		*n = *o;
 		/*
 		 * uprobe's refcnt has to be positive at this point, kept by
 		 * utask->return_instances items; return_instances can't be
@@ -1895,39 +1925,35 @@ static void cleanup_return_instances(struct uprobe_task *utask, bool chained,
 	utask->return_instances = ri;
 }
 
-static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
+static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs,
+			      struct return_instance *ri)
 {
-	struct return_instance *ri;
 	struct uprobe_task *utask;
 	unsigned long orig_ret_vaddr, trampoline_vaddr;
 	bool chained;
 
 	if (!get_xol_area())
-		return;
+		goto free;
 
 	utask = get_utask();
 	if (!utask)
-		return;
+		goto free;
 
 	if (utask->depth >= MAX_URETPROBE_DEPTH) {
 		printk_ratelimited(KERN_INFO "uprobe: omit uretprobe due to"
 				" nestedness limit pid/tgid=%d/%d\n",
 				current->pid, current->tgid);
-		return;
+		goto free;
 	}
 
 	/* we need to bump refcount to store uprobe in utask */
 	if (!try_get_uprobe(uprobe))
-		return;
-
-	ri = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
-	if (!ri)
-		goto fail;
+		goto free;
 
 	trampoline_vaddr = uprobe_get_trampoline_vaddr();
 	orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
 	if (orig_ret_vaddr == -1)
-		goto fail;
+		goto put;
 
 	/* drop the entries invalidated by longjmp() */
 	chained = (orig_ret_vaddr == trampoline_vaddr);
@@ -1945,7 +1971,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
 			 * attack from user-space.
 			 */
 			uprobe_warn(current, "handle tail call");
-			goto fail;
+			goto put;
 		}
 		orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
 	}
@@ -1960,9 +1986,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
 	utask->return_instances = ri;
 
 	return;
-fail:
-	kfree(ri);
+put:
 	put_uprobe(uprobe);
+free:
+	kfree(ri);
 }
 
 /* Prepare to single-step probed instruction out of line. */
@@ -2114,35 +2141,78 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
 	return uprobe;
 }
 
+static struct return_consumer *
+return_consumer_next(struct return_instance *ri, struct return_consumer *ric)
+{
+	return ric ? ric + 1 : &ri->consumers[0];
+}
+
+static struct return_consumer *
+return_consumer_find(struct return_instance *ri, int *iter, int id)
+{
+	struct return_consumer *ric;
+	int idx = *iter;
+
+	for (ric = &ri->consumers[idx]; idx < ri->consumers_cnt; idx++, ric++) {
+		if (ric->id == id) {
+			*iter = idx + 1;
+			return ric;
+		}
+	}
+	return NULL;
+}
+
 static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 {
 	struct uprobe_consumer *uc;
 	int remove = UPROBE_HANDLER_REMOVE;
-	bool need_prep = false; /* prepare return uprobe, when needed */
+	struct return_consumer *ric = NULL;
+	struct return_instance *ri = NULL;
 	bool has_consumers = false;
 
 	current->utask->auprobe = &uprobe->arch;
 
 	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
 				 srcu_read_lock_held(&uprobes_srcu)) {
+		__u64 cookie = 0;
 		int rc = 0;
 
 		if (uc->handler) {
-			rc = uc->handler(uc, regs);
-			WARN(rc & ~UPROBE_HANDLER_MASK,
+			rc = uc->handler(uc, regs, &cookie);
+			WARN(rc < 0 || rc > 2,
 				"bad rc=0x%x from %ps()\n", rc, uc->handler);
 		}
 
-		if (uc->ret_handler)
-			need_prep = true;
-
+		/*
+		 * The handler can return following values:
+		 * 0 - execute ret_handler (if it's defined)
+		 * 1 - remove uprobe
+		 * 2 - do nothing (ignore ret_handler)
+		 */
 		remove &= rc;
 		has_consumers = true;
+
+		if (rc == 0 && uc->ret_handler) {
+			/*
+			 * Preallocate return_instance object optimistically with
+			 * all possible consumers, so we allocate just once.
+			 */
+			if (!ri) {
+				ri = alloc_return_instance(uprobe->consumers_cnt);
+				if (!ri)
+					return;
+			}
+			ric = return_consumer_next(ri, ric);
+			ric->cookie = cookie;
+			ric->id = uc->id;
+		}
 	}
 	current->utask->auprobe = NULL;
 
-	if (need_prep && !remove)
-		prepare_uretprobe(uprobe, regs); /* put bp at return */
+	if (ri && !remove)
+		prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
+	else
+		kfree(ri);
 
 	if (remove && has_consumers) {
 		down_read(&uprobe->register_rwsem);
@@ -2160,15 +2230,25 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 static void
 handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
 {
+	struct return_consumer *ric = NULL;
 	struct uprobe *uprobe = ri->uprobe;
 	struct uprobe_consumer *uc;
-	int srcu_idx;
+	int srcu_idx, iter = 0;
 
 	srcu_idx = srcu_read_lock(&uprobes_srcu);
 	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
 				 srcu_read_lock_held(&uprobes_srcu)) {
+		/*
+		 * If we don't find return consumer, it means uprobe consumer
+		 * was added after we hit uprobe and return consumer did not
+		 * get registered in which case we call the ret_handler only
+		 * if it's not session consumer.
+		 */
+		ric = return_consumer_find(ri, &iter, uc->id);
+		if (!ric && uc->session)
+			continue;
 		if (uc->ret_handler)
-			uc->ret_handler(uc, ri->func, regs);
+			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
 	}
 	srcu_read_unlock(&uprobes_srcu, srcu_idx);
 }
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ac0a01cc8634..de241503c8fb 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3332,7 +3332,8 @@ uprobe_multi_link_filter(struct uprobe_consumer *con, struct mm_struct *mm)
 }
 
 static int
-uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs)
+uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
+			  __u64 *data)
 {
 	struct bpf_uprobe *uprobe;
 
@@ -3341,7 +3342,8 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs)
 }
 
 static int
-uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, struct pt_regs *regs)
+uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, struct pt_regs *regs,
+			      __u64 *data)
 {
 	struct bpf_uprobe *uprobe;
 
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f7443e996b1b..11103dde897b 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -88,9 +88,11 @@ static struct trace_uprobe *to_trace_uprobe(struct dyn_event *ev)
 static int register_uprobe_event(struct trace_uprobe *tu);
 static int unregister_uprobe_event(struct trace_uprobe *tu);
 
-static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
+static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs,
+			     __u64 *data);
 static int uretprobe_dispatcher(struct uprobe_consumer *con,
-				unsigned long func, struct pt_regs *regs);
+				unsigned long func, struct pt_regs *regs,
+				__u64 *data);
 
 #ifdef CONFIG_STACK_GROWSUP
 static unsigned long adjust_stack_addr(unsigned long addr, unsigned int n)
@@ -1500,7 +1502,8 @@ trace_uprobe_register(struct trace_event_call *event, enum trace_reg type,
 	}
 }
 
-static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
+static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs,
+			     __u64 *data)
 {
 	struct trace_uprobe *tu;
 	struct uprobe_dispatch_data udd;
@@ -1530,7 +1533,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 }
 
 static int uretprobe_dispatcher(struct uprobe_consumer *con,
-				unsigned long func, struct pt_regs *regs)
+				unsigned long func, struct pt_regs *regs,
+				__u64 *data)
 {
 	struct trace_uprobe *tu;
 	struct uprobe_dispatch_data udd;
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 1fc16657cf42..e91ff5b285f0 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -421,7 +421,7 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
 
 static int
 uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
-		   struct pt_regs *regs)
+		   struct pt_regs *regs, __u64 *data)
 
 {
 	regs->ax  = 0x12345678deadbeef;
-- 
2.46.0

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Andrii Nakryiko]

On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support for uprobe consumer to be defined as session and have
> new behaviour for consumer's 'handler' and 'ret_handler' callbacks.
>
> The session means that 'handler' and 'ret_handler' callbacks are
> connected in a way that allows to:
>
>   - control execution of 'ret_handler' from 'handler' callback
>   - share data between 'handler' and 'ret_handler' callbacks
>
> The session is enabled by setting new 'session' bool field to true
> in uprobe_consumer object.
>
> We use return_consumer object to keep track of consumers with
> 'ret_handler'. This object also carries the shared data between
> 'handler' and and 'ret_handler' callbacks.

and and

>
> The control of 'ret_handler' callback execution is done via return
> value of the 'handler' callback. This patch adds new 'ret_handler'
> return value (2) which means to ignore ret_handler callback.
>
> Actions on 'handler' callback return values are now:
>
>   0 - execute ret_handler (if it's defined)
>   1 - remove uprobe
>   2 - do nothing (ignore ret_handler)
>
> The session concept fits to our common use case where we do filtering
> on entry uprobe and based on the result we decide to run the return
> uprobe (or not).
>
> It's also convenient to share the data between session callbacks.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Just minor things:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  include/linux/uprobes.h                       |  17 ++-
>  kernel/events/uprobes.c                       | 132 ++++++++++++++----
>  kernel/trace/bpf_trace.c                      |   6 +-
>  kernel/trace/trace_uprobe.c                   |  12 +-
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   2 +-
>  5 files changed, 133 insertions(+), 36 deletions(-)
>

[...]

>  enum rp_check {
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4b7e590dc428..9e971f86afdf 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -67,6 +67,8 @@ struct uprobe {
>         loff_t                  ref_ctr_offset;
>         unsigned long           flags;

we should shorten flags to unsigned int, we use one bit out of it

>
> +       unsigned int            consumers_cnt;
> +

and then this won't increase the size of the struct unnecessarily

>         /*
>          * The generic code assumes that it has two members of unknown type
>          * owned by the arch-specific code:
> @@ -826,8 +828,12 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
>

[...]

>         current->utask->auprobe = NULL;
>
> -       if (need_prep && !remove)
> -               prepare_uretprobe(uprobe, regs); /* put bp at return */
> +       if (ri && !remove)
> +               prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> +       else
> +               kfree(ri);

maybe `else if (ri) kfree(ri)` to avoid unnecessary calls to kfree
when we only have uprobes?

>
>         if (remove && has_consumers) {
>                 down_read(&uprobe->register_rwsem);
> @@ -2160,15 +2230,25 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  static void
>  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
>  {
> +       struct return_consumer *ric = NULL;
>         struct uprobe *uprobe = ri->uprobe;
>         struct uprobe_consumer *uc;
> -       int srcu_idx;
> +       int srcu_idx, iter = 0;

iter -> next_ric_idx  or just ric_idx?

>
>         srcu_idx = srcu_read_lock(&uprobes_srcu);
>         list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
>                                  srcu_read_lock_held(&uprobes_srcu)) {
> +               /*
> +                * If we don't find return consumer, it means uprobe consumer
> +                * was added after we hit uprobe and return consumer did not
> +                * get registered in which case we call the ret_handler only
> +                * if it's not session consumer.
> +                */
> +               ric = return_consumer_find(ri, &iter, uc->id);
> +               if (!ric && uc->session)
> +                       continue;
>                 if (uc->ret_handler)
> -                       uc->ret_handler(uc, ri->func, regs);
> +                       uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
>         }
>         srcu_read_unlock(&uprobes_srcu, srcu_idx);
>  }

[...]

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

On Mon, Sep 09, 2024 at 04:44:09PM -0700, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support for uprobe consumer to be defined as session and have
> > new behaviour for consumer's 'handler' and 'ret_handler' callbacks.
> >
> > The session means that 'handler' and 'ret_handler' callbacks are
> > connected in a way that allows to:
> >
> >   - control execution of 'ret_handler' from 'handler' callback
> >   - share data between 'handler' and 'ret_handler' callbacks
> >
> > The session is enabled by setting new 'session' bool field to true
> > in uprobe_consumer object.
> >
> > We use return_consumer object to keep track of consumers with
> > 'ret_handler'. This object also carries the shared data between
> > 'handler' and and 'ret_handler' callbacks.
> 
> and and

ok

> 
> >
> > The control of 'ret_handler' callback execution is done via return
> > value of the 'handler' callback. This patch adds new 'ret_handler'
> > return value (2) which means to ignore ret_handler callback.
> >
> > Actions on 'handler' callback return values are now:
> >
> >   0 - execute ret_handler (if it's defined)
> >   1 - remove uprobe
> >   2 - do nothing (ignore ret_handler)
> >
> > The session concept fits to our common use case where we do filtering
> > on entry uprobe and based on the result we decide to run the return
> > uprobe (or not).
> >
> > It's also convenient to share the data between session callbacks.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> Just minor things:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  include/linux/uprobes.h                       |  17 ++-
> >  kernel/events/uprobes.c                       | 132 ++++++++++++++----
> >  kernel/trace/bpf_trace.c                      |   6 +-
> >  kernel/trace/trace_uprobe.c                   |  12 +-
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   2 +-
> >  5 files changed, 133 insertions(+), 36 deletions(-)
> >
> 
> [...]
> 
> >  enum rp_check {
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 4b7e590dc428..9e971f86afdf 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -67,6 +67,8 @@ struct uprobe {
> >         loff_t                  ref_ctr_offset;
> >         unsigned long           flags;
> 
> we should shorten flags to unsigned int, we use one bit out of it
> 
> >
> > +       unsigned int            consumers_cnt;
> > +
> 
> and then this won't increase the size of the struct unnecessarily

right, makes sense

> 
> >         /*
> >          * The generic code assumes that it has two members of unknown type
> >          * owned by the arch-specific code:
> > @@ -826,8 +828,12 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
> >
> 
> [...]
> 
> >         current->utask->auprobe = NULL;
> >
> > -       if (need_prep && !remove)
> > -               prepare_uretprobe(uprobe, regs); /* put bp at return */
> > +       if (ri && !remove)
> > +               prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> > +       else
> > +               kfree(ri);
> 
> maybe `else if (ri) kfree(ri)` to avoid unnecessary calls to kfree
> when we only have uprobes?

there's null check in kfree, but it's true that we can skip the
whole call and there's the else condition line already, ok

> 
> >
> >         if (remove && has_consumers) {
> >                 down_read(&uprobe->register_rwsem);
> > @@ -2160,15 +2230,25 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> >  static void
> >  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> >  {
> > +       struct return_consumer *ric = NULL;
> >         struct uprobe *uprobe = ri->uprobe;
> >         struct uprobe_consumer *uc;
> > -       int srcu_idx;
> > +       int srcu_idx, iter = 0;
> 
> iter -> next_ric_idx  or just ric_idx?

sure, ric_idx seems ok to me

thanks,
jirka

> 
> >
> >         srcu_idx = srcu_read_lock(&uprobes_srcu);
> >         list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> >                                  srcu_read_lock_held(&uprobes_srcu)) {
> > +               /*
> > +                * If we don't find return consumer, it means uprobe consumer
> > +                * was added after we hit uprobe and return consumer did not
> > +                * get registered in which case we call the ret_handler only
> > +                * if it's not session consumer.
> > +                */
> > +               ric = return_consumer_find(ri, &iter, uc->id);
> > +               if (!ric && uc->session)
> > +                       continue;
> >                 if (uc->ret_handler)
> > -                       uc->ret_handler(uc, ri->func, regs);
> > +                       uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
> >         }
> >         srcu_read_unlock(&uprobes_srcu, srcu_idx);
> >  }
> 
> [...]

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Masami Hiramatsu]

On Mon,  9 Sep 2024 09:45:48 +0200
Jiri Olsa <jolsa@kernel.org> wrote:

> Adding support for uprobe consumer to be defined as session and have
> new behaviour for consumer's 'handler' and 'ret_handler' callbacks.
> 
> The session means that 'handler' and 'ret_handler' callbacks are
> connected in a way that allows to:
> 
>   - control execution of 'ret_handler' from 'handler' callback
>   - share data between 'handler' and 'ret_handler' callbacks
> 
> The session is enabled by setting new 'session' bool field to true
> in uprobe_consumer object.
> 
> We use return_consumer object to keep track of consumers with
> 'ret_handler'. This object also carries the shared data between
> 'handler' and and 'ret_handler' callbacks.
> 
> The control of 'ret_handler' callback execution is done via return
> value of the 'handler' callback. This patch adds new 'ret_handler'
> return value (2) which means to ignore ret_handler callback.
> 
> Actions on 'handler' callback return values are now:
> 
>   0 - execute ret_handler (if it's defined)
>   1 - remove uprobe
>   2 - do nothing (ignore ret_handler)
> 
> The session concept fits to our common use case where we do filtering
> on entry uprobe and based on the result we decide to run the return
> uprobe (or not).

OK, so this allows uprobes handler to record any input parameter and
access it from ret_handler as fprobe's private entry data, right?

The code looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

trace_uprobe should also support $argN in retuprobe.

https://lore.kernel.org/all/170952365552.229804.224112990211602895.stgit@devnote2/

Thank you,

> 
> It's also convenient to share the data between session callbacks.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/uprobes.h                       |  17 ++-
>  kernel/events/uprobes.c                       | 132 ++++++++++++++----
>  kernel/trace/bpf_trace.c                      |   6 +-
>  kernel/trace/trace_uprobe.c                   |  12 +-
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   2 +-
>  5 files changed, 133 insertions(+), 36 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 2b294bf1881f..557901e04624 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -24,7 +24,7 @@ struct notifier_block;
>  struct page;
>  
>  #define UPROBE_HANDLER_REMOVE		1
> -#define UPROBE_HANDLER_MASK		1
> +#define UPROBE_HANDLER_IGNORE		2
>  
>  #define MAX_URETPROBE_DEPTH		64
>  
> @@ -37,13 +37,16 @@ struct uprobe_consumer {
>  	 * for the current process. If filter() is omitted or returns true,
>  	 * UPROBE_HANDLER_REMOVE is effectively ignored.
>  	 */
> -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> +	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data);
>  	int (*ret_handler)(struct uprobe_consumer *self,
>  				unsigned long func,
> -				struct pt_regs *regs);
> +				struct pt_regs *regs, __u64 *data);
>  	bool (*filter)(struct uprobe_consumer *self, struct mm_struct *mm);
>  
>  	struct list_head cons_node;
> +	bool session;
> +
> +	__u64 id;	/* set when uprobe_consumer is registered */
>  };
>  
>  #ifdef CONFIG_UPROBES
> @@ -83,14 +86,22 @@ struct uprobe_task {
>  	unsigned int			depth;
>  };
>  
> +struct return_consumer {
> +	__u64	cookie;
> +	__u64	id;
> +};
> +
>  struct return_instance {
>  	struct uprobe		*uprobe;
>  	unsigned long		func;
>  	unsigned long		stack;		/* stack pointer */
>  	unsigned long		orig_ret_vaddr; /* original return address */
>  	bool			chained;	/* true, if instance is nested */
> +	int			consumers_cnt;
>  
>  	struct return_instance	*next;		/* keep as stack */
> +
> +	struct return_consumer	consumers[] __counted_by(consumers_cnt);
>  };
>  
>  enum rp_check {
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4b7e590dc428..9e971f86afdf 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -67,6 +67,8 @@ struct uprobe {
>  	loff_t			ref_ctr_offset;
>  	unsigned long		flags;
>  
> +	unsigned int		consumers_cnt;
> +
>  	/*
>  	 * The generic code assumes that it has two members of unknown type
>  	 * owned by the arch-specific code:
> @@ -826,8 +828,12 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
>  
>  static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
> +	static atomic64_t id;
> +
>  	down_write(&uprobe->consumer_rwsem);
>  	list_add_rcu(&uc->cons_node, &uprobe->consumers);
> +	uc->id = (__u64) atomic64_inc_return(&id);
> +	uprobe->consumers_cnt++;
>  	up_write(&uprobe->consumer_rwsem);
>  }
>  
> @@ -839,6 +845,7 @@ static void consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
>  	down_write(&uprobe->consumer_rwsem);
>  	list_del_rcu(&uc->cons_node);
> +	uprobe->consumers_cnt--;
>  	up_write(&uprobe->consumer_rwsem);
>  }
>  
> @@ -1786,6 +1793,30 @@ static struct uprobe_task *get_utask(void)
>  	return current->utask;
>  }
>  
> +static size_t ri_size(int consumers_cnt)
> +{
> +	struct return_instance *ri;
> +
> +	return sizeof(*ri) + sizeof(ri->consumers[0]) * consumers_cnt;
> +}
> +
> +static struct return_instance *alloc_return_instance(int consumers_cnt)
> +{
> +	struct return_instance *ri;
> +
> +	ri = kzalloc(ri_size(consumers_cnt), GFP_KERNEL);
> +	if (ri)
> +		ri->consumers_cnt = consumers_cnt;
> +	return ri;
> +}
> +
> +static struct return_instance *dup_return_instance(struct return_instance *old)
> +{
> +	size_t size = ri_size(old->consumers_cnt);
> +
> +	return kmemdup(old, size, GFP_KERNEL);
> +}
> +
>  static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
>  {
>  	struct uprobe_task *n_utask;
> @@ -1798,11 +1829,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
>  
>  	p = &n_utask->return_instances;
>  	for (o = o_utask->return_instances; o; o = o->next) {
> -		n = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
> +		n = dup_return_instance(o);
>  		if (!n)
>  			return -ENOMEM;
>  
> -		*n = *o;
>  		/*
>  		 * uprobe's refcnt has to be positive at this point, kept by
>  		 * utask->return_instances items; return_instances can't be
> @@ -1895,39 +1925,35 @@ static void cleanup_return_instances(struct uprobe_task *utask, bool chained,
>  	utask->return_instances = ri;
>  }
>  
> -static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs,
> +			      struct return_instance *ri)
>  {
> -	struct return_instance *ri;
>  	struct uprobe_task *utask;
>  	unsigned long orig_ret_vaddr, trampoline_vaddr;
>  	bool chained;
>  
>  	if (!get_xol_area())
> -		return;
> +		goto free;
>  
>  	utask = get_utask();
>  	if (!utask)
> -		return;
> +		goto free;
>  
>  	if (utask->depth >= MAX_URETPROBE_DEPTH) {
>  		printk_ratelimited(KERN_INFO "uprobe: omit uretprobe due to"
>  				" nestedness limit pid/tgid=%d/%d\n",
>  				current->pid, current->tgid);
> -		return;
> +		goto free;
>  	}
>  
>  	/* we need to bump refcount to store uprobe in utask */
>  	if (!try_get_uprobe(uprobe))
> -		return;
> -
> -	ri = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
> -	if (!ri)
> -		goto fail;
> +		goto free;
>  
>  	trampoline_vaddr = uprobe_get_trampoline_vaddr();
>  	orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
>  	if (orig_ret_vaddr == -1)
> -		goto fail;
> +		goto put;
>  
>  	/* drop the entries invalidated by longjmp() */
>  	chained = (orig_ret_vaddr == trampoline_vaddr);
> @@ -1945,7 +1971,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
>  			 * attack from user-space.
>  			 */
>  			uprobe_warn(current, "handle tail call");
> -			goto fail;
> +			goto put;
>  		}
>  		orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
>  	}
> @@ -1960,9 +1986,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
>  	utask->return_instances = ri;
>  
>  	return;
> -fail:
> -	kfree(ri);
> +put:
>  	put_uprobe(uprobe);
> +free:
> +	kfree(ri);
>  }
>  
>  /* Prepare to single-step probed instruction out of line. */
> @@ -2114,35 +2141,78 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
>  	return uprobe;
>  }
>  
> +static struct return_consumer *
> +return_consumer_next(struct return_instance *ri, struct return_consumer *ric)
> +{
> +	return ric ? ric + 1 : &ri->consumers[0];
> +}
> +
> +static struct return_consumer *
> +return_consumer_find(struct return_instance *ri, int *iter, int id)
> +{
> +	struct return_consumer *ric;
> +	int idx = *iter;
> +
> +	for (ric = &ri->consumers[idx]; idx < ri->consumers_cnt; idx++, ric++) {
> +		if (ric->id == id) {
> +			*iter = idx + 1;
> +			return ric;
> +		}
> +	}
> +	return NULL;
> +}
> +
>  static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  {
>  	struct uprobe_consumer *uc;
>  	int remove = UPROBE_HANDLER_REMOVE;
> -	bool need_prep = false; /* prepare return uprobe, when needed */
> +	struct return_consumer *ric = NULL;
> +	struct return_instance *ri = NULL;
>  	bool has_consumers = false;
>  
>  	current->utask->auprobe = &uprobe->arch;
>  
>  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
>  				 srcu_read_lock_held(&uprobes_srcu)) {
> +		__u64 cookie = 0;
>  		int rc = 0;
>  
>  		if (uc->handler) {
> -			rc = uc->handler(uc, regs);
> -			WARN(rc & ~UPROBE_HANDLER_MASK,
> +			rc = uc->handler(uc, regs, &cookie);
> +			WARN(rc < 0 || rc > 2,
>  				"bad rc=0x%x from %ps()\n", rc, uc->handler);
>  		}
>  
> -		if (uc->ret_handler)
> -			need_prep = true;
> -
> +		/*
> +		 * The handler can return following values:
> +		 * 0 - execute ret_handler (if it's defined)
> +		 * 1 - remove uprobe
> +		 * 2 - do nothing (ignore ret_handler)
> +		 */
>  		remove &= rc;
>  		has_consumers = true;
> +
> +		if (rc == 0 && uc->ret_handler) {
> +			/*
> +			 * Preallocate return_instance object optimistically with
> +			 * all possible consumers, so we allocate just once.
> +			 */
> +			if (!ri) {
> +				ri = alloc_return_instance(uprobe->consumers_cnt);
> +				if (!ri)
> +					return;
> +			}
> +			ric = return_consumer_next(ri, ric);
> +			ric->cookie = cookie;
> +			ric->id = uc->id;
> +		}
>  	}
>  	current->utask->auprobe = NULL;
>  
> -	if (need_prep && !remove)
> -		prepare_uretprobe(uprobe, regs); /* put bp at return */
> +	if (ri && !remove)
> +		prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> +	else
> +		kfree(ri);
>  
>  	if (remove && has_consumers) {
>  		down_read(&uprobe->register_rwsem);
> @@ -2160,15 +2230,25 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  static void
>  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
>  {
> +	struct return_consumer *ric = NULL;
>  	struct uprobe *uprobe = ri->uprobe;
>  	struct uprobe_consumer *uc;
> -	int srcu_idx;
> +	int srcu_idx, iter = 0;
>  
>  	srcu_idx = srcu_read_lock(&uprobes_srcu);
>  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
>  				 srcu_read_lock_held(&uprobes_srcu)) {
> +		/*
> +		 * If we don't find return consumer, it means uprobe consumer
> +		 * was added after we hit uprobe and return consumer did not
> +		 * get registered in which case we call the ret_handler only
> +		 * if it's not session consumer.
> +		 */
> +		ric = return_consumer_find(ri, &iter, uc->id);
> +		if (!ric && uc->session)
> +			continue;
>  		if (uc->ret_handler)
> -			uc->ret_handler(uc, ri->func, regs);
> +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
>  	}
>  	srcu_read_unlock(&uprobes_srcu, srcu_idx);
>  }
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ac0a01cc8634..de241503c8fb 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3332,7 +3332,8 @@ uprobe_multi_link_filter(struct uprobe_consumer *con, struct mm_struct *mm)
>  }
>  
>  static int
> -uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs)
> +uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
> +			  __u64 *data)
>  {
>  	struct bpf_uprobe *uprobe;
>  
> @@ -3341,7 +3342,8 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs)
>  }
>  
>  static int
> -uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, struct pt_regs *regs)
> +uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, struct pt_regs *regs,
> +			      __u64 *data)
>  {
>  	struct bpf_uprobe *uprobe;
>  
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index f7443e996b1b..11103dde897b 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -88,9 +88,11 @@ static struct trace_uprobe *to_trace_uprobe(struct dyn_event *ev)
>  static int register_uprobe_event(struct trace_uprobe *tu);
>  static int unregister_uprobe_event(struct trace_uprobe *tu);
>  
> -static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
> +static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs,
> +			     __u64 *data);
>  static int uretprobe_dispatcher(struct uprobe_consumer *con,
> -				unsigned long func, struct pt_regs *regs);
> +				unsigned long func, struct pt_regs *regs,
> +				__u64 *data);
>  
>  #ifdef CONFIG_STACK_GROWSUP
>  static unsigned long adjust_stack_addr(unsigned long addr, unsigned int n)
> @@ -1500,7 +1502,8 @@ trace_uprobe_register(struct trace_event_call *event, enum trace_reg type,
>  	}
>  }
>  
> -static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> +static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs,
> +			     __u64 *data)
>  {
>  	struct trace_uprobe *tu;
>  	struct uprobe_dispatch_data udd;
> @@ -1530,7 +1533,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
>  }
>  
>  static int uretprobe_dispatcher(struct uprobe_consumer *con,
> -				unsigned long func, struct pt_regs *regs)
> +				unsigned long func, struct pt_regs *regs,
> +				__u64 *data)
>  {
>  	struct trace_uprobe *tu;
>  	struct uprobe_dispatch_data udd;
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 1fc16657cf42..e91ff5b285f0 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -421,7 +421,7 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
>  
>  static int
>  uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
> -		   struct pt_regs *regs)
> +		   struct pt_regs *regs, __u64 *data)
>  
>  {
>  	regs->ax  = 0x12345678deadbeef;
> -- 
> 2.46.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

On Tue, Sep 10, 2024 at 11:10:23PM +0900, Masami Hiramatsu wrote:
> On Mon,  9 Sep 2024 09:45:48 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > Adding support for uprobe consumer to be defined as session and have
> > new behaviour for consumer's 'handler' and 'ret_handler' callbacks.
> > 
> > The session means that 'handler' and 'ret_handler' callbacks are
> > connected in a way that allows to:
> > 
> >   - control execution of 'ret_handler' from 'handler' callback
> >   - share data between 'handler' and 'ret_handler' callbacks
> > 
> > The session is enabled by setting new 'session' bool field to true
> > in uprobe_consumer object.
> > 
> > We use return_consumer object to keep track of consumers with
> > 'ret_handler'. This object also carries the shared data between
> > 'handler' and and 'ret_handler' callbacks.
> > 
> > The control of 'ret_handler' callback execution is done via return
> > value of the 'handler' callback. This patch adds new 'ret_handler'
> > return value (2) which means to ignore ret_handler callback.
> > 
> > Actions on 'handler' callback return values are now:
> > 
> >   0 - execute ret_handler (if it's defined)
> >   1 - remove uprobe
> >   2 - do nothing (ignore ret_handler)
> > 
> > The session concept fits to our common use case where we do filtering
> > on entry uprobe and based on the result we decide to run the return
> > uprobe (or not).
> 
> OK, so this allows uprobes handler to record any input parameter and
> access it from ret_handler as fprobe's private entry data, right?

at the moment the shared data is 8 bytes.. I explored the way of having
the generic (configured) sized data and it complicates the code a bit
and we have no use case for that at the moment, so I decided to keep it
simple for now

I mentioned that in the cover email, I think it can be done as follow if
it's needed in future:

  - I kept the session cookie instead of introducing generic session
    data, because it makes the change much more complicated, I think
    it can be added as a followup if it's needed

> 
> The code looks good to me.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> trace_uprobe should also support $argN in retuprobe.

could you please share more details on the use case?

thanks,
jirka

> 
> https://lore.kernel.org/all/170952365552.229804.224112990211602895.stgit@devnote2/
> 
> Thank you,
> 
> > 
> > It's also convenient to share the data between session callbacks.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/uprobes.h                       |  17 ++-
> >  kernel/events/uprobes.c                       | 132 ++++++++++++++----
> >  kernel/trace/bpf_trace.c                      |   6 +-
> >  kernel/trace/trace_uprobe.c                   |  12 +-
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   2 +-
> >  5 files changed, 133 insertions(+), 36 deletions(-)
> > 
> > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > index 2b294bf1881f..557901e04624 100644
> > --- a/include/linux/uprobes.h
> > +++ b/include/linux/uprobes.h
> > @@ -24,7 +24,7 @@ struct notifier_block;
> >  struct page;
> >  
> >  #define UPROBE_HANDLER_REMOVE		1
> > -#define UPROBE_HANDLER_MASK		1
> > +#define UPROBE_HANDLER_IGNORE		2
> >  
> >  #define MAX_URETPROBE_DEPTH		64
> >  
> > @@ -37,13 +37,16 @@ struct uprobe_consumer {
> >  	 * for the current process. If filter() is omitted or returns true,
> >  	 * UPROBE_HANDLER_REMOVE is effectively ignored.
> >  	 */
> > -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> > +	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data);
> >  	int (*ret_handler)(struct uprobe_consumer *self,
> >  				unsigned long func,
> > -				struct pt_regs *regs);
> > +				struct pt_regs *regs, __u64 *data);
> >  	bool (*filter)(struct uprobe_consumer *self, struct mm_struct *mm);
> >  
> >  	struct list_head cons_node;
> > +	bool session;
> > +
> > +	__u64 id;	/* set when uprobe_consumer is registered */
> >  };
> >  
> >  #ifdef CONFIG_UPROBES
> > @@ -83,14 +86,22 @@ struct uprobe_task {
> >  	unsigned int			depth;
> >  };
> >  
> > +struct return_consumer {
> > +	__u64	cookie;
> > +	__u64	id;
> > +};
> > +
> >  struct return_instance {
> >  	struct uprobe		*uprobe;
> >  	unsigned long		func;
> >  	unsigned long		stack;		/* stack pointer */
> >  	unsigned long		orig_ret_vaddr; /* original return address */
> >  	bool			chained;	/* true, if instance is nested */
> > +	int			consumers_cnt;
> >  
> >  	struct return_instance	*next;		/* keep as stack */
> > +
> > +	struct return_consumer	consumers[] __counted_by(consumers_cnt);
> >  };
> >  
> >  enum rp_check {
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 4b7e590dc428..9e971f86afdf 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -67,6 +67,8 @@ struct uprobe {
> >  	loff_t			ref_ctr_offset;
> >  	unsigned long		flags;
> >  
> > +	unsigned int		consumers_cnt;
> > +
> >  	/*
> >  	 * The generic code assumes that it has two members of unknown type
> >  	 * owned by the arch-specific code:
> > @@ -826,8 +828,12 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
> >  
> >  static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
> >  {
> > +	static atomic64_t id;
> > +
> >  	down_write(&uprobe->consumer_rwsem);
> >  	list_add_rcu(&uc->cons_node, &uprobe->consumers);
> > +	uc->id = (__u64) atomic64_inc_return(&id);
> > +	uprobe->consumers_cnt++;
> >  	up_write(&uprobe->consumer_rwsem);
> >  }
> >  
> > @@ -839,6 +845,7 @@ static void consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
> >  {
> >  	down_write(&uprobe->consumer_rwsem);
> >  	list_del_rcu(&uc->cons_node);
> > +	uprobe->consumers_cnt--;
> >  	up_write(&uprobe->consumer_rwsem);
> >  }
> >  
> > @@ -1786,6 +1793,30 @@ static struct uprobe_task *get_utask(void)
> >  	return current->utask;
> >  }
> >  
> > +static size_t ri_size(int consumers_cnt)
> > +{
> > +	struct return_instance *ri;
> > +
> > +	return sizeof(*ri) + sizeof(ri->consumers[0]) * consumers_cnt;
> > +}
> > +
> > +static struct return_instance *alloc_return_instance(int consumers_cnt)
> > +{
> > +	struct return_instance *ri;
> > +
> > +	ri = kzalloc(ri_size(consumers_cnt), GFP_KERNEL);
> > +	if (ri)
> > +		ri->consumers_cnt = consumers_cnt;
> > +	return ri;
> > +}
> > +
> > +static struct return_instance *dup_return_instance(struct return_instance *old)
> > +{
> > +	size_t size = ri_size(old->consumers_cnt);
> > +
> > +	return kmemdup(old, size, GFP_KERNEL);
> > +}
> > +
> >  static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> >  {
> >  	struct uprobe_task *n_utask;
> > @@ -1798,11 +1829,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> >  
> >  	p = &n_utask->return_instances;
> >  	for (o = o_utask->return_instances; o; o = o->next) {
> > -		n = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
> > +		n = dup_return_instance(o);
> >  		if (!n)
> >  			return -ENOMEM;
> >  
> > -		*n = *o;
> >  		/*
> >  		 * uprobe's refcnt has to be positive at this point, kept by
> >  		 * utask->return_instances items; return_instances can't be
> > @@ -1895,39 +1925,35 @@ static void cleanup_return_instances(struct uprobe_task *utask, bool chained,
> >  	utask->return_instances = ri;
> >  }
> >  
> > -static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> > +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs,
> > +			      struct return_instance *ri)
> >  {
> > -	struct return_instance *ri;
> >  	struct uprobe_task *utask;
> >  	unsigned long orig_ret_vaddr, trampoline_vaddr;
> >  	bool chained;
> >  
> >  	if (!get_xol_area())
> > -		return;
> > +		goto free;
> >  
> >  	utask = get_utask();
> >  	if (!utask)
> > -		return;
> > +		goto free;
> >  
> >  	if (utask->depth >= MAX_URETPROBE_DEPTH) {
> >  		printk_ratelimited(KERN_INFO "uprobe: omit uretprobe due to"
> >  				" nestedness limit pid/tgid=%d/%d\n",
> >  				current->pid, current->tgid);
> > -		return;
> > +		goto free;
> >  	}
> >  
> >  	/* we need to bump refcount to store uprobe in utask */
> >  	if (!try_get_uprobe(uprobe))
> > -		return;
> > -
> > -	ri = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
> > -	if (!ri)
> > -		goto fail;
> > +		goto free;
> >  
> >  	trampoline_vaddr = uprobe_get_trampoline_vaddr();
> >  	orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
> >  	if (orig_ret_vaddr == -1)
> > -		goto fail;
> > +		goto put;
> >  
> >  	/* drop the entries invalidated by longjmp() */
> >  	chained = (orig_ret_vaddr == trampoline_vaddr);
> > @@ -1945,7 +1971,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> >  			 * attack from user-space.
> >  			 */
> >  			uprobe_warn(current, "handle tail call");
> > -			goto fail;
> > +			goto put;
> >  		}
> >  		orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
> >  	}
> > @@ -1960,9 +1986,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> >  	utask->return_instances = ri;
> >  
> >  	return;
> > -fail:
> > -	kfree(ri);
> > +put:
> >  	put_uprobe(uprobe);
> > +free:
> > +	kfree(ri);
> >  }
> >  
> >  /* Prepare to single-step probed instruction out of line. */
> > @@ -2114,35 +2141,78 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
> >  	return uprobe;
> >  }
> >  
> > +static struct return_consumer *
> > +return_consumer_next(struct return_instance *ri, struct return_consumer *ric)
> > +{
> > +	return ric ? ric + 1 : &ri->consumers[0];
> > +}
> > +
> > +static struct return_consumer *
> > +return_consumer_find(struct return_instance *ri, int *iter, int id)
> > +{
> > +	struct return_consumer *ric;
> > +	int idx = *iter;
> > +
> > +	for (ric = &ri->consumers[idx]; idx < ri->consumers_cnt; idx++, ric++) {
> > +		if (ric->id == id) {
> > +			*iter = idx + 1;
> > +			return ric;
> > +		}
> > +	}
> > +	return NULL;
> > +}
> > +
> >  static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> >  {
> >  	struct uprobe_consumer *uc;
> >  	int remove = UPROBE_HANDLER_REMOVE;
> > -	bool need_prep = false; /* prepare return uprobe, when needed */
> > +	struct return_consumer *ric = NULL;
> > +	struct return_instance *ri = NULL;
> >  	bool has_consumers = false;
> >  
> >  	current->utask->auprobe = &uprobe->arch;
> >  
> >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > +		__u64 cookie = 0;
> >  		int rc = 0;
> >  
> >  		if (uc->handler) {
> > -			rc = uc->handler(uc, regs);
> > -			WARN(rc & ~UPROBE_HANDLER_MASK,
> > +			rc = uc->handler(uc, regs, &cookie);
> > +			WARN(rc < 0 || rc > 2,
> >  				"bad rc=0x%x from %ps()\n", rc, uc->handler);
> >  		}
> >  
> > -		if (uc->ret_handler)
> > -			need_prep = true;
> > -
> > +		/*
> > +		 * The handler can return following values:
> > +		 * 0 - execute ret_handler (if it's defined)
> > +		 * 1 - remove uprobe
> > +		 * 2 - do nothing (ignore ret_handler)
> > +		 */
> >  		remove &= rc;
> >  		has_consumers = true;
> > +
> > +		if (rc == 0 && uc->ret_handler) {
> > +			/*
> > +			 * Preallocate return_instance object optimistically with
> > +			 * all possible consumers, so we allocate just once.
> > +			 */
> > +			if (!ri) {
> > +				ri = alloc_return_instance(uprobe->consumers_cnt);
> > +				if (!ri)
> > +					return;
> > +			}
> > +			ric = return_consumer_next(ri, ric);
> > +			ric->cookie = cookie;
> > +			ric->id = uc->id;
> > +		}
> >  	}
> >  	current->utask->auprobe = NULL;
> >  
> > -	if (need_prep && !remove)
> > -		prepare_uretprobe(uprobe, regs); /* put bp at return */
> > +	if (ri && !remove)
> > +		prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> > +	else
> > +		kfree(ri);
> >  
> >  	if (remove && has_consumers) {
> >  		down_read(&uprobe->register_rwsem);
> > @@ -2160,15 +2230,25 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> >  static void
> >  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> >  {
> > +	struct return_consumer *ric = NULL;
> >  	struct uprobe *uprobe = ri->uprobe;
> >  	struct uprobe_consumer *uc;
> > -	int srcu_idx;
> > +	int srcu_idx, iter = 0;
> >  
> >  	srcu_idx = srcu_read_lock(&uprobes_srcu);
> >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > +		/*
> > +		 * If we don't find return consumer, it means uprobe consumer
> > +		 * was added after we hit uprobe and return consumer did not
> > +		 * get registered in which case we call the ret_handler only
> > +		 * if it's not session consumer.
> > +		 */
> > +		ric = return_consumer_find(ri, &iter, uc->id);
> > +		if (!ric && uc->session)
> > +			continue;
> >  		if (uc->ret_handler)
> > -			uc->ret_handler(uc, ri->func, regs);
> > +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
> >  	}
> >  	srcu_read_unlock(&uprobes_srcu, srcu_idx);
> >  }
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index ac0a01cc8634..de241503c8fb 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3332,7 +3332,8 @@ uprobe_multi_link_filter(struct uprobe_consumer *con, struct mm_struct *mm)
> >  }
> >  
> >  static int
> > -uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs)
> > +uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
> > +			  __u64 *data)
> >  {
> >  	struct bpf_uprobe *uprobe;
> >  
> > @@ -3341,7 +3342,8 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs)
> >  }
> >  
> >  static int
> > -uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, struct pt_regs *regs)
> > +uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, struct pt_regs *regs,
> > +			      __u64 *data)
> >  {
> >  	struct bpf_uprobe *uprobe;
> >  
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index f7443e996b1b..11103dde897b 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -88,9 +88,11 @@ static struct trace_uprobe *to_trace_uprobe(struct dyn_event *ev)
> >  static int register_uprobe_event(struct trace_uprobe *tu);
> >  static int unregister_uprobe_event(struct trace_uprobe *tu);
> >  
> > -static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
> > +static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs,
> > +			     __u64 *data);
> >  static int uretprobe_dispatcher(struct uprobe_consumer *con,
> > -				unsigned long func, struct pt_regs *regs);
> > +				unsigned long func, struct pt_regs *regs,
> > +				__u64 *data);
> >  
> >  #ifdef CONFIG_STACK_GROWSUP
> >  static unsigned long adjust_stack_addr(unsigned long addr, unsigned int n)
> > @@ -1500,7 +1502,8 @@ trace_uprobe_register(struct trace_event_call *event, enum trace_reg type,
> >  	}
> >  }
> >  
> > -static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> > +static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs,
> > +			     __u64 *data)
> >  {
> >  	struct trace_uprobe *tu;
> >  	struct uprobe_dispatch_data udd;
> > @@ -1530,7 +1533,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> >  }
> >  
> >  static int uretprobe_dispatcher(struct uprobe_consumer *con,
> > -				unsigned long func, struct pt_regs *regs)
> > +				unsigned long func, struct pt_regs *regs,
> > +				__u64 *data)
> >  {
> >  	struct trace_uprobe *tu;
> >  	struct uprobe_dispatch_data udd;
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 1fc16657cf42..e91ff5b285f0 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -421,7 +421,7 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
> >  
> >  static int
> >  uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
> > -		   struct pt_regs *regs)
> > +		   struct pt_regs *regs, __u64 *data)
> >  
> >  {
> >  	regs->ax  = 0x12345678deadbeef;
> > -- 
> > 2.46.0
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/09, Jiri Olsa wrote:
>
>  static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  {
>  	struct uprobe_consumer *uc;
>  	int remove = UPROBE_HANDLER_REMOVE;
> -	bool need_prep = false; /* prepare return uprobe, when needed */
> +	struct return_consumer *ric = NULL;
> +	struct return_instance *ri = NULL;
>  	bool has_consumers = false;
>
>  	current->utask->auprobe = &uprobe->arch;
>
>  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
>  				 srcu_read_lock_held(&uprobes_srcu)) {
> +		__u64 cookie = 0;
>  		int rc = 0;
>
>  		if (uc->handler) {
> -			rc = uc->handler(uc, regs);
> -			WARN(rc & ~UPROBE_HANDLER_MASK,
> +			rc = uc->handler(uc, regs, &cookie);
> +			WARN(rc < 0 || rc > 2,
>  				"bad rc=0x%x from %ps()\n", rc, uc->handler);
>  		}
>
> -		if (uc->ret_handler)
> -			need_prep = true;
> -
> +		/*
> +		 * The handler can return following values:
> +		 * 0 - execute ret_handler (if it's defined)
> +		 * 1 - remove uprobe
> +		 * 2 - do nothing (ignore ret_handler)
> +		 */
>  		remove &= rc;
>  		has_consumers = true;
> +
> +		if (rc == 0 && uc->ret_handler) {

should we enter this block if uc->handler == NULL?

> +			/*
> +			 * Preallocate return_instance object optimistically with
> +			 * all possible consumers, so we allocate just once.
> +			 */
> +			if (!ri) {
> +				ri = alloc_return_instance(uprobe->consumers_cnt);

This doesn't look right...

Suppose we have a single consumer C1, so uprobe->consumers_cnt == 1 and
alloc_return_instance() allocates return_instance with for a single consumer,
so that only ri->consumers[0] is valid.

Right after that uprobe_register()->consumer_add() adds another consumer
C2 with ->ret_handler != NULL.

On the next iteration return_consumer_next() will return the invalid addr
== &ri->consumers[1].

perhaps this needs krealloc() ?

> +				if (!ri)
> +					return;

Not sure we should simply return if kzalloc fails... at least it would be better
to clear current->utask->auprobe.

> +	if (ri && !remove)
> +		prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> +	else
> +		kfree(ri);

Well, if ri != NULL then remove is not possible, afaics... ri != NULL means
that at least one ->handler() returned rc = 0, thus "remove" must be zero.

So it seems you can just do

	if (ri)
		prepare_uretprobe(...);


Didn't read other parts of your patch yet ;)

Oleg.

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

On Thu, Sep 12, 2024 at 06:20:29PM +0200, Oleg Nesterov wrote:
> On 09/09, Jiri Olsa wrote:
> >
> >  static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> >  {
> >  	struct uprobe_consumer *uc;
> >  	int remove = UPROBE_HANDLER_REMOVE;
> > -	bool need_prep = false; /* prepare return uprobe, when needed */
> > +	struct return_consumer *ric = NULL;
> > +	struct return_instance *ri = NULL;
> >  	bool has_consumers = false;
> >
> >  	current->utask->auprobe = &uprobe->arch;
> >
> >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > +		__u64 cookie = 0;
> >  		int rc = 0;
> >
> >  		if (uc->handler) {
> > -			rc = uc->handler(uc, regs);
> > -			WARN(rc & ~UPROBE_HANDLER_MASK,
> > +			rc = uc->handler(uc, regs, &cookie);
> > +			WARN(rc < 0 || rc > 2,
> >  				"bad rc=0x%x from %ps()\n", rc, uc->handler);
> >  		}
> >
> > -		if (uc->ret_handler)
> > -			need_prep = true;
> > -
> > +		/*
> > +		 * The handler can return following values:
> > +		 * 0 - execute ret_handler (if it's defined)
> > +		 * 1 - remove uprobe
> > +		 * 2 - do nothing (ignore ret_handler)
> > +		 */
> >  		remove &= rc;
> >  		has_consumers = true;
> > +
> > +		if (rc == 0 && uc->ret_handler) {
> 
> should we enter this block if uc->handler == NULL?

yes, consumer can have just ret_handler defined

> 
> > +			/*
> > +			 * Preallocate return_instance object optimistically with
> > +			 * all possible consumers, so we allocate just once.
> > +			 */
> > +			if (!ri) {
> > +				ri = alloc_return_instance(uprobe->consumers_cnt);
> 
> This doesn't look right...
> 
> Suppose we have a single consumer C1, so uprobe->consumers_cnt == 1 and
> alloc_return_instance() allocates return_instance with for a single consumer,
> so that only ri->consumers[0] is valid.
> 
> Right after that uprobe_register()->consumer_add() adds another consumer
> C2 with ->ret_handler != NULL.
> 
> On the next iteration return_consumer_next() will return the invalid addr
> == &ri->consumers[1].
> 
> perhaps this needs krealloc() ?

damn.. there used to be a lock ;-) ok, for some reason I thought we are safe
in that list iteration and we are not.. I just made selftest that triggers that

I'm not sure the realloc will help, I feel like we need to allocate return
consumer for each called handler separately to be safe

> 
> > +				if (!ri)
> > +					return;
> 
> Not sure we should simply return if kzalloc fails... at least it would be better
> to clear current->utask->auprobe.
> 
> > +	if (ri && !remove)
> > +		prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> > +	else
> > +		kfree(ri);
> 
> Well, if ri != NULL then remove is not possible, afaics... ri != NULL means
> that at least one ->handler() returned rc = 0, thus "remove" must be zero.
> 
> So it seems you can just do
> 
> 	if (ri)
> 		prepare_uretprobe(...);

true, I think that should be enough

thanks,
jirka

> 
> 
> Didn't read other parts of your patch yet ;)
> 
> Oleg.
> 

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/13, Jiri Olsa wrote:
>
> On Thu, Sep 12, 2024 at 06:20:29PM +0200, Oleg Nesterov wrote:
> > > +
> > > +		if (rc == 0 && uc->ret_handler) {
> >
> > should we enter this block if uc->handler == NULL?
>
> yes, consumer can have just ret_handler defined

Sorry, I meant we do not need to push { cookie, id } into return_instance
if uc->handler == NULL.

And in fact I'd prefer (but won't insist) the new

	UPROBE_HANDLER_I_WANT_MY_COOKIE_PLEASE

return code to make this logic more explicit.

Oleg.

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/13, Jiri Olsa wrote:
>
> I'm not sure the realloc will help, I feel like we need to allocate return
> consumer for each called handler separately to be safe

How about something like the (pseudo) code below? Note that this way
we do not need uprobe->consumers_cnt. Note also that krealloc() should
be unlikely and it checks ksize() before it does another allocation.

Oleg.

static size_t ri_size(int consumers_cnt)
{
	return sizeof(struct return_instance) +
		      sizeof(struct return_consumer) * consumers_cnt;
}

#define DEF_CNT	4	// arbitrary value

static struct return_instance *alloc_return_instance(void)
{
	struct return_instance *ri;

	ri = kzalloc(ri_size(DEF_CNT), GFP_KERNEL);
	if (!ri)
		return ZERO_SIZE_PTR;

	ri->consumers_cnt = DEF_CNT;
	return ri;
}

static struct return_instance *push_id_cookie(struct return_instance *ri, int idx,
						__u64 id, __u64 cookie)
{
	if (unlikely(ri == ZERO_SIZE_PTR))
		return ri;

	if (unlikely(idx >= ri->consumers_cnt)) {
		ri->consumers_cnt += DEF_CNT;
		ri = krealloc(ri, ri_size(ri->consumers_cnt), GFP_KERNEL);
		if (!ri) {
			kfree(ri);
			return ZERO_SIZE_PTR;
		}
	}

	ri->consumers[idx].id = id;
	ri->consumers[idx].cookie = cookie;
	return ri;
}

static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
{
	...
	struct return_instance *ri = NULL;
	int push_idx = 0;

	list_for_each_entry_rcu(uc, &uprobe->consumers, cons_node, rcu_read_lock_trace_held()) {
		__u64 cookie = 0;
		int rc = 0;

		if (uc->handler)
			rc = uc->handler(uc, regs, &cookie);

		remove &= rc;
		has_consumers = true;

		if (!uc->ret_handler || rc == UPROBE_HANDLER_REMOVE || rc == 2)
			continue;

		if (!ri)
			ri = alloc_return_instance();

		// or, better if (rc = UPROBE_HANDLER_I_WANT_MY_COOKIE)
		if (uc->handler))
			ri = push_id_cookie(ri, push_idx++, uc->id, cookie);
	}

	if (!ZERO_OR_NULL_PTR(ri)) {
		ri->consumers_cnt = push_idx;
		prepare_uretprobe(uprobe, regs, ri);
	}

	...
}

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

On Fri, Sep 13, 2024 at 12:57:51PM +0200, Oleg Nesterov wrote:
> On 09/13, Jiri Olsa wrote:
> >
> > I'm not sure the realloc will help, I feel like we need to allocate return
> > consumer for each called handler separately to be safe
> 
> How about something like the (pseudo) code below? Note that this way
> we do not need uprobe->consumers_cnt. Note also that krealloc() should
> be unlikely and it checks ksize() before it does another allocation.
> 
> Oleg.
> 
> static size_t ri_size(int consumers_cnt)
> {
> 	return sizeof(struct return_instance) +
> 		      sizeof(struct return_consumer) * consumers_cnt;
> }
> 
> #define DEF_CNT	4	// arbitrary value
> 
> static struct return_instance *alloc_return_instance(void)
> {
> 	struct return_instance *ri;
> 
> 	ri = kzalloc(ri_size(DEF_CNT), GFP_KERNEL);
> 	if (!ri)
> 		return ZERO_SIZE_PTR;
> 
> 	ri->consumers_cnt = DEF_CNT;
> 	return ri;
> }
> 
> static struct return_instance *push_id_cookie(struct return_instance *ri, int idx,
> 						__u64 id, __u64 cookie)
> {
> 	if (unlikely(ri == ZERO_SIZE_PTR))
> 		return ri;
> 
> 	if (unlikely(idx >= ri->consumers_cnt)) {
> 		ri->consumers_cnt += DEF_CNT;
> 		ri = krealloc(ri, ri_size(ri->consumers_cnt), GFP_KERNEL);
> 		if (!ri) {
> 			kfree(ri);
> 			return ZERO_SIZE_PTR;
> 		}
> 	}
> 
> 	ri->consumers[idx].id = id;
> 	ri->consumers[idx].cookie = cookie;
> 	return ri;
> }
> 
> static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> {
> 	...
> 	struct return_instance *ri = NULL;
> 	int push_idx = 0;
> 
> 	list_for_each_entry_rcu(uc, &uprobe->consumers, cons_node, rcu_read_lock_trace_held()) {
> 		__u64 cookie = 0;
> 		int rc = 0;
> 
> 		if (uc->handler)
> 			rc = uc->handler(uc, regs, &cookie);
> 
> 		remove &= rc;
> 		has_consumers = true;
> 
> 		if (!uc->ret_handler || rc == UPROBE_HANDLER_REMOVE || rc == 2)
> 			continue;
> 
> 		if (!ri)
> 			ri = alloc_return_instance();
> 
> 		// or, better if (rc = UPROBE_HANDLER_I_WANT_MY_COOKIE)
> 		if (uc->handler))
> 			ri = push_id_cookie(ri, push_idx++, uc->id, cookie);
> 	}
> 
> 	if (!ZERO_OR_NULL_PTR(ri)) {

should we rather bail out right after we fail to allocate ri above?

> 		ri->consumers_cnt = push_idx;
> 		prepare_uretprobe(uprobe, regs, ri);
> 	}
> 
> 	...
> }
> 

nice, I like that, will try to to plug it with the rest

thanks,
jirka

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/13, Jiri Olsa wrote:
>
> On Fri, Sep 13, 2024 at 12:57:51PM +0200, Oleg Nesterov wrote:
>
> > static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > {
> > 	...
> > 	struct return_instance *ri = NULL;
> > 	int push_idx = 0;
> >
> > 	list_for_each_entry_rcu(uc, &uprobe->consumers, cons_node, rcu_read_lock_trace_held()) {
> > 		__u64 cookie = 0;
> > 		int rc = 0;
> >
> > 		if (uc->handler)
> > 			rc = uc->handler(uc, regs, &cookie);
> >
> > 		remove &= rc;
> > 		has_consumers = true;
> >
> > 		if (!uc->ret_handler || rc == UPROBE_HANDLER_REMOVE || rc == 2)
> > 			continue;
> >
> > 		if (!ri)
> > 			ri = alloc_return_instance();
> >
> > 		// or, better if (rc = UPROBE_HANDLER_I_WANT_MY_COOKIE)
> > 		if (uc->handler))
> > 			ri = push_id_cookie(ri, push_idx++, uc->id, cookie);
> > 	}
> >
> > 	if (!ZERO_OR_NULL_PTR(ri)) {
>
> should we rather bail out right after we fail to allocate ri above?

I think handler_chain() should call all the ->handler's even if
kzalloc/krealloc fails.

This is close to what the current code does, all the ->handler's are
called even if then later prepare_uretprobe()->kmalloc() fails.

Oleg.

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/09, Jiri Olsa wrote:
>
>  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
>  {
> +	struct return_consumer *ric = NULL;
>  	struct uprobe *uprobe = ri->uprobe;
>  	struct uprobe_consumer *uc;
> -	int srcu_idx;
> +	int srcu_idx, iter = 0;
>
>  	srcu_idx = srcu_read_lock(&uprobes_srcu);
>  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
>  				 srcu_read_lock_held(&uprobes_srcu)) {
> +		/*
> +		 * If we don't find return consumer, it means uprobe consumer
> +		 * was added after we hit uprobe and return consumer did not
> +		 * get registered in which case we call the ret_handler only
> +		 * if it's not session consumer.
> +		 */
> +		ric = return_consumer_find(ri, &iter, uc->id);
> +		if (!ric && uc->session)
> +			continue;
>  		if (uc->ret_handler)
> -			uc->ret_handler(uc, ri->func, regs);
> +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);

So why do we need the new uc->session member and the uc->session above ?

If return_consumer_find() returns NULL, uc->ret_handler(..., NULL) can handle
this case itself?

Oleg.

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

On Thu, Sep 12, 2024 at 06:35:39PM +0200, Oleg Nesterov wrote:
> On 09/09, Jiri Olsa wrote:
> >
> >  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> >  {
> > +	struct return_consumer *ric = NULL;
> >  	struct uprobe *uprobe = ri->uprobe;
> >  	struct uprobe_consumer *uc;
> > -	int srcu_idx;
> > +	int srcu_idx, iter = 0;
> >
> >  	srcu_idx = srcu_read_lock(&uprobes_srcu);
> >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > +		/*
> > +		 * If we don't find return consumer, it means uprobe consumer
> > +		 * was added after we hit uprobe and return consumer did not
> > +		 * get registered in which case we call the ret_handler only
> > +		 * if it's not session consumer.
> > +		 */
> > +		ric = return_consumer_find(ri, &iter, uc->id);
> > +		if (!ric && uc->session)
> > +			continue;
> >  		if (uc->ret_handler)
> > -			uc->ret_handler(uc, ri->func, regs);
> > +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
> 
> So why do we need the new uc->session member and the uc->session above ?
> 
> If return_consumer_find() returns NULL, uc->ret_handler(..., NULL) can handle
> this case itself?

I tried to explain that in the comment above.. we do not want to
execute session ret_handler at all in this case, because its entry
counterpart did not run

jirka

> 
> Oleg.
> 

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/13, Jiri Olsa wrote:
>
> On Thu, Sep 12, 2024 at 06:35:39PM +0200, Oleg Nesterov wrote:
> > >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> > >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > > +		/*
> > > +		 * If we don't find return consumer, it means uprobe consumer
> > > +		 * was added after we hit uprobe and return consumer did not
> > > +		 * get registered in which case we call the ret_handler only
> > > +		 * if it's not session consumer.
> > > +		 */
> > > +		ric = return_consumer_find(ri, &iter, uc->id);
> > > +		if (!ric && uc->session)
> > > +			continue;
> > >  		if (uc->ret_handler)
> > > -			uc->ret_handler(uc, ri->func, regs);
> > > +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
> >
> > So why do we need the new uc->session member and the uc->session above ?
> >
> > If return_consumer_find() returns NULL, uc->ret_handler(..., NULL) can handle
> > this case itself?
>
> I tried to explain that in the comment above.. we do not want to
> execute session ret_handler at all in this case, because its entry
> counterpart did not run

I understand, but the session ret_handler(..., __u64 *data) can simply do

	// my ->handler() didn't run or it didn't return 0
	if (!data)
		return;

at the start?

Oleg.

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Jiri Olsa]

On Fri, Sep 13, 2024 at 11:32:01AM +0200, Oleg Nesterov wrote:
> On 09/13, Jiri Olsa wrote:
> >
> > On Thu, Sep 12, 2024 at 06:35:39PM +0200, Oleg Nesterov wrote:
> > > >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> > > >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > > > +		/*
> > > > +		 * If we don't find return consumer, it means uprobe consumer
> > > > +		 * was added after we hit uprobe and return consumer did not
> > > > +		 * get registered in which case we call the ret_handler only
> > > > +		 * if it's not session consumer.
> > > > +		 */
> > > > +		ric = return_consumer_find(ri, &iter, uc->id);
> > > > +		if (!ric && uc->session)
> > > > +			continue;
> > > >  		if (uc->ret_handler)
> > > > -			uc->ret_handler(uc, ri->func, regs);
> > > > +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
> > >
> > > So why do we need the new uc->session member and the uc->session above ?
> > >
> > > If return_consumer_find() returns NULL, uc->ret_handler(..., NULL) can handle
> > > this case itself?
> >
> > I tried to explain that in the comment above.. we do not want to
> > execute session ret_handler at all in this case, because its entry
> > counterpart did not run
> 
> I understand, but the session ret_handler(..., __u64 *data) can simply do
> 
> 	// my ->handler() didn't run or it didn't return 0
> 	if (!data)
> 		return;
> 
> at the start?

I see, that's actualy the only usage of the 'session' flag, so we could
get rid of it and we'd do above check in uprobe_multi layer.. good idea

thanks,
jirka

Re: [PATCHv3 1/7] uprobe: Add support for session consumer

[Oleg Nesterov]

On 09/09, Jiri Olsa wrote:
>
> @@ -37,13 +37,16 @@ struct uprobe_consumer {
>  	 * for the current process. If filter() is omitted or returns true,
>  	 * UPROBE_HANDLER_REMOVE is effectively ignored.
>  	 */
> -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> +	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data);
>  	int (*ret_handler)(struct uprobe_consumer *self,
>  				unsigned long func,
> -				struct pt_regs *regs);
> +				struct pt_regs *regs, __u64 *data);

And... I won't insist, but I'd suggest to do this in a separate patch
which should also update the current users in bpf_trace.c, trace_uprobe.c
and bpf_testmod.c.

Then it would be easier to review the next "functional" change. But this
is minor, feel free to ignore.


Finally, imo this documentation in handler_chain()

		/*
		 * The handler can return following values:
		 * 0 - execute ret_handler (if it's defined)
		 * 1 - remove uprobe
		 * 2 - do nothing (ignore ret_handler)
		 */

should be moved to uprobes.h and explain UPROBE_HANDLER_REMOVE/IGNORE there.

And note that "remove uprobe" is misleading, it should say something
like "remove the breakpoint from current->mm".

Oleg.

[PATCHv3 2/7] bpf: Add support for uprobe multi session attach

[Jiri Olsa]

Adding support to attach bpf program for entry and return probe
of the same function. This is common use case which at the moment
requires to create two uprobe multi links.

Adding new BPF_TRACE_UPROBE_SESSION attach type that instructs
kernel to attach single link program to both entry and exit probe.

It's possible to control execution of the bpf program on return
probe simply by returning zero or non zero from the entry bpf
program execution to execute or not the bpf program on return
probe respectively.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/syscall.c           |  9 +++++++--
 kernel/trace/bpf_trace.c       | 32 ++++++++++++++++++++++++--------
 tools/include/uapi/linux/bpf.h |  1 +
 tools/lib/bpf/libbpf.c         |  1 +
 5 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 35bcf52dbc65..1d93cb014884 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1116,6 +1116,7 @@ enum bpf_attach_type {
 	BPF_NETKIT_PRIMARY,
 	BPF_NETKIT_PEER,
 	BPF_TRACE_KPROBE_SESSION,
+	BPF_TRACE_UPROBE_SESSION,
 	__MAX_BPF_ATTACH_TYPE
 };
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index bf6c5f685ea2..1347f3000bd0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4049,10 +4049,14 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
 		if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI &&
 		    attach_type != BPF_TRACE_UPROBE_MULTI)
 			return -EINVAL;
+		if (prog->expected_attach_type == BPF_TRACE_UPROBE_SESSION &&
+		    attach_type != BPF_TRACE_UPROBE_SESSION)
+			return -EINVAL;
 		if (attach_type != BPF_PERF_EVENT &&
 		    attach_type != BPF_TRACE_KPROBE_MULTI &&
 		    attach_type != BPF_TRACE_KPROBE_SESSION &&
-		    attach_type != BPF_TRACE_UPROBE_MULTI)
+		    attach_type != BPF_TRACE_UPROBE_MULTI &&
+		    attach_type != BPF_TRACE_UPROBE_SESSION)
 			return -EINVAL;
 		return 0;
 	case BPF_PROG_TYPE_SCHED_CLS:
@@ -5315,7 +5319,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		else if (attr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI ||
 			 attr->link_create.attach_type == BPF_TRACE_KPROBE_SESSION)
 			ret = bpf_kprobe_multi_link_attach(attr, prog);
-		else if (attr->link_create.attach_type == BPF_TRACE_UPROBE_MULTI)
+		else if (attr->link_create.attach_type == BPF_TRACE_UPROBE_MULTI ||
+			 attr->link_create.attach_type == BPF_TRACE_UPROBE_SESSION)
 			ret = bpf_uprobe_multi_link_attach(attr, prog);
 		break;
 	default:
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index de241503c8fb..a433e80771d2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1645,6 +1645,17 @@ static inline bool is_kprobe_session(const struct bpf_prog *prog)
 	return prog->expected_attach_type == BPF_TRACE_KPROBE_SESSION;
 }
 
+static inline bool is_uprobe_multi(const struct bpf_prog *prog)
+{
+	return prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI ||
+	       prog->expected_attach_type == BPF_TRACE_UPROBE_SESSION;
+}
+
+static inline bool is_uprobe_session(const struct bpf_prog *prog)
+{
+	return prog->expected_attach_type == BPF_TRACE_UPROBE_SESSION;
+}
+
 static const struct bpf_func_proto *
 kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1662,13 +1673,13 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	case BPF_FUNC_get_func_ip:
 		if (is_kprobe_multi(prog))
 			return &bpf_get_func_ip_proto_kprobe_multi;
-		if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI)
+		if (is_uprobe_multi(prog))
 			return &bpf_get_func_ip_proto_uprobe_multi;
 		return &bpf_get_func_ip_proto_kprobe;
 	case BPF_FUNC_get_attach_cookie:
 		if (is_kprobe_multi(prog))
 			return &bpf_get_attach_cookie_proto_kmulti;
-		if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI)
+		if (is_uprobe_multi(prog))
 			return &bpf_get_attach_cookie_proto_umulti;
 		return &bpf_get_attach_cookie_proto_trace;
 	default:
@@ -3336,9 +3347,13 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
 			  __u64 *data)
 {
 	struct bpf_uprobe *uprobe;
+	int ret;
 
 	uprobe = container_of(con, struct bpf_uprobe, consumer);
-	return uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
+	ret = uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
+	if (uprobe->consumer.session)
+		return ret ? UPROBE_HANDLER_IGNORE : 0;
+	return ret;
 }
 
 static int
@@ -3387,7 +3402,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (sizeof(u64) != sizeof(void *))
 		return -EOPNOTSUPP;
 
-	if (prog->expected_attach_type != BPF_TRACE_UPROBE_MULTI)
+	if (!is_uprobe_multi(prog))
 		return -EINVAL;
 
 	flags = attr->link_create.uprobe_multi.flags;
@@ -3463,11 +3478,12 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 
 		uprobes[i].link = link;
 
-		if (flags & BPF_F_UPROBE_MULTI_RETURN)
-			uprobes[i].consumer.ret_handler = uprobe_multi_link_ret_handler;
-		else
+		if (!(flags & BPF_F_UPROBE_MULTI_RETURN))
 			uprobes[i].consumer.handler = uprobe_multi_link_handler;
-
+		if (flags & BPF_F_UPROBE_MULTI_RETURN || is_uprobe_session(prog))
+			uprobes[i].consumer.ret_handler = uprobe_multi_link_ret_handler;
+		if (is_uprobe_session(prog))
+			uprobes[i].consumer.session = true;
 		if (pid)
 			uprobes[i].consumer.filter = uprobe_multi_link_filter;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 35bcf52dbc65..1d93cb014884 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1116,6 +1116,7 @@ enum bpf_attach_type {
 	BPF_NETKIT_PRIMARY,
 	BPF_NETKIT_PEER,
 	BPF_TRACE_KPROBE_SESSION,
+	BPF_TRACE_UPROBE_SESSION,
 	__MAX_BPF_ATTACH_TYPE
 };
 
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a3be6f8fac09..274441674f92 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -133,6 +133,7 @@ static const char * const attach_type_name[] = {
 	[BPF_NETKIT_PRIMARY]		= "netkit_primary",
 	[BPF_NETKIT_PEER]		= "netkit_peer",
 	[BPF_TRACE_KPROBE_SESSION]	= "trace_kprobe_session",
+	[BPF_TRACE_UPROBE_SESSION]	= "trace_uprobe_session",
 };
 
 static const char * const link_type_name[] = {
-- 
2.46.0

Re: [PATCHv3 2/7] bpf: Add support for uprobe multi session attach

[Andrii Nakryiko]

On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to attach bpf program for entry and return probe
> of the same function. This is common use case which at the moment
> requires to create two uprobe multi links.
>
> Adding new BPF_TRACE_UPROBE_SESSION attach type that instructs
> kernel to attach single link program to both entry and exit probe.
>
> It's possible to control execution of the bpf program on return
> probe simply by returning zero or non zero from the entry bpf
> program execution to execute or not the bpf program on return
> probe respectively.
>

pedantic nit: bpf -> BPF

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/syscall.c           |  9 +++++++--
>  kernel/trace/bpf_trace.c       | 32 ++++++++++++++++++++++++--------
>  tools/include/uapi/linux/bpf.h |  1 +
>  tools/lib/bpf/libbpf.c         |  1 +
>  5 files changed, 34 insertions(+), 10 deletions(-)
>

LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

[...]

> @@ -3336,9 +3347,13 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
>                           __u64 *data)
>  {
>         struct bpf_uprobe *uprobe;
> +       int ret;
>
>         uprobe = container_of(con, struct bpf_uprobe, consumer);
> -       return uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
> +       ret = uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
> +       if (uprobe->consumer.session)
> +               return ret ? UPROBE_HANDLER_IGNORE : 0;

Should we restrict the return range to [0, 1] for UPROBE_SESSION
programs on the verifier side (given it's a new program type and we
can do that)?

> +       return ret;
>  }
>

[...]

Re: [PATCHv3 2/7] bpf: Add support for uprobe multi session attach

[Jiri Olsa]

On Mon, Sep 09, 2024 at 04:44:29PM -0700, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to attach bpf program for entry and return probe
> > of the same function. This is common use case which at the moment
> > requires to create two uprobe multi links.
> >
> > Adding new BPF_TRACE_UPROBE_SESSION attach type that instructs
> > kernel to attach single link program to both entry and exit probe.
> >
> > It's possible to control execution of the bpf program on return
> > probe simply by returning zero or non zero from the entry bpf
> > program execution to execute or not the bpf program on return
> > probe respectively.
> >
> 
> pedantic nit: bpf -> BPF

ok

> 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/syscall.c           |  9 +++++++--
> >  kernel/trace/bpf_trace.c       | 32 ++++++++++++++++++++++++--------
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  tools/lib/bpf/libbpf.c         |  1 +
> >  5 files changed, 34 insertions(+), 10 deletions(-)
> >
> 
> LGTM
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> [...]
> 
> > @@ -3336,9 +3347,13 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
> >                           __u64 *data)
> >  {
> >         struct bpf_uprobe *uprobe;
> > +       int ret;
> >
> >         uprobe = container_of(con, struct bpf_uprobe, consumer);
> > -       return uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
> > +       ret = uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
> > +       if (uprobe->consumer.session)
> > +               return ret ? UPROBE_HANDLER_IGNORE : 0;
> 
> Should we restrict the return range to [0, 1] for UPROBE_SESSION
> programs on the verifier side (given it's a new program type and we
> can do that)?

yes, I think we can do that.. we have BPF_TRACE_UPROBE_SESSION as
expected_attach_type so we can do that during the load

hum, is it too late to do that for kprobe session as well?

thanks,
jirka

> 
> > +       return ret;
> >  }
> >
> 
> [...]

Re: [PATCHv3 2/7] bpf: Add support for uprobe multi session attach

[Andrii Nakryiko]

On Tue, Sep 10, 2024 at 12:17 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Sep 09, 2024 at 04:44:29PM -0700, Andrii Nakryiko wrote:
> > On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding support to attach bpf program for entry and return probe
> > > of the same function. This is common use case which at the moment
> > > requires to create two uprobe multi links.
> > >
> > > Adding new BPF_TRACE_UPROBE_SESSION attach type that instructs
> > > kernel to attach single link program to both entry and exit probe.
> > >
> > > It's possible to control execution of the bpf program on return
> > > probe simply by returning zero or non zero from the entry bpf
> > > program execution to execute or not the bpf program on return
> > > probe respectively.
> > >
> >
> > pedantic nit: bpf -> BPF
>
> ok
>
> >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  include/uapi/linux/bpf.h       |  1 +
> > >  kernel/bpf/syscall.c           |  9 +++++++--
> > >  kernel/trace/bpf_trace.c       | 32 ++++++++++++++++++++++++--------
> > >  tools/include/uapi/linux/bpf.h |  1 +
> > >  tools/lib/bpf/libbpf.c         |  1 +
> > >  5 files changed, 34 insertions(+), 10 deletions(-)
> > >
> >
> > LGTM
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > [...]
> >
> > > @@ -3336,9 +3347,13 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
> > >                           __u64 *data)
> > >  {
> > >         struct bpf_uprobe *uprobe;
> > > +       int ret;
> > >
> > >         uprobe = container_of(con, struct bpf_uprobe, consumer);
> > > -       return uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
> > > +       ret = uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
> > > +       if (uprobe->consumer.session)
> > > +               return ret ? UPROBE_HANDLER_IGNORE : 0;
> >
> > Should we restrict the return range to [0, 1] for UPROBE_SESSION
> > programs on the verifier side (given it's a new program type and we
> > can do that)?
>
> yes, I think we can do that.. we have BPF_TRACE_UPROBE_SESSION as
> expected_attach_type so we can do that during the load
>
> hum, is it too late to do that for kprobe session as well?

I'd say let's do it, unlikely we'll break anyone. I'd expect everyone
doing explicit return 0 or return 1 anyways.

>
> thanks,
> jirka
>
> >
> > > +       return ret;
> > >  }
> > >
> >
> > [...]

[PATCHv3 3/7] bpf: Add support for uprobe multi session context

[Jiri Olsa]

Placing bpf_session_run_ctx layer in between bpf_run_ctx and
bpf_uprobe_multi_run_ctx, so the session data can be retrieved
from uprobe_multi link.

Plus granting session kfuncs access to uprobe session programs.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a433e80771d2..c8bd0ac98592 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3185,7 +3185,7 @@ struct bpf_uprobe_multi_link {
 };
 
 struct bpf_uprobe_multi_run_ctx {
-	struct bpf_run_ctx run_ctx;
+	struct bpf_session_run_ctx session_ctx;
 	unsigned long entry_ip;
 	struct bpf_uprobe *uprobe;
 };
@@ -3298,10 +3298,15 @@ static const struct bpf_link_ops bpf_uprobe_multi_link_lops = {
 
 static int uprobe_prog_run(struct bpf_uprobe *uprobe,
 			   unsigned long entry_ip,
-			   struct pt_regs *regs)
+			   struct pt_regs *regs,
+			   bool is_return, void *data)
 {
 	struct bpf_uprobe_multi_link *link = uprobe->link;
 	struct bpf_uprobe_multi_run_ctx run_ctx = {
+		.session_ctx = {
+			.is_return = is_return,
+			.data = data,
+		},
 		.entry_ip = entry_ip,
 		.uprobe = uprobe,
 	};
@@ -3320,7 +3325,7 @@ static int uprobe_prog_run(struct bpf_uprobe *uprobe,
 
 	migrate_disable();
 
-	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
+	old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx);
 	err = bpf_prog_run(link->link.prog, regs);
 	bpf_reset_run_ctx(old_run_ctx);
 
@@ -3350,7 +3355,7 @@ uprobe_multi_link_handler(struct uprobe_consumer *con, struct pt_regs *regs,
 	int ret;
 
 	uprobe = container_of(con, struct bpf_uprobe, consumer);
-	ret = uprobe_prog_run(uprobe, instruction_pointer(regs), regs);
+	ret = uprobe_prog_run(uprobe, instruction_pointer(regs), regs, false, data);
 	if (uprobe->consumer.session)
 		return ret ? UPROBE_HANDLER_IGNORE : 0;
 	return ret;
@@ -3363,14 +3368,15 @@ uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, s
 	struct bpf_uprobe *uprobe;
 
 	uprobe = container_of(con, struct bpf_uprobe, consumer);
-	return uprobe_prog_run(uprobe, func, regs);
+	return uprobe_prog_run(uprobe, func, regs, true, data);
 }
 
 static u64 bpf_uprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
 {
 	struct bpf_uprobe_multi_run_ctx *run_ctx;
 
-	run_ctx = container_of(current->bpf_ctx, struct bpf_uprobe_multi_run_ctx, run_ctx);
+	run_ctx = container_of(current->bpf_ctx, struct bpf_uprobe_multi_run_ctx,
+			       session_ctx.run_ctx);
 	return run_ctx->entry_ip;
 }
 
@@ -3378,7 +3384,8 @@ static u64 bpf_uprobe_multi_cookie(struct bpf_run_ctx *ctx)
 {
 	struct bpf_uprobe_multi_run_ctx *run_ctx;
 
-	run_ctx = container_of(current->bpf_ctx, struct bpf_uprobe_multi_run_ctx, run_ctx);
+	run_ctx = container_of(current->bpf_ctx, struct bpf_uprobe_multi_run_ctx,
+			       session_ctx.run_ctx);
 	return run_ctx->uprobe->cookie;
 }
 
@@ -3572,7 +3579,7 @@ static int bpf_kprobe_multi_filter(const struct bpf_prog *prog, u32 kfunc_id)
 	if (!btf_id_set8_contains(&kprobe_multi_kfunc_set_ids, kfunc_id))
 		return 0;
 
-	if (!is_kprobe_session(prog))
+	if (!is_kprobe_session(prog) && !is_uprobe_session(prog))
 		return -EACCES;
 
 	return 0;
-- 
2.46.0

[PATCHv3 4/7] libbpf: Add support for uprobe multi session attach

[Jiri Olsa]

Adding support to attach program in uprobe session mode
with bpf_program__attach_uprobe_multi function.

Adding session bool to bpf_uprobe_multi_opts struct that allows
to load and attach the bpf program via uprobe session.
the attachment to create uprobe multi session.

Also adding new program loader section that allows:
  SEC("uprobe.session/bpf_fentry_test*")

and loads/attaches uprobe program as uprobe session.

Adding sleepable hook (uprobe.session.s) as well.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/bpf.c    |  1 +
 tools/lib/bpf/libbpf.c | 50 ++++++++++++++++++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h |  4 +++-
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 2a4c71501a17..becdfa701c75 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -776,6 +776,7 @@ int bpf_link_create(int prog_fd, int target_fd,
 			return libbpf_err(-EINVAL);
 		break;
 	case BPF_TRACE_UPROBE_MULTI:
+	case BPF_TRACE_UPROBE_SESSION:
 		attr.link_create.uprobe_multi.flags = OPTS_GET(opts, uprobe_multi.flags, 0);
 		attr.link_create.uprobe_multi.cnt = OPTS_GET(opts, uprobe_multi.cnt, 0);
 		attr.link_create.uprobe_multi.path = ptr_to_u64(OPTS_GET(opts, uprobe_multi.path, 0));
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 274441674f92..7b72a29b09a7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9345,6 +9345,7 @@ static int attach_trace(const struct bpf_program *prog, long cookie, struct bpf_
 static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_kprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
+static int attach_uprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_link **link);
 
@@ -9363,8 +9364,10 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("kprobe.session+",	KPROBE,	BPF_TRACE_KPROBE_SESSION, SEC_NONE, attach_kprobe_session),
 	SEC_DEF("uprobe.multi+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
 	SEC_DEF("uretprobe.multi+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
+	SEC_DEF("uprobe.session+",	KPROBE,	BPF_TRACE_UPROBE_SESSION, SEC_NONE, attach_uprobe_session),
 	SEC_DEF("uprobe.multi.s+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_SLEEPABLE, attach_uprobe_multi),
 	SEC_DEF("uretprobe.multi.s+",	KPROBE,	BPF_TRACE_UPROBE_MULTI, SEC_SLEEPABLE, attach_uprobe_multi),
+	SEC_DEF("uprobe.session.s+",	KPROBE,	BPF_TRACE_UPROBE_SESSION, SEC_SLEEPABLE, attach_uprobe_session),
 	SEC_DEF("ksyscall+",		KPROBE,	0, SEC_NONE, attach_ksyscall),
 	SEC_DEF("kretsyscall+",		KPROBE, 0, SEC_NONE, attach_ksyscall),
 	SEC_DEF("usdt+",		KPROBE,	0, SEC_USDT, attach_usdt),
@@ -11699,6 +11702,39 @@ static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, stru
 	return ret;
 }
 
+static int attach_uprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link)
+{
+	LIBBPF_OPTS(bpf_uprobe_multi_opts, opts, .session = true);
+	char *binary_path = NULL, *func_name = NULL;
+	int n, ret = -EINVAL;
+	const char *spec;
+
+	*link = NULL;
+
+	spec = prog->sec_name + sizeof("uprobe.session/") - 1;
+	if (cookie & SEC_SLEEPABLE)
+		spec += 2; /* extra '.s' */
+	n = sscanf(spec, "%m[^:]:%m[^\n]", &binary_path, &func_name);
+
+	switch (n) {
+	case 1:
+		/* but auto-attach is impossible. */
+		ret = 0;
+		break;
+	case 2:
+		*link = bpf_program__attach_uprobe_multi(prog, -1, binary_path, func_name, &opts);
+		ret = *link ? 0 : -errno;
+		break;
+	default:
+		pr_warn("prog '%s': invalid format of section definition '%s'\n", prog->name,
+			prog->sec_name);
+		break;
+	}
+	free(binary_path);
+	free(func_name);
+	return ret;
+}
+
 static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
 					 const char *binary_path, uint64_t offset)
 {
@@ -11933,10 +11969,12 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
 	const unsigned long *ref_ctr_offsets = NULL, *offsets = NULL;
 	LIBBPF_OPTS(bpf_link_create_opts, lopts);
 	unsigned long *resolved_offsets = NULL;
+	enum bpf_attach_type attach_type;
 	int err = 0, link_fd, prog_fd;
 	struct bpf_link *link = NULL;
 	char errmsg[STRERR_BUFSIZE];
 	char full_path[PATH_MAX];
+	bool retprobe, session;
 	const __u64 *cookies;
 	const char **syms;
 	size_t cnt;
@@ -12007,12 +12045,20 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
 		offsets = resolved_offsets;
 	}
 
+	retprobe = OPTS_GET(opts, retprobe, false);
+	session  = OPTS_GET(opts, session, false);
+
+	if (retprobe && session)
+		return libbpf_err_ptr(-EINVAL);
+
+	attach_type = session ? BPF_TRACE_UPROBE_SESSION : BPF_TRACE_UPROBE_MULTI;
+
 	lopts.uprobe_multi.path = path;
 	lopts.uprobe_multi.offsets = offsets;
 	lopts.uprobe_multi.ref_ctr_offsets = ref_ctr_offsets;
 	lopts.uprobe_multi.cookies = cookies;
 	lopts.uprobe_multi.cnt = cnt;
-	lopts.uprobe_multi.flags = OPTS_GET(opts, retprobe, false) ? BPF_F_UPROBE_MULTI_RETURN : 0;
+	lopts.uprobe_multi.flags = retprobe ? BPF_F_UPROBE_MULTI_RETURN : 0;
 
 	if (pid == 0)
 		pid = getpid();
@@ -12026,7 +12072,7 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
 	}
 	link->detach = &bpf_link__detach_fd;
 
-	link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &lopts);
+	link_fd = bpf_link_create(prog_fd, 0, attach_type, &lopts);
 	if (link_fd < 0) {
 		err = -errno;
 		pr_warn("prog '%s': failed to attach multi-uprobe: %s\n",
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 64a6a3d323e3..f6a7835dc519 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -569,10 +569,12 @@ struct bpf_uprobe_multi_opts {
 	size_t cnt;
 	/* create return uprobes */
 	bool retprobe;
+	/* create session kprobes */
+	bool session;
 	size_t :0;
 };
 
-#define bpf_uprobe_multi_opts__last_field retprobe
+#define bpf_uprobe_multi_opts__last_field session
 
 /**
  * @brief **bpf_program__attach_uprobe_multi()** attaches a BPF program
-- 
2.46.0

Re: [PATCHv3 4/7] libbpf: Add support for uprobe multi session attach

[Andrii Nakryiko]

On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to attach program in uprobe session mode
> with bpf_program__attach_uprobe_multi function.
>
> Adding session bool to bpf_uprobe_multi_opts struct that allows
> to load and attach the bpf program via uprobe session.
> the attachment to create uprobe multi session.
>
> Also adding new program loader section that allows:
>   SEC("uprobe.session/bpf_fentry_test*")
>
> and loads/attaches uprobe program as uprobe session.
>
> Adding sleepable hook (uprobe.session.s) as well.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/bpf.c    |  1 +
>  tools/lib/bpf/libbpf.c | 50 ++++++++++++++++++++++++++++++++++++++++--
>  tools/lib/bpf/libbpf.h |  4 +++-
>  3 files changed, 52 insertions(+), 3 deletions(-)
>

[...]

> +static int attach_uprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link)
> +{
> +       LIBBPF_OPTS(bpf_uprobe_multi_opts, opts, .session = true);
> +       char *binary_path = NULL, *func_name = NULL;
> +       int n, ret = -EINVAL;
> +       const char *spec;
> +
> +       *link = NULL;
> +
> +       spec = prog->sec_name + sizeof("uprobe.session/") - 1;
> +       if (cookie & SEC_SLEEPABLE)
> +               spec += 2; /* extra '.s' */
> +       n = sscanf(spec, "%m[^:]:%m[^\n]", &binary_path, &func_name);
> +
> +       switch (n) {
> +       case 1:
> +               /* but auto-attach is impossible. */
> +               ret = 0;
> +               break;
> +       case 2:
> +               *link = bpf_program__attach_uprobe_multi(prog, -1, binary_path, func_name, &opts);
> +               ret = *link ? 0 : -errno;
> +               break;
> +       default:
> +               pr_warn("prog '%s': invalid format of section definition '%s'\n", prog->name,
> +                       prog->sec_name);
> +               break;
> +       }
> +       free(binary_path);
> +       free(func_name);
> +       return ret;
> +}

why adding this whole attach_uprobe_session if attach_uprobe_multi()
is almost exactly what you need. We just need to teach
attach_uprobe_multi to recognize uprobe.session prefix with strncmp(),
no? The rest of the logic is exactly the same.

BTW, maybe you can fix attach_uprobe_multi() while at it:

opts.retprobe = strcmp(probe_type, "uretprobe.multi") == 0;

that should be strncmp() to accommodate uretprobe.multi.s, no?
Original author (wink-wink) didn't account for that ".s", it seems...

(actually please send a small fix to bpf-next separately, so we can
apply it sooner)

> +
>  static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
>                                          const char *binary_path, uint64_t offset)
>  {
> @@ -11933,10 +11969,12 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
>         const unsigned long *ref_ctr_offsets = NULL, *offsets = NULL;
>         LIBBPF_OPTS(bpf_link_create_opts, lopts);
>         unsigned long *resolved_offsets = NULL;
> +       enum bpf_attach_type attach_type;
>         int err = 0, link_fd, prog_fd;
>         struct bpf_link *link = NULL;
>         char errmsg[STRERR_BUFSIZE];
>         char full_path[PATH_MAX];
> +       bool retprobe, session;
>         const __u64 *cookies;
>         const char **syms;
>         size_t cnt;

[...]

Re: [PATCHv3 4/7] libbpf: Add support for uprobe multi session attach

[Jiri Olsa]

On Mon, Sep 09, 2024 at 04:44:44PM -0700, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to attach program in uprobe session mode
> > with bpf_program__attach_uprobe_multi function.
> >
> > Adding session bool to bpf_uprobe_multi_opts struct that allows
> > to load and attach the bpf program via uprobe session.
> > the attachment to create uprobe multi session.
> >
> > Also adding new program loader section that allows:
> >   SEC("uprobe.session/bpf_fentry_test*")
> >
> > and loads/attaches uprobe program as uprobe session.
> >
> > Adding sleepable hook (uprobe.session.s) as well.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/bpf.c    |  1 +
> >  tools/lib/bpf/libbpf.c | 50 ++++++++++++++++++++++++++++++++++++++++--
> >  tools/lib/bpf/libbpf.h |  4 +++-
> >  3 files changed, 52 insertions(+), 3 deletions(-)
> >
> 
> [...]
> 
> > +static int attach_uprobe_session(const struct bpf_program *prog, long cookie, struct bpf_link **link)
> > +{
> > +       LIBBPF_OPTS(bpf_uprobe_multi_opts, opts, .session = true);
> > +       char *binary_path = NULL, *func_name = NULL;
> > +       int n, ret = -EINVAL;
> > +       const char *spec;
> > +
> > +       *link = NULL;
> > +
> > +       spec = prog->sec_name + sizeof("uprobe.session/") - 1;
> > +       if (cookie & SEC_SLEEPABLE)
> > +               spec += 2; /* extra '.s' */
> > +       n = sscanf(spec, "%m[^:]:%m[^\n]", &binary_path, &func_name);
> > +
> > +       switch (n) {
> > +       case 1:
> > +               /* but auto-attach is impossible. */
> > +               ret = 0;
> > +               break;
> > +       case 2:
> > +               *link = bpf_program__attach_uprobe_multi(prog, -1, binary_path, func_name, &opts);
> > +               ret = *link ? 0 : -errno;
> > +               break;
> > +       default:
> > +               pr_warn("prog '%s': invalid format of section definition '%s'\n", prog->name,
> > +                       prog->sec_name);
> > +               break;
> > +       }
> > +       free(binary_path);
> > +       free(func_name);
> > +       return ret;
> > +}
> 
> why adding this whole attach_uprobe_session if attach_uprobe_multi()
> is almost exactly what you need. We just need to teach
> attach_uprobe_multi to recognize uprobe.session prefix with strncmp(),
>  no? The rest of the logic is exactly the same.

ok, that's better

> 
> BTW, maybe you can fix attach_uprobe_multi() while at it:
> 
> opts.retprobe = strcmp(probe_type, "uretprobe.multi") == 0;
> 
> that should be strncmp() to accommodate uretprobe.multi.s, no?
> Original author (wink-wink) didn't account for that ".s", it seems...
> 
> (actually please send a small fix to bpf-next separately, so we can
> apply it sooner)

hum, right.. I wonder why the test is passing, will send a fix

thanks,
jirka

> 
> > +
> >  static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
> >                                          const char *binary_path, uint64_t offset)
> >  {
> > @@ -11933,10 +11969,12 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> >         const unsigned long *ref_ctr_offsets = NULL, *offsets = NULL;
> >         LIBBPF_OPTS(bpf_link_create_opts, lopts);
> >         unsigned long *resolved_offsets = NULL;
> > +       enum bpf_attach_type attach_type;
> >         int err = 0, link_fd, prog_fd;
> >         struct bpf_link *link = NULL;
> >         char errmsg[STRERR_BUFSIZE];
> >         char full_path[PATH_MAX];
> > +       bool retprobe, session;
> >         const __u64 *cookies;
> >         const char **syms;
> >         size_t cnt;
> 
> [...]

[PATCHv3 5/7] selftests/bpf: Add uprobe session test

[Jiri Olsa]

Adding uprobe session test and testing that the entry program
return value controls execution of the return probe program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/uprobe_multi_test.c        | 47 ++++++++++++
 .../bpf/progs/uprobe_multi_session.c          | 71 +++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session.c

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index bf6ca8e3eb13..cc32288bfe26 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -6,6 +6,7 @@
 #include "uprobe_multi.skel.h"
 #include "uprobe_multi_bench.skel.h"
 #include "uprobe_multi_usdt.skel.h"
+#include "uprobe_multi_session.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "testing_helpers.h"
 #include "../sdt.h"
@@ -615,6 +616,50 @@ static void test_link_api(void)
 	__test_link_api(child);
 }
 
+static void test_session_skel_api(void)
+{
+	struct uprobe_multi_session *skel = NULL;
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	struct bpf_link *link = NULL;
+	int err;
+
+	skel = uprobe_multi_session__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "uprobe_multi_session__open_and_load"))
+		goto cleanup;
+
+	skel->bss->pid = getpid();
+	skel->bss->user_ptr = test_data;
+
+	err = uprobe_multi_session__attach(skel);
+	if (!ASSERT_OK(err, " uprobe_multi_session__attach"))
+		goto cleanup;
+
+	/* trigger all probes */
+	skel->bss->uprobe_multi_func_1_addr = (__u64) uprobe_multi_func_1;
+	skel->bss->uprobe_multi_func_2_addr = (__u64) uprobe_multi_func_2;
+	skel->bss->uprobe_multi_func_3_addr = (__u64) uprobe_multi_func_3;
+
+	uprobe_multi_func_1();
+	uprobe_multi_func_2();
+	uprobe_multi_func_3();
+
+	/*
+	 * We expect 2 for uprobe_multi_func_2 because it runs both entry/return probe,
+	 * uprobe_multi_func_[13] run just the entry probe. All expected numbers are
+	 * doubled, because we run extra test for sleepable session.
+	 */
+	ASSERT_EQ(skel->bss->uprobe_session_result[0], 2, "uprobe_multi_func_1_result");
+	ASSERT_EQ(skel->bss->uprobe_session_result[1], 4, "uprobe_multi_func_2_result");
+	ASSERT_EQ(skel->bss->uprobe_session_result[2], 2, "uprobe_multi_func_3_result");
+
+	/* We expect increase in 3 entry and 1 return session calls -> 4 */
+	ASSERT_EQ(skel->bss->uprobe_multi_sleep_result, 4, "uprobe_multi_sleep_result");
+
+cleanup:
+	bpf_link__destroy(link);
+	uprobe_multi_session__destroy(skel);
+}
+
 static void test_bench_attach_uprobe(void)
 {
 	long attach_start_ns = 0, attach_end_ns = 0;
@@ -703,4 +748,6 @@ void test_uprobe_multi_test(void)
 		test_bench_attach_usdt();
 	if (test__start_subtest("attach_api_fails"))
 		test_attach_api_fails();
+	if (test__start_subtest("session"))
+		test_session_skel_api();
 }
diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi_session.c b/tools/testing/selftests/bpf/progs/uprobe_multi_session.c
new file mode 100644
index 000000000000..30bff90b68dc
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_multi_session.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.h>
+#include "bpf_kfuncs.h"
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+__u64 uprobe_multi_func_1_addr = 0;
+__u64 uprobe_multi_func_2_addr = 0;
+__u64 uprobe_multi_func_3_addr = 0;
+
+__u64 uprobe_session_result[3] = {};
+__u64 uprobe_multi_sleep_result = 0;
+
+void *user_ptr = 0;
+int pid = 0;
+
+static int uprobe_multi_check(void *ctx, bool is_return)
+{
+	const __u64 funcs[] = {
+		uprobe_multi_func_1_addr,
+		uprobe_multi_func_2_addr,
+		uprobe_multi_func_3_addr,
+	};
+	unsigned int i;
+	__u64 addr;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 1;
+
+	addr = bpf_get_func_ip(ctx);
+
+	for (i = 0; i < ARRAY_SIZE(funcs); i++) {
+		if (funcs[i] == addr) {
+			uprobe_session_result[i]++;
+			break;
+		}
+	}
+
+	/* only uprobe_multi_func_2 executes return probe */
+	if ((addr == uprobe_multi_func_1_addr) ||
+	    (addr == uprobe_multi_func_3_addr))
+		return 1;
+
+	return 0;
+}
+
+SEC("uprobe.session//proc/self/exe:uprobe_multi_func_*")
+int uprobe(struct pt_regs *ctx)
+{
+	return uprobe_multi_check(ctx, bpf_session_is_return());
+}
+
+static __always_inline bool verify_sleepable_user_copy(void)
+{
+	char data[9];
+
+	bpf_copy_from_user(data, sizeof(data), user_ptr);
+	return bpf_strncmp(data, sizeof(data), "test_data") == 0;
+}
+
+SEC("uprobe.session.s//proc/self/exe:uprobe_multi_func_*")
+int uprobe_sleepable(struct pt_regs *ctx)
+{
+	if (verify_sleepable_user_copy())
+		uprobe_multi_sleep_result++;
+	return uprobe_multi_check(ctx, bpf_session_is_return());
+}
-- 
2.46.0

Re: [PATCHv3 5/7] selftests/bpf: Add uprobe session test

[Andrii Nakryiko]

On Mon, Sep 9, 2024 at 12:47 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding uprobe session test and testing that the entry program
> return value controls execution of the return probe program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../bpf/prog_tests/uprobe_multi_test.c        | 47 ++++++++++++
>  .../bpf/progs/uprobe_multi_session.c          | 71 +++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session.c
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>

(also note Stanislav's change of email, please don't cc his old email)

[...]

Re: [PATCHv3 5/7] selftests/bpf: Add uprobe session test

[Jiri Olsa]

On Mon, Sep 09, 2024 at 04:45:35PM -0700, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 12:47 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding uprobe session test and testing that the entry program
> > return value controls execution of the return probe program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../bpf/prog_tests/uprobe_multi_test.c        | 47 ++++++++++++
> >  .../bpf/progs/uprobe_multi_session.c          | 71 +++++++++++++++++++
> >  2 files changed, 118 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session.c
> >
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> (also note Stanislav's change of email, please don't cc his old email)

yea sorry about that, will change

thanks,
jirka

[PATCHv3 6/7] selftests/bpf: Add uprobe session cookie test

[Jiri Olsa]

Adding uprobe session test that verifies the cookie value
get properly propagated from entry to return program.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/uprobe_multi_test.c        | 31 ++++++++++++
 .../bpf/progs/uprobe_multi_session_cookie.c   | 48 +++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session_cookie.c

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index cc32288bfe26..8f56066a0195 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -7,6 +7,7 @@
 #include "uprobe_multi_bench.skel.h"
 #include "uprobe_multi_usdt.skel.h"
 #include "uprobe_multi_session.skel.h"
+#include "uprobe_multi_session_cookie.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "testing_helpers.h"
 #include "../sdt.h"
@@ -660,6 +661,34 @@ static void test_session_skel_api(void)
 	uprobe_multi_session__destroy(skel);
 }
 
+static void test_session_cookie_skel_api(void)
+{
+	struct uprobe_multi_session_cookie *skel = NULL;
+	int err;
+
+	skel = uprobe_multi_session_cookie__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "uprobe_multi_session_cookie__open_and_load"))
+		goto cleanup;
+
+	skel->bss->pid = getpid();
+
+	err = uprobe_multi_session_cookie__attach(skel);
+	if (!ASSERT_OK(err, "uprobe_multi_session_cookie__attach"))
+		goto cleanup;
+
+	/* trigger all probes */
+	uprobe_multi_func_1();
+	uprobe_multi_func_2();
+	uprobe_multi_func_3();
+
+	ASSERT_EQ(skel->bss->test_uprobe_1_result, 1, "test_uprobe_1_result");
+	ASSERT_EQ(skel->bss->test_uprobe_2_result, 2, "test_uprobe_2_result");
+	ASSERT_EQ(skel->bss->test_uprobe_3_result, 3, "test_uprobe_3_result");
+
+cleanup:
+	uprobe_multi_session_cookie__destroy(skel);
+}
+
 static void test_bench_attach_uprobe(void)
 {
 	long attach_start_ns = 0, attach_end_ns = 0;
@@ -750,4 +779,6 @@ void test_uprobe_multi_test(void)
 		test_attach_api_fails();
 	if (test__start_subtest("session"))
 		test_session_skel_api();
+	if (test__start_subtest("session_cookie"))
+		test_session_cookie_skel_api();
 }
diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi_session_cookie.c b/tools/testing/selftests/bpf/progs/uprobe_multi_session_cookie.c
new file mode 100644
index 000000000000..5befdf944dc6
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_multi_session_cookie.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+int pid = 0;
+
+__u64 test_uprobe_1_result = 0;
+__u64 test_uprobe_2_result = 0;
+__u64 test_uprobe_3_result = 0;
+
+static int check_cookie(__u64 val, __u64 *result)
+{
+	__u64 *cookie;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 1;
+
+	cookie = bpf_session_cookie();
+
+	if (bpf_session_is_return())
+		*result = *cookie == val ? val : 0;
+	else
+		*cookie = val;
+	return 0;
+}
+
+SEC("uprobe.session//proc/self/exe:uprobe_multi_func_1")
+int uprobe_1(struct pt_regs *ctx)
+{
+	return check_cookie(1, &test_uprobe_1_result);
+}
+
+SEC("uprobe.session//proc/self/exe:uprobe_multi_func_2")
+int uprobe_2(struct pt_regs *ctx)
+{
+	return check_cookie(2, &test_uprobe_2_result);
+}
+
+SEC("uprobe.session//proc/self/exe:uprobe_multi_func_3")
+int uprobe_3(struct pt_regs *ctx)
+{
+	return check_cookie(3, &test_uprobe_3_result);
+}
-- 
2.46.0

[PATCHv3 7/7] selftests/bpf: Add uprobe session recursive test

[Jiri Olsa]

Adding uprobe session test that verifies the cookie value is stored
properly when single uprobe-ed function is executed recursively.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/uprobe_multi_test.c        | 57 +++++++++++++++++++
 .../progs/uprobe_multi_session_recursive.c    | 44 ++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index 8f56066a0195..71d1fc00b2f4 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -8,6 +8,7 @@
 #include "uprobe_multi_usdt.skel.h"
 #include "uprobe_multi_session.skel.h"
 #include "uprobe_multi_session_cookie.skel.h"
+#include "uprobe_multi_session_recursive.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "testing_helpers.h"
 #include "../sdt.h"
@@ -34,6 +35,12 @@ noinline void usdt_trigger(void)
 	STAP_PROBE(test, pid_filter_usdt);
 }
 
+noinline void uprobe_session_recursive(int i)
+{
+	if (i)
+		uprobe_session_recursive(i - 1);
+}
+
 struct child {
 	int go[2];
 	int c2p[2]; /* child -> parent channel */
@@ -689,6 +696,54 @@ static void test_session_cookie_skel_api(void)
 	uprobe_multi_session_cookie__destroy(skel);
 }
 
+static void test_session_recursive_skel_api(void)
+{
+	struct uprobe_multi_session_recursive *skel = NULL;
+	int i, err;
+
+	skel = uprobe_multi_session_recursive__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "uprobe_multi_session_recursive__open_and_load"))
+		goto cleanup;
+
+	skel->bss->pid = getpid();
+
+	err = uprobe_multi_session_recursive__attach(skel);
+	if (!ASSERT_OK(err, "uprobe_multi_session_recursive__attach"))
+		goto cleanup;
+
+	for (i = 0; i < ARRAY_SIZE(skel->bss->test_uprobe_cookie_entry); i++)
+		skel->bss->test_uprobe_cookie_entry[i] = i + 1;
+
+	uprobe_session_recursive(5);
+
+	/*
+	 *                                         entry uprobe:
+	 * uprobe_session_recursive(5) {             *cookie = 1, return 0
+	 *   uprobe_session_recursive(4) {           *cookie = 2, return 1
+	 *     uprobe_session_recursive(3) {         *cookie = 3, return 0
+	 *       uprobe_session_recursive(2) {       *cookie = 4, return 1
+	 *         uprobe_session_recursive(1) {     *cookie = 5, return 0
+	 *           uprobe_session_recursive(0) {   *cookie = 6, return 1
+	 *                                          return uprobe:
+	 *           } i = 0                          not executed
+	 *         } i = 1                            test_uprobe_cookie_return[0] = 5
+	 *       } i = 2                              not executed
+	 *     } i = 3                                test_uprobe_cookie_return[1] = 3
+	 *   } i = 4                                  not executed
+	 * } i = 5                                    test_uprobe_cookie_return[2] = 1
+	 */
+
+	ASSERT_EQ(skel->bss->idx_entry, 6, "idx_entry");
+	ASSERT_EQ(skel->bss->idx_return, 3, "idx_return");
+
+	ASSERT_EQ(skel->bss->test_uprobe_cookie_return[0], 5, "test_uprobe_cookie_return[0]");
+	ASSERT_EQ(skel->bss->test_uprobe_cookie_return[1], 3, "test_uprobe_cookie_return[1]");
+	ASSERT_EQ(skel->bss->test_uprobe_cookie_return[2], 1, "test_uprobe_cookie_return[2]");
+
+cleanup:
+	uprobe_multi_session_recursive__destroy(skel);
+}
+
 static void test_bench_attach_uprobe(void)
 {
 	long attach_start_ns = 0, attach_end_ns = 0;
@@ -781,4 +836,6 @@ void test_uprobe_multi_test(void)
 		test_session_skel_api();
 	if (test__start_subtest("session_cookie"))
 		test_session_cookie_skel_api();
+	if (test__start_subtest("session_cookie_recursive"))
+		test_session_recursive_skel_api();
 }
diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c b/tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c
new file mode 100644
index 000000000000..8fbcd69fae22
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <stdbool.h>
+#include "bpf_kfuncs.h"
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+int pid = 0;
+
+int idx_entry = 0;
+int idx_return = 0;
+
+__u64 test_uprobe_cookie_entry[6];
+__u64 test_uprobe_cookie_return[3];
+
+static int check_cookie(void)
+{
+	__u64 *cookie = bpf_session_cookie();
+
+	if (bpf_session_is_return()) {
+		if (idx_return >= ARRAY_SIZE(test_uprobe_cookie_return))
+			return 1;
+		test_uprobe_cookie_return[idx_return++] = *cookie;
+		return 0;
+	}
+
+	if (idx_entry >= ARRAY_SIZE(test_uprobe_cookie_entry))
+		return 1;
+	*cookie = test_uprobe_cookie_entry[idx_entry];
+	return idx_entry++ % 2;
+}
+
+
+SEC("uprobe.session//proc/self/exe:uprobe_session_recursive")
+int uprobe_recursive(struct pt_regs *ctx)
+{
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 1;
+
+	return check_cookie();
+}
-- 
2.46.0