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: <Zof1PiQOI0kex_xY@krava>
Date: Fri, 5 Jul 2024 15:29:34 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: 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>,
	Masami Hiramatsu <mhiramat@...nel.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, Jul 03, 2024 at 02:48:28PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 3, 2024 at 10:13 AM Jiri Olsa <olsajiri@...il.com> wrote:
> >
> > On Tue, Jul 02, 2024 at 01:51:28PM -0700, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > >  #ifdef CONFIG_UPROBES
> > > > @@ -80,6 +83,12 @@ struct uprobe_task {
> > > >         unsigned int                    depth;
> > > >  };
> > > >
> > > > +struct session_consumer {
> > > > +       __u64           cookie;
> > > > +       unsigned int    id;
> > > > +       int             rc;
> > >
> > > you'll be using u64 for ID, right? so this struct will be 24 bytes.
> >
> > yes
> >
> > > Maybe we can just use topmost bit of ID to store whether uretprobe
> > > should run or not? It's trivial to mask out during ID comparisons
> >
> > actually.. I think we could store just consumers that need to be
> > executed in return probe so there will be no need for 'rc' value
> 
> ah, nice idea. NULL would mean we have session uprobe, but for this
> particular run we "disabled" uretprobe part of it. Great. And for
> non-session uprobes we just won't have session_consumer at all, right?

hm, I think we don't need to add both session or non-session consumer
if it's not supposed to run.. let's see

> 
> [...]
> 
> > > > +static struct session_consumer *
> > > > +session_consumer_next(struct return_instance *ri, struct session_consumer *sc,
> > > > +                     int session_id)
> > > > +{
> > > > +       struct session_consumer *next;
> > > > +
> > > > +       next = sc ? sc + 1 : &ri->sessions[0];
> > > > +       next->id = session_id;
> > >
> > > it's kind of unexpected that "session_consumer_next" would actually
> > > set an ID... Maybe drop int session_id as input argument and fill it
> > > out outside of this function, this function being just a simple
> > > iterator?
> >
> > yea, I was going back and forth on what to have in that function
> > or not, to keep the change minimal, but makes sense, will move
> >
> 
> great, thanks
> 
> > >
> > > > +       return next;
> > > > +}
> > > > +
> 
> [...]
> 
> > >
> > > > +               } else if (uc->ret_handler) {
> > > >                         need_prep = true;
> > > > +               }
> > > >
> > > >                 remove &= rc;
> > > >         }
> > > >
> > > > +       /* no removal if there's at least one session consumer */
> > > > +       remove &= !uprobe->sessions_cnt;
> > >
> > > this is counter (not error, not pointer), let's stick to ` == 0`, please
> > >
> > > is this
> > >
> > > if (uprobe->sessions_cnt != 0)
> > >    remove = 0;
> >
> > yes ;-) will change
> >
> 
> Thanks, I feel bad for being the only one to call this out, but I find
> all these '!<some_integer_variable>` constructs extremely unintuitive
> and hard to reason about quickly. It's only pointers and error cases
> that are more or less intuitive. Everything else, including
> !strcmp(...) is just mind bending and exhausting... Perhaps I'm just
> not a kernel engineer enough :)

heh I was going for minimal change.. but it's intrusive enough already,
so let's keep it at least readable

jirka