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
