On Mon, Jun 10, 2024 at 4:06 AM Jiri Olsa [off-list ref] wrote:

On Thu, Jun 06, 2024 at 09:52:39AM -0700, Andrii Nakryiko wrote:

quoted

On Thu, Jun 6, 2024 at 9:46 AM Jiri Olsa [off-list ref] wrote:

quoted

On Wed, Jun 05, 2024 at 10:50:11PM +0200, Jiri Olsa wrote:

quoted

On Wed, Jun 05, 2024 at 07:56:19PM +0200, Oleg Nesterov wrote:

quoted

On 06/05, Andrii Nakryiko wrote:

quoted

so any such
limitations will cause problems, issue reports, investigation, etc.

Agreed...

quoted

As one possible solution, what if we do

struct return_instance {
    ...
    u64 session_cookies[];
};

and allocate sizeof(struct return_instance) + 8 *
<num-of-session-consumers> and then at runtime pass
&session_cookies[i] as data pointer to session-aware callbacks?

I too thought about this, but I guess it is not that simple.

Just for example. Suppose we have 2 session-consumers C1 and C2.
What if uprobe_unregister(C1) comes before the probed function
returns?

We need something like map_cookie_to_consumer().

I guess we could have hash table in return_instance that gets 'consumer -> cookie' ?

ok, hash table is probably too big for this.. I guess some solution that
would iterate consumers and cookies made sure it matches would be fine

Yes, I was hoping to avoid hash tables for this, and in the common
case have no added overhead.

hi,
here's first stab on that.. the change below:
  - extends current handlers with extra argument rather than adding new
    set of handlers
  - store session consumers objects within return_instance object and
  - iterate these objects ^^^ in handle_uretprobe_chain

I guess it could be still polished, but I wonder if this could
be the right direction to do this.. thoughts? ;-)

Yeah, I think this is the right direction. It's a bit sad that this
makes getting rid of rw_sem on hot path even harder, but that's a
separate problem.

can we use __u64 here? This long vs __u64 might cause problems for BPF
when the host is 32-bit architecture (BPF is always 64-bit).

        int (*ret_handler)(struct uprobe_consumer *self,
                                unsigned long func,
-                               struct pt_regs *regs);
+                               struct pt_regs *regs,
+                               unsigned long *data);
        bool (*filter)(struct uprobe_consumer *self,
                                enum uprobe_filter_ctx ctx,
                                struct mm_struct *mm);

[...]

you have struct uprobe, why do you need to pass session_cnt? Also,
given return_instance is cached, it seems more natural to have

struct return_instance **ri as in/out parameter, and keep the function
itself as void

 {
-       struct return_instance *ri;
        struct uprobe_task *utask;
        unsigned long orig_ret_vaddr, trampoline_vaddr;
        bool chained;

[...]

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

        if (remove && uprobe->consumers) {
                WARN_ON(!uprobe_is_active(uprobe));
                unapply_uprobe(uprobe, current->mm);
        }
+ out:
        up_read(&uprobe->register_rwsem);
 }

+static struct session_consumer *
+consumer_find(struct session_consumer *sc, struct uprobe_consumer *uc)

why can't we keep track of remaining number of session_consumer items
instead of using entire extra entry as a terminating element? Seems
wasteful and unnecessary.

+{
+       for (; sc && sc->id; sc++) {
+               if (sc->id == uc->id)
+                       return sc;
+       }
+       return NULL;
+}
+

[...]