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).
> 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
> {
> - 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;
> +}
> +
[...]
