Re: [PATCHv3 1/7] uprobe: Add support for session consumer
From: Andrii Nakryiko <hidden>
Date: 2024-09-09 23:44:22
Also in:
bpf, lkml
On Mon, Sep 9, 2024 at 12:46 AM Jiri Olsa [off-list ref] wrote:
Adding support for uprobe consumer to be defined as session and have new behaviour for consumer's 'handler' and 'ret_handler' callbacks. The session means that 'handler' and 'ret_handler' callbacks are connected in a way that allows to: - control execution of 'ret_handler' from 'handler' callback - share data between 'handler' and 'ret_handler' callbacks The session is enabled by setting new 'session' bool field to true in uprobe_consumer object. We use return_consumer object to keep track of consumers with 'ret_handler'. This object also carries the shared data between 'handler' and and 'ret_handler' callbacks.
and and
The control of 'ret_handler' callback execution is done via return value of the 'handler' callback. This patch adds new 'ret_handler' return value (2) which means to ignore ret_handler callback. Actions on 'handler' callback return values are now: 0 - execute ret_handler (if it's defined) 1 - remove uprobe 2 - do nothing (ignore ret_handler) The session concept fits to our common use case where we do filtering on entry uprobe and based on the result we decide to run the return uprobe (or not). It's also convenient to share the data between session callbacks. Signed-off-by: Jiri Olsa <jolsa@kernel.org> ---
Just minor things: Acked-by: Andrii Nakryiko <andrii@kernel.org>
include/linux/uprobes.h | 17 ++- kernel/events/uprobes.c | 132 ++++++++++++++---- kernel/trace/bpf_trace.c | 6 +- kernel/trace/trace_uprobe.c | 12 +- .../selftests/bpf/bpf_testmod/bpf_testmod.c | 2 +- 5 files changed, 133 insertions(+), 36 deletions(-)
[...]
quoted hunk ↗ jump to hunk
enum rp_check {diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 4b7e590dc428..9e971f86afdf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c@@ -67,6 +67,8 @@ struct uprobe { loff_t ref_ctr_offset; unsigned long flags;
we should shorten flags to unsigned int, we use one bit out of it
+ unsigned int consumers_cnt; +
and then this won't increase the size of the struct unnecessarily
quoted hunk ↗ jump to hunk
/* * The generic code assumes that it has two members of unknown type * owned by the arch-specific code:@@ -826,8 +828,12 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
[...]
current->utask->auprobe = NULL; - if (need_prep && !remove) - prepare_uretprobe(uprobe, regs); /* put bp at return */ + if (ri && !remove) + prepare_uretprobe(uprobe, regs, ri); /* put bp at return */ + else + kfree(ri);
maybe `else if (ri) kfree(ri)` to avoid unnecessary calls to kfree when we only have uprobes?
quoted hunk ↗ jump to hunk
if (remove && has_consumers) { down_read(&uprobe->register_rwsem);@@ -2160,15 +2230,25 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs) static void handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs) { + struct return_consumer *ric = NULL; struct uprobe *uprobe = ri->uprobe; struct uprobe_consumer *uc; - int srcu_idx; + int srcu_idx, iter = 0;
iter -> next_ric_idx or just ric_idx?
srcu_idx = srcu_read_lock(&uprobes_srcu);
list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node,
srcu_read_lock_held(&uprobes_srcu)) {
+ /*
+ * If we don't find return consumer, it means uprobe consumer
+ * was added after we hit uprobe and return consumer did not
+ * get registered in which case we call the ret_handler only
+ * if it's not session consumer.
+ */
+ ric = return_consumer_find(ri, &iter, uc->id);
+ if (!ric && uc->session)
+ continue;
if (uc->ret_handler)
- uc->ret_handler(uc, ri->func, regs);
+ uc->ret_handler(uc, ri->func, regs, ric ? &ric->cookie : NULL);
}
srcu_read_unlock(&uprobes_srcu, srcu_idx);
}[...]