Re: [PATCHv4 02/14] uprobe: Add support for session consumer

[Jiri Olsa <olsajiri@gmail.com>]

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