On 09/09, Jiri Olsa wrote:
>
> static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> {
> struct uprobe_consumer *uc;
> int remove = UPROBE_HANDLER_REMOVE;
> - bool need_prep = false; /* prepare return uprobe, when needed */
> + struct return_consumer *ric = NULL;
> + struct return_instance *ri = NULL;
> bool has_consumers = false;
>
> current->utask->auprobe = &uprobe->arch;
>
> list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
> srcu_read_lock_held(&uprobes_srcu)) {
> + __u64 cookie = 0;
> int rc = 0;
>
> if (uc->handler) {
> - rc = uc->handler(uc, regs);
> - WARN(rc & ~UPROBE_HANDLER_MASK,
> + rc = uc->handler(uc, regs, &cookie);
> + WARN(rc < 0 || rc > 2,
> "bad rc=0x%x from %ps()\n", rc, uc->handler);
> }
>
> - if (uc->ret_handler)
> - need_prep = true;
> -
> + /*
> + * The handler can return following values:
> + * 0 - execute ret_handler (if it's defined)
> + * 1 - remove uprobe
> + * 2 - do nothing (ignore ret_handler)
> + */
> remove &= rc;
> has_consumers = true;
> +
> + if (rc == 0 && uc->ret_handler) {
should we enter this block if uc->handler == NULL?
> + /*
> + * Preallocate return_instance object optimistically with
> + * all possible consumers, so we allocate just once.
> + */
> + if (!ri) {
> + ri = alloc_return_instance(uprobe->consumers_cnt);
This doesn't look right...
Suppose we have a single consumer C1, so uprobe->consumers_cnt == 1 and
alloc_return_instance() allocates return_instance with for a single consumer,
so that only ri->consumers[0] is valid.
Right after that uprobe_register()->consumer_add() adds another consumer
C2 with ->ret_handler != NULL.
On the next iteration return_consumer_next() will return the invalid addr
== &ri->consumers[1].
perhaps this needs krealloc() ?
> + if (!ri)
> + return;
Not sure we should simply return if kzalloc fails... at least it would be better
to clear current->utask->auprobe.
> + if (ri && !remove)
> + prepare_uretprobe(uprobe, regs, ri); /* put bp at return */
> + else
> + kfree(ri);
Well, if ri != NULL then remove is not possible, afaics... ri != NULL means
that at least one ->handler() returned rc = 0, thus "remove" must be zero.
So it seems you can just do
if (ri)
prepare_uretprobe(...);
Didn't read other parts of your patch yet ;)
Oleg.
