linux-kernel - Re: [PATCH v4 4/8] uprobes: travers uprobe's consumer list locklessly under SRCU protection

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: <20240831172543.GB9683@redhat.com>
Date: Sat, 31 Aug 2024 19:25:44 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Jiri Olsa <olsajiri@...il.com>,
	Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org,
	peterz@...radead.org, rostedt@...dmis.org, mhiramat@...nel.org,
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
	paulmck@...nel.org, willy@...radead.org, surenb@...gle.com,
	akpm@...ux-foundation.org, linux-mm@...ck.org
Subject: Re: [PATCH v4 4/8] uprobes: travers uprobe's consumer list
 locklessly under SRCU protection

On 08/30, Jiri Olsa wrote:
>
> with this change the probe will not get removed in the attached test,
> it'll get 2 hits, without this change just 1 hit

Thanks again for pointing out the subtle change in behaviour, but could
you add more details for me? ;)

I was going to read the test below today, but no. As I said many times
I know nothing about bpf, I simply can't understand what this test-case
actually do in kernel-space.

According to git grep, the only in kernel user of UPROBE_HANDLER_REMOVE
is uprobe_perf_func(), but if it returns UPROBE_HANDLER_REMOVE then
consumer->filter == uprobe_perf_filter() should return false?

So could you explay how/why exactly this changes affects your test-case?

But perhaps it uses bpf_uprobe_multi_link_attach() and ->handler is
uprobe_multi_link_handler() ? But uprobe_prog_run() returns zero if
current->mm != link->task->mm.

OTOH, otherwise it returns the error code from bpf_prog_run() and this looks
confusing to me. I have no idea what prog->bpf_func(ctx, insnsi) can return
in this case, but note the WARN(rc & ~UPROBE_HANDLER_MASK) in handler_chain...

Hmm... looking at your test-case again,

> +SEC("uprobe.multi//proc/self/exe:uprobe_multi_func_1")
> +int uprobe(struct pt_regs *ctx)
> +{
> +	test++;
> +	return 1;
> +}

So may be this (compiled to ebpf) is what prog->bpf_func() actually executes?
If yes, everything is clear. And this "proves" that the patch makes the current
API less flexible, as I mentioned in my reply to Andrii.

If I got it right, I'd suggest to add a comment into this code to explain
that we return UPROBE_HANDLER_REMOVE after the 1st hit, for git-grep.

Oleg.