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@xxxxxxxxx> 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@xxxxxxxxx> 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
