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: <Zul7UCsftY_ZX6wT@krava>
Date: Tue, 17 Sep 2024 14:51:28 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.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

On Tue, Sep 17, 2024 at 02:03:17PM +0200, Oleg Nesterov wrote:
> I don't see anything wrong after a quick glance, but I don't
> really understand the UPROBE_HANDLER_IGNORE logic, see below.
> 
> On 09/17, Jiri Olsa wrote:
> >
> > + * UPROBE_HANDLER_IWANTMYCOOKIE
> > + * - Store cookie and pass it to ret_handler (if defined).
> 
> Cough ;) yes it was me who used this name in the previous discussion, but maybe
> 
> 	UPROBE_HANDLER_COOKIE
> 
> will look a bit better? Feel free to ignore.

ok, no fun it is..

> 
> >  static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> ...
> > +		if (!uc->ret_handler || rc == UPROBE_HANDLER_REMOVE)
> > +			continue;
> > +
> > +		/*
> > +		 * If alloc_return_instance and push_consumer fail, the return probe
> > +		 * won't be prepared, but we'll finish to execute all entry handlers.
> > +		 *
> > +		 * We need to store handler's return value in case the return uprobe
> > +		 * gets installed and contains consumers that need to be ignored.
> > +		 */
> > +		if (!ri)
> > +			ri = alloc_return_instance();
> > +
> > +		if (rc == UPROBE_HANDLER_IWANTMYCOOKIE || rc == UPROBE_HANDLER_IGNORE)
> > +			ri = push_consumer(ri, push_idx++, uc->id, cookie, rc);
> 
> So this code allocates ri (which implies prepare_uretprobe!) and calls push_consumer()
> even if rc == UPROBE_HANDLER_IGNORE.
> 
> Why? The comment in uprobes.h says:
> 
> 	UPROBE_HANDLER_IGNORE
> 	- Ignore ret_handler callback for this consumer
> 
> but the ret_handler callback won't be ignored?
> 
> 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

the change solves this by storing also return value for consumer

if all consumers ignore the ret_handler callback return uprobe is not installed

jirka