Message-Id: <20240705173544.9ef034c30ae93c52164ecc1b@kernel.org>
Date: Fri, 5 Jul 2024 17:35:44 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Jiri Olsa <olsajiri@...il.com>, Masami Hiramatsu <mhiramat@...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 Wed, 3 Jul 2024 14:43:27 -0700
Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:

> On Wed, Jul 3, 2024 at 9:09 AM Jiri Olsa <olsajiri@...il.com> wrote:
> >
> > On Tue, Jul 02, 2024 at 05:13:38PM -0700, Andrii Nakryiko wrote:
> > > 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
> >
> > and session_id is set when uprobe is registered and used when
> > return uprobe is executed to find matching uprobe_consumer,
> > plz check handle_uretprobe_chain/session_consumer_find
> >
> > > 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).
> >
> > ok, makes sense
> >
> > >
> > > > 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.
> >
> > hum.. so the current code installs return uprobe if there's ret_handler
> > defined in consumer and also the entry 'handler' needs to return 0
> >
> > if entry 'handler' returns 1 the uprobe is unregistered
> >
> > we could define new return value from 'handler' to 'not execute the
> > 'ret_handler' and have 'handler' return values:
> >
> >   0 - execute 'ret_handler' if defined
> >   1 - remove the uprobe
> >   2 - do NOT execute 'ret_handler'  // this current triggers WARN
> >
> > we could delay the allocation of 'return_instance' until the first
> > consumer returns 0, so there's no perf regression
> >
> > that way we could treat all consumers the same and we wouldn't need
> > the session flag..
> >
> > ok looks like good idea ;-) will try that
> 
> Just please double check that we don't pass through 1 or 2 as a return
> result for BPF uprobes/multi-uprobes, so that we don't have any
> accidental changes of behavior.

Agreed. BTW, even if the uprobe is removed, the ret_handler should be called?
I think both 1 and 2 case, we should skip ret_handler.

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

Hmm, so you mean user will allocate another "data map" and use cookie as
a key to access the data? That is possible but sounds a bit redundant.
If such "data map" allocation is also provided, it is more useful.


Thank you,




-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists