linux-kernel - Re: [PATCHv4 02/14] 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: <20240922152722.GA12833@redhat.com>
Date: Sun, 22 Sep 2024 17:27:23 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: 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@...ichev.me>, 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: [PATCHv4 02/14] uprobe: Add support for session consumer

Damn, sorry for delay :/

And sorry, still can't understand, see below...

On 09/17, Jiri Olsa wrote:
>
> On Tue, Sep 17, 2024 at 02:03:17PM +0200, Oleg Nesterov wrote:
> >
> > To me this code should do:
> >
> > 		if (!uc->ret_handler || UPROBE_HANDLER_REMOVE || UPROBE_HANDLER_IGNORE)
> > 			continue;
> >
> > 		if (!ri)
> > 			ri = alloc_return_instance();
> >
> > 		if (rc == UPROBE_HANDLER_IWANTMYCOOKIE)
> > 			ri = push_consumer(...);
> >
> > And,
> >
> > >  handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> > ...
> > >  	list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> > >  				 srcu_read_lock_held(&uprobes_srcu)) {
> > > +		ric = return_consumer_find(ri, &ric_idx, uc->id);
> > > +		if (ric && ric->rc == UPROBE_HANDLER_IGNORE)
> > > +			continue;
> > >  		if (uc->ret_handler)
> > > -			uc->ret_handler(uc, ri->func, regs);
> > > +			uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
> > >  	}
> >
> > the UPROBE_HANDLER_IGNORE check above and the new ric->rc member should die,
> >
> > 		if (!uc->ret_handler)
> > 			continue;
> >
> > 		ric = return_consumer_find(...);
> > 		uc->ret_handler(..., ric ? &ric->cookie : NULL);
> >
> > as we have already discussed, the session ret_handler(data) can simply do
> >
> > 		// my ->handler() wasn't called or it didn't return
> > 		// UPROBE_HANDLER_IWANTMYCOOKIE
> > 		if (!data)
> > 			return;
> >
> > at the start.
> >
> > Could you explain why this can't work?
>
> I'll try ;-) it's for the case when consumer does not use UPROBE_HANDLER_IWANTMYCOOKIE
>
> let's have 2 consumers on single uprobe, consumer-A returning UPROBE_HANDLER_IGNORE
> and the consumer-B returning zero, so we want the return uprobe installed, but we
> want just consumer-B to be executed
>
>   - so uprobe gets installed and handle_uretprobe_chain goes over all consumers
>     calling ret_handler callback
>
>   - but we don't know consumer-A needs to be ignored, and it does not
>     expect cookie so we have no way to find out it needs to be ignored

How does this differ from the case when consumer-A returns _REMOVE but another
consumer returns 0?

But what I really can't understand is

	and it does not
	expect cookie so we have no way to find out it needs to be ignored

If we change the code as I suggested above, push_consumer() won't be called
if consumer-A returns UPROBE_HANDLER_IGNORE.

This means that handle_uretprobe_chain() -> return_consumer_find() will
return NULL, so handle_uretprobe_chain() won't pass the valid cookie to
consumer-A's ret_handler callback, it will pass data => NULL.

So, again, why can't consumer-A's ret_handler callback do

	// my ->handler() wasn't called or it didn't return
	// UPROBE_HANDLER_IWANTMYCOOKIE
	if (!data)
		return;

at the start?

Why the UPROBE_HANDLER_IGNORE case is more problematic than the
UPROBE_HANDLER_REMOVE case?

Oleg.