On 8/1/23 12:44 PM, Yonghong Song wrote:
On 8/1/23 4:53 AM, Yafang Shao wrote:
On Tue, Aug 1, 2023 at 3:30 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
Adding support for bpf_get_func_ip helper for uprobe program to return
probed address for both uprobe and return uprobe.
We discussed this in [1] and agreed that uprobe can have special use
of bpf_get_func_ip helper that differs from kprobe.
The kprobe bpf_get_func_ip returns:
- address of the function if probe is attach on function entry
for both kprobe and return kprobe
- 0 if the probe is not attach on function entry
The uprobe bpf_get_func_ip returns:
- address of the probe for both uprobe and return uprobe
The reason for this semantic change is that kernel can't really tell
if the probe user space address is function entry.
The uprobe program is actually kprobe type program attached as uprobe.
One of the consequences of this design is that uprobes do not have its
own set of helpers, but share them with kprobes.
As we need different functionality for bpf_get_func_ip helper for
uprobe,
I'm adding the bool value to the bpf_trace_run_ctx, so the helper can
detect that it's executed in uprobe context and call specific code.
The is_uprobe bool is set as true in bpf_prog_run_array_sleepable which
is currently used only for executing bpf programs in uprobe.
That is error-prone. If we don't intend to rename
bpf_prog_run_array_sleepable() to bpf_prog_run_array_uprobe(), I think
we'd better introduce a new parameter 'bool is_uprobe' into it.
Agree that renaming bpf_prog_run_array_sleepable() to
bpf_prog_run_array_uprobe() probably better. This way, it is
self-explainable for `run_ctx.is_uprobe = true`.
If unlikely case in the future, another sleepable run prog array
is needed. They can have their own bpf_prog_run_array_<..>
and underlying bpf_prog_run_array_sleepable() can be factored out.
Or if want to avoid unnecessary code churn, at least add
a comment in bpf_prog_run_array_sleepable() to explain
that why it is safe to do `run_ctx.is_uprobe = true;`.
Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
[1]
https://lore.kernel.org/bpf/CAEf4BzZ=xLVkG5eurEuvLU79wAMtwho7ReR+XJAgwhFF4M-7Cg@xxxxxxxxxxxxxx/
Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
---
include/linux/bpf.h | 5 +++++
include/uapi/linux/bpf.h | 7 ++++++-
kernel/trace/bpf_trace.c | 21 ++++++++++++++++++++-
kernel/trace/trace_probe.h | 5 +++++
kernel/trace/trace_uprobe.c | 5 -----
tools/include/uapi/linux/bpf.h | 7 ++++++-
6 files changed, 42 insertions(+), 8 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index ceaa8c23287f..8ea071383ef1 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1819,6 +1819,7 @@ struct bpf_cg_run_ctx {
struct bpf_trace_run_ctx {
struct bpf_run_ctx run_ctx;
u64 bpf_cookie;
+ bool is_uprobe;
};
struct bpf_tramp_run_ctx {
@@ -1867,6 +1868,8 @@ bpf_prog_run_array(const struct bpf_prog_array
*array,
if (unlikely(!array))
return ret;
+ run_ctx.is_uprobe = false;
+
migrate_disable();
old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
item = &array->items[0];
@@ -1906,6 +1909,8 @@ bpf_prog_run_array_sleepable(const struct
bpf_prog_array __rcu *array_rcu,
rcu_read_lock_trace();
migrate_disable();
+ run_ctx.is_uprobe = true;
+
array = rcu_dereference_check(array_rcu,
rcu_read_lock_trace_held());
if (unlikely(!array))
goto out;
[...]
