linux-kernel - Re: [PATCHv2 bpf-next 1/9] uprobe: Add support for session consumer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zof3RIqSl6TgaVKq@krava>
Date: Fri, 5 Jul 2024 15:38:12 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>,
	Jiri Olsa <olsajiri@...il.com>, 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 Fri, Jul 05, 2024 at 05:35:44PM +0900, Masami Hiramatsu wrote:

SNIP

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

do you mean what happens when the uretprobe is installed and its consumer
is unregistered before it's triggered?

I think it won't get executed, because the consumer is removed right away,
even if the uprobe object stays because the return_instance holds ref to it

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

is the argument only about the size of the shared data?

we can make it configurable.. for bpf user we will probably stick to
8 bytes to match the kprobe session and to be compatible with existing
helpers accessing the cookie

jirka