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

On Mon, Sep 09, 2024 at 04:44:09PM -0700, Andrii Nakryiko wrote:

On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa [off-list ref] wrote:

quoted

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

quoted

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>

quoted

 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(-)

[...]

quoted

 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

quoted

+       unsigned int            consumers_cnt;
+

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

right, makes sense

quoted

        /*
         * 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,

[...]

quoted

        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

quoted

        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

quoted

        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);
 }

[...]