| Message ID | 20230203182812.20657-1-grantseltzer@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2,bpf-next] Add support for tracing programs in BPF_PROG_RUN | expand |
On 2/3/23 10:28 AM, Grant Seltzer wrote: > This patch changes the behavior of how BPF_PROG_RUN treats tracing > (fentry/fexit) programs. Previously only a return value is injected > but the actual program was not run. hmm... I don't understand this. The actual program is run by attaching to the bpf_fentry_test{1,2,3...}. eg. The test in fentry_test.c > New behavior mirrors that of running raw tracepoint BPF programs which > actually runs the instructions of the program via `bpf_prog_run()` Which tracepoint and how is it tested? The CI kernel is crashing: https://patchwork.kernel.org/project/netdevbpf/patch/20230203182812.20657-1-grantseltzer@gmail.com/
On Sat, Feb 4, 2023 at 1:58 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/3/23 10:28 AM, Grant Seltzer wrote: > > This patch changes the behavior of how BPF_PROG_RUN treats tracing > > (fentry/fexit) programs. Previously only a return value is injected > > but the actual program was not run. > > hmm... I don't understand this. The actual program is run by attaching to the > bpf_fentry_test{1,2,3...}. eg. The test in fentry_test.c I'm not sure what you mean. Are you saying in order to use the BPF_PROG_RUN bpf syscall command the user must first attach to `bpf_fentry_test1` (or any 1-8), and then execute the BPF_PROG_RUN? > > > New behavior mirrors that of running raw tracepoint BPF programs which > > actually runs the instructions of the program via `bpf_prog_run()` > > Which tracepoint and how is it tested? I was referring to the `bpf_prog_test_run_raw_tp()` function in the same file. I can write additional selftests > > The CI kernel is crashing: > https://patchwork.kernel.org/project/netdevbpf/patch/20230203182812.20657-1-grantseltzer@gmail.com/ > Thanks for linking to this, I was unaware of this being available!
On 2/5/23 9:29 AM, Grant Seltzer Richman wrote: > On Sat, Feb 4, 2023 at 1:58 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 2/3/23 10:28 AM, Grant Seltzer wrote: >>> This patch changes the behavior of how BPF_PROG_RUN treats tracing >>> (fentry/fexit) programs. Previously only a return value is injected >>> but the actual program was not run. >> >> hmm... I don't understand this. The actual program is run by attaching to the >> bpf_fentry_test{1,2,3...}. eg. The test in fentry_test.c > > I'm not sure what you mean. Are you saying in order to use the > BPF_PROG_RUN bpf syscall command the user must first attach to > `bpf_fentry_test1` (or any 1-8), and then execute the BPF_PROG_RUN? It is how the fentry/fexit/fmod_ret...BPF_PROG_TYPE_TRACIN_xxx prog is setup to run now in test_run. afaik, these tracing progs require the trampoline setup before calling the bpf prog, so don't understand how __bpf_prog_test_run_tracing will work safely. A selftest will help how this will work without the traompline but may be first need to understand what it is trying to solve.
On Mon, Feb 6, 2023 at 3:37 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/5/23 9:29 AM, Grant Seltzer Richman wrote: > > On Sat, Feb 4, 2023 at 1:58 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >> > >> On 2/3/23 10:28 AM, Grant Seltzer wrote: > >>> This patch changes the behavior of how BPF_PROG_RUN treats tracing > >>> (fentry/fexit) programs. Previously only a return value is injected > >>> but the actual program was not run. > >> > >> hmm... I don't understand this. The actual program is run by attaching to the > >> bpf_fentry_test{1,2,3...}. eg. The test in fentry_test.c > > > > I'm not sure what you mean. Are you saying in order to use the > > BPF_PROG_RUN bpf syscall command the user must first attach to > > `bpf_fentry_test1` (or any 1-8), and then execute the BPF_PROG_RUN? > > It is how the fentry/fexit/fmod_ret...BPF_PROG_TYPE_TRACIN_xxx prog is setup to > run now in test_run. afaik, these tracing progs require the trampoline setup > before calling the bpf prog, so don't understand how __bpf_prog_test_run_tracing > will work safely. My goal is to be able to take a bpf program of type BPF_PROG_TYPE_TRACING and run it via BPF_PROG_TEST_RUN without having to attach it. The motivation is testing. You can run tracing programs but the actual program isn't run, from the users perspective the syscall just returns 0. You can see how I'm testing this here [1]. If I understand you correctly, it's possible to do something like this, can you give me more information on how I can and I'll be sure to submit documentation for it? [1] https://github.com/grantseltzer/bpf-prog-test-run/tree/main/programs > > A selftest will help how this will work without the traompline but may be first > need to understand what it is trying to solve.
On 2/7/23 7:46 AM, Grant Seltzer Richman wrote: > On Mon, Feb 6, 2023 at 3:37 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 2/5/23 9:29 AM, Grant Seltzer Richman wrote: >>> On Sat, Feb 4, 2023 at 1:58 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: >>>> >>>> On 2/3/23 10:28 AM, Grant Seltzer wrote: >>>>> This patch changes the behavior of how BPF_PROG_RUN treats tracing >>>>> (fentry/fexit) programs. Previously only a return value is injected >>>>> but the actual program was not run. >>>> >>>> hmm... I don't understand this. The actual program is run by attaching to the >>>> bpf_fentry_test{1,2,3...}. eg. The test in fentry_test.c >>> >>> I'm not sure what you mean. Are you saying in order to use the >>> BPF_PROG_RUN bpf syscall command the user must first attach to >>> `bpf_fentry_test1` (or any 1-8), and then execute the BPF_PROG_RUN? >> >> It is how the fentry/fexit/fmod_ret...BPF_PROG_TYPE_TRACIN_xxx prog is setup to >> run now in test_run. afaik, these tracing progs require the trampoline setup >> before calling the bpf prog, so don't understand how __bpf_prog_test_run_tracing >> will work safely. > > My goal is to be able to take a bpf program of type > BPF_PROG_TYPE_TRACING and run it via BPF_PROG_TEST_RUN without having > to attach it. The motivation is testing. You can run tracing programs > but the actual program isn't run, from the users perspective the > syscall just returns 0. You can see how I'm testing this here [1]. > > If I understand you correctly, it's possible to do something like > this, can you give me more information on how I can and I'll be sure > to submit documentation for it? > > [1] https://github.com/grantseltzer/bpf-prog-test-run/tree/main/programs In raw tracepoint, the "ctx" is just a u64 array for the args. fentry/fexit/fmod_ret is much demanding than preparing a u64 array. The trampoline is preparing more than just 'args'. The trampoline is likely to be expanded and changed in the future also. You can take a look at arch_prepare_bpf_trampoline(). Yes, might be the trampoline preparation can be reused. However, I am not convinced tracing program can be tested through test_run in a meaningful and reliable way to worth this complication. eg. A tracing function taking 'struct task_struct *task'. It is not easy for the user space program to prepare the ctx containing a task_struct and the task_struct layout may change also. There are so many traceable kernel functions and I don't think test_run can ever become a single point to test tracing prog for all kernel functions. [ Side-note: test_run for skb/xdp has much narrower focus in terms of argument because it is driven by the packet header like the standard IPv6/TCP/UDP. ] Even for bpf_prog_test_run_raw_tp, the raw_tp_test_run.c is mostly testing if the prog is running on a particular cpu. It is not looking into the args which is what the tracing prog usually does. Please attach the tracing prog to the kernel function to test or reuse the existing bpf_prog_test_run_raw_tp to test it if it does not care the args.
On Tue, Feb 7, 2023 at 8:05 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/7/23 7:46 AM, Grant Seltzer Richman wrote: > > On Mon, Feb 6, 2023 at 3:37 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >> > >> On 2/5/23 9:29 AM, Grant Seltzer Richman wrote: > >>> On Sat, Feb 4, 2023 at 1:58 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >>>> > >>>> On 2/3/23 10:28 AM, Grant Seltzer wrote: > >>>>> This patch changes the behavior of how BPF_PROG_RUN treats tracing > >>>>> (fentry/fexit) programs. Previously only a return value is injected > >>>>> but the actual program was not run. > >>>> > >>>> hmm... I don't understand this. The actual program is run by attaching to the > >>>> bpf_fentry_test{1,2,3...}. eg. The test in fentry_test.c > >>> > >>> I'm not sure what you mean. Are you saying in order to use the > >>> BPF_PROG_RUN bpf syscall command the user must first attach to > >>> `bpf_fentry_test1` (or any 1-8), and then execute the BPF_PROG_RUN? > >> > >> It is how the fentry/fexit/fmod_ret...BPF_PROG_TYPE_TRACIN_xxx prog is setup to > >> run now in test_run. afaik, these tracing progs require the trampoline setup > >> before calling the bpf prog, so don't understand how __bpf_prog_test_run_tracing > >> will work safely. > > > > My goal is to be able to take a bpf program of type > > BPF_PROG_TYPE_TRACING and run it via BPF_PROG_TEST_RUN without having > > to attach it. The motivation is testing. You can run tracing programs > > but the actual program isn't run, from the users perspective the > > syscall just returns 0. You can see how I'm testing this here [1]. > > > > If I understand you correctly, it's possible to do something like > > this, can you give me more information on how I can and I'll be sure > > to submit documentation for it? > > > > [1] https://github.com/grantseltzer/bpf-prog-test-run/tree/main/programs > > In raw tracepoint, the "ctx" is just a u64 array for the args. > > fentry/fexit/fmod_ret is much demanding than preparing a u64 array. The > trampoline is preparing more than just 'args'. The trampoline is likely to be > expanded and changed in the future also. You can take a look at > arch_prepare_bpf_trampoline(). > > Yes, might be the trampoline preparation can be reused. However, I am not > convinced tracing program can be tested through test_run in a meaningful and > reliable way to worth this complication. eg. A tracing function taking 'struct > task_struct *task'. It is not easy for the user space program to prepare the ctx > containing a task_struct and the task_struct layout may change also. There are > so many traceable kernel functions and I don't think test_run can ever become a > single point to test tracing prog for all kernel functions. > [ Side-note: test_run for skb/xdp has much narrower focus in terms of argument > because it is driven by the packet header like the standard IPv6/TCP/UDP. ] > > Even for bpf_prog_test_run_raw_tp, the raw_tp_test_run.c is mostly testing if > the prog is running on a particular cpu. It is not looking into the args which > is what the tracing prog usually does. > > Please attach the tracing prog to the kernel function to test > or reuse the existing bpf_prog_test_run_raw_tp to test it if it does not care > the args. Thank you for the explanation, I understand!
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 8da0d73b368e..0c36ed7dd88e 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -794,14 +794,34 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, return data; } +struct bpf_tracing_test_run_info { + struct bpf_prog *prog; + void *ctx; + u32 retval; +}; + +static void +__bpf_prog_test_run_tracing(void *data) +{ + struct bpf_tracing_test_run_info *info = data; + + rcu_read_lock(); + info->retval = bpf_prog_run(info->prog, info->ctx); + rcu_read_unlock(); +} + int bpf_prog_test_run_tracing(struct bpf_prog *prog, const union bpf_attr *kattr, union bpf_attr __user *uattr) { struct bpf_fentry_test_t arg = {}; - u16 side_effect = 0, ret = 0; - int b = 2, err = -EFAULT; - u32 retval = 0; + int b = 2, err = -EFAULT, current_cpu; + + void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in); + __u32 ctx_size_in = kattr->test.ctx_size_in; + struct bpf_tracing_test_run_info info; + int cpu = kattr->test.cpu; + u16 side_effect = 0; if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size) return -EINVAL; @@ -820,7 +840,7 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog, goto out; break; case BPF_MODIFY_RETURN: - ret = bpf_modify_return_test(1, &b); + bpf_modify_return_test(1, &b); if (b != 2) side_effect = 1; break; @@ -828,11 +848,54 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog, goto out; } - retval = ((u32)side_effect << 16) | ret; - if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) - goto out; + /* doesn't support data_in/out, ctx_out, duration, or repeat */ + if (kattr->test.data_in || kattr->test.data_out || + kattr->test.ctx_out || kattr->test.duration || + kattr->test.repeat || kattr->test.batch_size) + return -EINVAL; + + if (ctx_size_in < prog->aux->max_ctx_offset || + ctx_size_in > MAX_BPF_FUNC_ARGS * sizeof(u64)) + return -EINVAL; + + if ((kattr->test.flags & BPF_F_TEST_RUN_ON_CPU) == 0 && cpu != 0) + return -EINVAL; + + if (ctx_size_in) { + info.ctx = memdup_user(ctx_in, ctx_size_in); + if (IS_ERR(info.ctx)) + return PTR_ERR(info.ctx); + } else { + info.ctx = NULL; + } err = 0; + info.prog = prog; + + current_cpu = get_cpu(); + if ((kattr->test.flags & BPF_F_TEST_RUN_ON_CPU) == 0 || + cpu == current_cpu) { + __bpf_prog_test_run_tracing(&info); + } else if (cpu >= nr_cpu_ids || !cpu_online(cpu)) { + /* smp_call_function_single() also checks cpu_online() + * after csd_lock(). However, since cpu is from user + * space, let's do an extra quick check to filter out + * invalid value before smp_call_function_single(). + */ + err = -ENXIO; + } else { + err = smp_call_function_single(cpu, __bpf_prog_test_run_tracing, + &info, 1); + } + put_cpu(); + + info.retval = ((u32)side_effect << 16) | info.retval; + if (!err && + copy_to_user(&uattr->test.retval, &info.retval, sizeof(u32))) + err = -EFAULT; + + kfree(info.ctx); + out: trace_bpf_test_finish(&err); return err;
This patch changes the behavior of how BPF_PROG_RUN treats tracing (fentry/fexit) programs. Previously only a return value is injected but the actual program was not run. New behavior mirrors that of running raw tracepoint BPF programs which actually runs the instructions of the program via `bpf_prog_run()` Tracing programs only needs to support an input context so we validate that non-relevant attributes are not set. Changes v1 -> v2: - Fixed unused variable and logic for how the fmod_ret test is handled Signed-off-by: Grant Seltzer <grantseltzer@gmail.com> --- net/bpf/test_run.c | 77 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 70 insertions(+), 7 deletions(-)