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: <CAEf4BzbfKE1cWWXfWnWN510pai8Aq_W6J-WSLSAyGO_=rZWX_Q@mail.gmail.com>
Date: Wed, 3 Jul 2024 14:48:28 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: 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 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?

[...]

> > > +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 :)

> jirka
>
> >
> > ? I can't tell (honestly), without spending ridiculous amounts of
> > mental resources (for the underlying simplicity of the condition).
>
> SNIP