Message-ID: <CAEf4Bzbn+jky3hb+tUwmDCUgUmgCBxL5Ru_9G5SO3=uTWpi=kA@mail.gmail.com>
Date: Tue, 2 Jul 2024 17:13:38 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Jiri Olsa <jolsa@...nel.org>, Oleg Nesterov <oleg@...hat.com>, 
	Peter Zijlstra <peterz@...radead.org>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org, 
	Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>, 
	John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...omium.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Steven Rostedt <rostedt@...dmis.org>, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCHv2 bpf-next 1/9] uprobe: Add support for session consumer

On Tue, Jul 2, 2024 at 4:55 PM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> Hi Jiri,
>
> On Mon,  1 Jul 2024 18:41:07 +0200
> Jiri Olsa <jolsa@...nel.org> wrote:
>
> > 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 keep count of session consumers for uprobe and allocate session_consumer
> > object for each in return_instance object. This allows us to store
> > return values of 'handler' callbacks and data pointers of shared
> > data between both handlers.
> >
> > 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.
> >
> > The control of 'ret_handler' callback execution is done via return
> > value of the 'handler' callback. If it's 0 we install and execute
> > return uprobe, if it's 1 we do not.
> >
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  include/linux/uprobes.h     |  16 ++++-
> >  kernel/events/uprobes.c     | 129 +++++++++++++++++++++++++++++++++---
> >  kernel/trace/bpf_trace.c    |   6 +-
> >  kernel/trace/trace_uprobe.c |  12 ++--
> >  4 files changed, 144 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > index f46e0ca0169c..903a860a8d01 100644
> > --- a/include/linux/uprobes.h
> > +++ b/include/linux/uprobes.h
> > @@ -34,15 +34,18 @@ 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, __u64 *data);
> >       int (*ret_handler)(struct uprobe_consumer *self,
> >                               unsigned long func,
> > -                             struct pt_regs *regs);
> > +                             struct pt_regs *regs, __u64 *data);
> >       bool (*filter)(struct uprobe_consumer *self,
> >                               enum uprobe_filter_ctx ctx,
> >                               struct mm_struct *mm);
> >
> >       struct uprobe_consumer *next;
> > +
> > +     bool                    session;        /* marks uprobe session consumer */
> > +     unsigned int            session_id;     /* set when uprobe_consumer is registered */
>
> Hmm, why this has both session and session_id?

session is caller's request to establish session semantics. Jiri, I
think it's better to move it higher next to
handler/ret_handler/filter, that's the part of uprobe_consumer struct
which has read-only caller-provided data (I'm adding offset and
ref_ctr_offset there as well).

> I also think we can use the address of uprobe_consumer itself as a unique id.

+1

>
> Also, if we can set session enabled by default, and skip ret_handler by handler's
> return value, it is more simpler. (If handler returns a specific value, skip ret_handler)

you mean derive if it's a session or not by both handler and
ret_handler being set? I guess this works fine for BPF side, because
there we never had them both set. If this doesn't regress others, I
think it's OK. We just need to make sure we don't unnecessarily
allocate session state for consumers that don't set both handler and
ret_handler. That would be a waste.

>
> >  };
> >
> >  #ifdef CONFIG_UPROBES
> > @@ -80,6 +83,12 @@ struct uprobe_task {
> >       unsigned int                    depth;
> >  };
> >
> > +struct session_consumer {
> > +     __u64           cookie;
>
> And this cookie looks not scalable. If we can pass a data to handler, I would like to
> reuse it to pass the target function parameters to ret_handler as kretprobe/fprobe does.
>
>         int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, void *data);
>
> uprobes can collect its uc's required sizes and allocate the memory (shadow stack frame)
> at handler_chain().

The goal here is to keep this simple and fast. I'd prefer to keep it
small and fixed size, if possible. I'm thinking about caching and
reusing return_instance as one of the future optimizations, so if we
can keep this more or less fixed (assuming there is typically not more
than 1 or 2 consumers per uprobe, which seems realistic), this will
provide a way to avoid excessive memory allocations.

>
> > +     unsigned int    id;
> > +     int             rc;
> > +};
> > +
> >  struct return_instance {
> >       struct uprobe           *uprobe;
> >       unsigned long           func;
> > @@ -88,6 +97,9 @@ struct return_instance {
> >       bool                    chained;        /* true, if instance is nested */
> >
> >       struct return_instance  *next;          /* keep as stack */
> > +
> > +     int                     sessions_cnt;
> > +     struct session_consumer sessions[];
>
> In that case, we don't have this array, but
>
>         char data[];
>
> And decode data array, which is a slice of variable length structure;
>
> struct session_consumer {
>         struct uprobe_consumer *uc;
>         char data[];
> };
>
> The size of session_consumer is uc->session_data_size + sizeof(uc).
>
> What would you think?
>
> Thank you,
>
> >  };
> >
> >  enum rp_check {
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 2c83ba776fc7..4da410460f2a 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -63,6 +63,8 @@ struct uprobe {
> >       loff_t                  ref_ctr_offset;
> >       unsigned long           flags;
> >
> > +     unsigned int            sessions_cnt;

[...]

Powered by blists - more mailing lists