Re: [PATCHv3 1/7] uprobe: Add support for session consumer

From: Oleg Nesterov
Date: Thu Sep 12 2024 - 12:21:08 EST


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.