Re: [RFC bpf-next 01/10] uprobe: Add session callbacks to uprobe_consumer

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Jun 17, 2024 at 03:53:50PM -0700, Andrii Nakryiko wrote:
> On Mon, Jun 10, 2024 at 4:06 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Jun 06, 2024 at 09:52:39AM -0700, Andrii Nakryiko wrote:
> > > On Thu, Jun 6, 2024 at 9:46 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Wed, Jun 05, 2024 at 10:50:11PM +0200, Jiri Olsa wrote:
> > > > > On Wed, Jun 05, 2024 at 07:56:19PM +0200, Oleg Nesterov wrote:
> > > > > > On 06/05, Andrii Nakryiko wrote:
> > > > > > >
> > > > > > > so any such
> > > > > > > limitations will cause problems, issue reports, investigation, etc.
> > > > > >
> > > > > > Agreed...
> > > > > >
> > > > > > > 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.
> 
> >
> > thanks,
> > jirka
> >
> >
> > ---
> > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > index f46e0ca0169c..4e40e8352eac 100644
> > --- a/include/linux/uprobes.h
> > +++ b/include/linux/uprobes.h
> > @@ -34,15 +34,19 @@ enum uprobe_filter_ctx {
> >  };
> >
> >  struct uprobe_consumer {
> > -       int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> > +       int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs,
> > +                       unsigned long *data);
> 
> 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).

ok

> 
> >         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);
> >
> 
> [...]
> 
> >  static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> >  {
> >         struct uprobe_task *n_utask;
> > @@ -1756,11 +1795,11 @@ 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 = alloc_return_instance(o->session_cnt);
> >                 if (!n)
> >                         return -ENOMEM;
> >
> > -               *n = *o;
> > +               memcpy(n, o, ri_size(o->session_cnt));
> >                 get_uprobe(n->uprobe);
> >                 n->next = NULL;
> >
> > @@ -1853,35 +1892,38 @@ 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 struct return_instance *
> > +prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs,
> > +                 struct return_instance *ri, int session_cnt)
> 
> 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

I tried that, but now I think it'd be better if we just let prepare_uretprobe
to allocate (if needed) and free ri in case it fails and do something like:

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

> 
> >  {
> > -       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.

ok I think it's possible, will try that

thanks,
jirka