Message ID | 20210928025228.88673-4-houtao1@huawei.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | introduce dummy BPF STRUCT_OPS | expand |
Context | Check | Description |
---|---|---|
netdev/apply | fail | Patch does not apply to bpf-next |
netdev/tree_selection | success | Clearly marked for bpf-next |
bpf/vmtest-bpf-next-PR | fail | merge-conflict |
On Tue, Sep 28, 2021 at 10:52:26AM +0800, Hou Tao wrote: > Now only program for bpf_dummy_ops::init() is supported. The following > two cases are exercised in bpf_dummy_st_ops_test_run(): > > (1) test and check the value returned from state arg in init(state) > The content of state is copied from data_in before calling init() and > copied back to data_out after calling, so test program could use > data_in to pass the input state and use data_out to get the > output state. > > (2) test and check the return value of init(NULL) > data_in_size is set as 0, so the state will be NULL and there will be > no copy-in & copy-out. Patch 1 and patch 3 in this set should be combined. > include/linux/bpf_dummy_ops.h | 13 ++- > net/bpf/bpf_dummy_struct_ops.c | 176 +++++++++++++++++++++++++++++++++ > 2 files changed, 188 insertions(+), 1 deletion(-) > > diff --git a/include/linux/bpf_dummy_ops.h b/include/linux/bpf_dummy_ops.h > index a594ae830eba..5049484e6693 100644 > --- a/include/linux/bpf_dummy_ops.h > +++ b/include/linux/bpf_dummy_ops.h The changes here seem not worth a new header file. Let see if they can be simplified and move the only needed things to bpf.h. > @@ -5,10 +5,21 @@ > #ifndef _BPF_DUMMY_OPS_H > #define _BPF_DUMMY_OPS_H > > -typedef int (*bpf_dummy_ops_init_t)(void); > +#include <linux/bpf.h> > +#include <linux/filter.h> > + > +struct bpf_dummy_ops_state { > + int val; > +}; This struct can be moved to net/bpf/bpf_dummy_struct_ops.c. > + > +typedef int (*bpf_dummy_ops_init_t)(struct bpf_dummy_ops_state *cb); If I read it correctly, the typedef is only useful in casting later. It would need another typedef in the future if new test function is added. Lets try to remove it (more on this later). > > struct bpf_dummy_ops { > bpf_dummy_ops_init_t init; "init" is a little confusing since it is not doing initialization. It is for testing purpose. How about renaming it to test1, test2, test3...: int (*test1)(struct bpf_dummy_ops_state *cb); Also, it should at least add another function to test more arguments which is another limitation of testing with tcp_congestion_ops. > }; The whole struct bpf_dummy_ops can be moved to include/linux/bpf.h also next to where other bpf_struct_ops_*() functions are residing. > > +extern int bpf_dummy_st_ops_test_run(struct bpf_prog *prog, > + const union bpf_attr *kattr, > + union bpf_attr __user *uattr); Same here. It can be moved to include/linux/bpf.h and remove the "extern" also. > + > #endif > diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > index 1249e4bb4ccb..da77736cd093 100644 > --- a/net/bpf/bpf_dummy_struct_ops.c > +++ b/net/bpf/bpf_dummy_struct_ops.c > @@ -10,12 +10,188 @@ > > extern struct bpf_struct_ops bpf_bpf_dummy_ops; > > +static const struct btf_type *dummy_ops_state; > + > +static struct bpf_dummy_ops_state * > +init_dummy_ops_state(const union bpf_attr *kattr) > +{ > + __u32 size_in; > + struct bpf_dummy_ops_state *state; > + void __user *data_in; > + > + size_in = kattr->test.data_size_in; These are the args for the test functions? Using ctx_in/ctx_size_in and ctx_out/ctx_size_out instead should be more consistent with other bpf_prog_test_run* in test_run.c. > + if (!size_in) > + return NULL; > + > + if (size_in != sizeof(*state)) > + return ERR_PTR(-EINVAL); > + > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + data_in = u64_to_user_ptr(kattr->test.data_in); > + if (copy_from_user(state, data_in, size_in)) { > + kfree(state); > + return ERR_PTR(-EFAULT); > + } > + > + return state; > +} > + > +static int copy_dummy_ops_state(struct bpf_dummy_ops_state *state, > + const union bpf_attr *kattr, > + union bpf_attr __user *uattr) > +{ > + int err = 0; > + void __user *data_out; > + > + if (!state) > + return 0; > + > + data_out = u64_to_user_ptr(kattr->test.data_out); > + if (copy_to_user(data_out, state, sizeof(*state))) { > + err = -EFAULT; > + goto out; Directly return err; > + } > + if (put_user(sizeof(*state), &uattr->test.data_size_out)) { > + err = -EFAULT; > + goto out; Same here. > + } > +out: > + return err; > +} > + > +static inline void exit_dummy_ops_state(struct bpf_dummy_ops_state *state) static is good enough. no need to inline. Allow the compiler to decide. > +{ > + kfree(state); Probably just remove this helper function and directly call kfree instead. Could you help to check if bpf_ctx_init and bpf_ctx_finish can be directly reused instead? I haven't looked at them closely to compare yet. > +} > + > +int bpf_dummy_st_ops_test_run(struct bpf_prog *prog, > + const union bpf_attr *kattr, > + union bpf_attr __user *uattr) > +{ > + const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops; > + struct bpf_dummy_ops_state *state = NULL; > + struct bpf_tramp_progs *tprogs = NULL; > + void *image = NULL; > + int err; > + int prog_ret; > + > + /* Now only support to call init(...) */ > + if (prog->expected_attach_type != 0) { > + err = -EOPNOTSUPP; > + goto out; > + } > + > + /* state will be NULL when data_size_in == 0 */ > + state = init_dummy_ops_state(kattr); > + if (IS_ERR(state)) { > + err = PTR_ERR(state); > + state = NULL; > + goto out; > + } > + > + tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL); > + if (!tprogs) { > + err = -ENOMEM; > + goto out; > + } > + > + image = bpf_jit_alloc_exec(PAGE_SIZE); > + if (!image) { > + err = -ENOMEM; > + goto out; > + } > + set_vm_flush_reset_perms(image); > + > + err = bpf_prepare_st_ops_prog(tprogs, prog, &st_ops->func_models[0], > + image, image + PAGE_SIZE); > + if (err < 0) > + goto out; > + > + set_memory_ro((long)image, 1); > + set_memory_x((long)image, 1); > + prog_ret = ((bpf_dummy_ops_init_t)image)(state); I would do something like this to avoid creating the bpf_dummy_ops_init_t typedef. struct bpf_dummy_ops ops; ops.init = (void *)image; prog_ret = ops.init(state); > + > + err = copy_dummy_ops_state(state, kattr, uattr); > + if (err) > + goto out; > + if (put_user(prog_ret, &uattr->test.retval)) > + err = -EFAULT; > +out: > + exit_dummy_ops_state(state); > + bpf_jit_free_exec(image); > + kfree(tprogs); > + return err; > +} > + > static int bpf_dummy_init(struct btf *btf) > { > + s32 type_id; > + > + type_id = btf_find_by_name_kind(btf, "bpf_dummy_ops_state", > + BTF_KIND_STRUCT); > + if (type_id < 0) > + return -EINVAL; > + > + dummy_ops_state = btf_type_by_id(btf, type_id); > + > return 0; > } > > +static bool bpf_dummy_ops_is_valid_access(int off, int size, > + enum bpf_access_type type, > + const struct bpf_prog *prog, > + struct bpf_insn_access_aux *info) > +{ > + /* init(state) only has one argument */ > + if (off || type != BPF_READ) > + return false; > + > + return btf_ctx_access(off, size, type, prog, info); > +} > + > +static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, > + const struct btf *btf, > + const struct btf_type *t, int off, > + int size, enum bpf_access_type atype, > + u32 *next_btf_id) > +{ > + size_t end; > + > + if (atype == BPF_READ) > + return btf_struct_access(log, btf, t, off, size, atype, > + next_btf_id); > + > + if (t != dummy_ops_state) { > + bpf_log(log, "only read is supported\n"); > + return -EACCES; > + } The idea is to only allow writing to dummy_ops_state? How about something like this (uncompiled code): int ret; if (atype != BPF_READ && t != dummy_ops_state) return -EACCES; ret = btf_struct_access(log, btf, t, off, size, atype, next_btf_id); if (ret < 0) return ret; return atype == BPF_READ ? ret : NOT_INIT; Then the following switch and offset logic can go away. > + > + switch (off) { > + case offsetof(struct bpf_dummy_ops_state, val): > + end = offsetofend(struct bpf_dummy_ops_state, val); > + break; > + default: > + bpf_log(log, "no write support to bpf_dummy_ops_state at off %d\n", > + off); > + return -EACCES; > + } > + > + if (off + size > end) { > + bpf_log(log, > + "write access at off %d with size %d beyond the member of bpf_dummy_ops_state ended at %zu\n", > + off, size, end); > + return -EACCES; > + } > + > + return NOT_INIT; > +} > + > static const struct bpf_verifier_ops bpf_dummy_verifier_ops = { > + .is_valid_access = bpf_dummy_ops_is_valid_access, > + .btf_struct_access = bpf_dummy_ops_btf_struct_access, > }; > > static int bpf_dummy_init_member(const struct btf_type *t, > -- > 2.29.2 >
Hi, On 9/30/2021 2:55 AM, Martin KaFai Lau wrote: > On Tue, Sep 28, 2021 at 10:52:26AM +0800, Hou Tao wrote: >> Now only program for bpf_dummy_ops::init() is supported. The following >> two cases are exercised in bpf_dummy_st_ops_test_run(): >> >> (1) test and check the value returned from state arg in init(state) >> The content of state is copied from data_in before calling init() and >> copied back to data_out after calling, so test program could use >> data_in to pass the input state and use data_out to get the >> output state. >> >> (2) test and check the return value of init(NULL) >> data_in_size is set as 0, so the state will be NULL and there will be >> no copy-in & copy-out. > Patch 1 and patch 3 in this set should be combined. Will do. The purpose of splitting into two patches is that if only the return value test is needed, patch 3 can be dropped. But now we will add more tests, so i think combine two patches into one is OK. > >> include/linux/bpf_dummy_ops.h | 13 ++- >> net/bpf/bpf_dummy_struct_ops.c | 176 +++++++++++++++++++++++++++++++++ >> 2 files changed, 188 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/bpf_dummy_ops.h b/include/linux/bpf_dummy_ops.h >> index a594ae830eba..5049484e6693 100644 >> --- a/include/linux/bpf_dummy_ops.h >> +++ b/include/linux/bpf_dummy_ops.h > The changes here seem not worth a new header file. > Let see if they can be simplified and move the only needed things to bpf.h. > >> @@ -5,10 +5,21 @@ >> #ifndef _BPF_DUMMY_OPS_H >> #define _BPF_DUMMY_OPS_H >> >> -typedef int (*bpf_dummy_ops_init_t)(void); >> +#include <linux/bpf.h> >> +#include <linux/filter.h> >> + >> +struct bpf_dummy_ops_state { >> + int val; >> +}; > This struct can be moved to net/bpf/bpf_dummy_struct_ops.c. > >> + >> +typedef int (*bpf_dummy_ops_init_t)(struct bpf_dummy_ops_state *cb); > If I read it correctly, the typedef is only useful in casting later. > It would need another typedef in the future if new test function is added. > Lets try to remove it (more on this later). > >> >> struct bpf_dummy_ops { >> bpf_dummy_ops_init_t init; > "init" is a little confusing since it is not doing initialization. > It is for testing purpose. How about renaming it to test1, test2, test3...: > > int (*test1)(struct bpf_dummy_ops_state *cb); > > Also, it should at least add another function to test more > arguments which is another limitation of testing with > tcp_congestion_ops. > >> }; > The whole struct bpf_dummy_ops can be moved to include/linux/bpf.h also > next to where other bpf_struct_ops_*() functions are residing. Will do. Thanks for your suggestions. > >> >> +extern int bpf_dummy_st_ops_test_run(struct bpf_prog *prog, >> + const union bpf_attr *kattr, >> + union bpf_attr __user *uattr); > Same here. It can be moved to include/linux/bpf.h and remove the > "extern" also. > >> + >> #endif >> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c >> index 1249e4bb4ccb..da77736cd093 100644 >> --- a/net/bpf/bpf_dummy_struct_ops.c >> +++ b/net/bpf/bpf_dummy_struct_ops.c >> @@ -10,12 +10,188 @@ >> >> extern struct bpf_struct_ops bpf_bpf_dummy_ops; >> >> +static const struct btf_type *dummy_ops_state; >> + >> +static struct bpf_dummy_ops_state * >> +init_dummy_ops_state(const union bpf_attr *kattr) >> +{ >> + __u32 size_in; >> + struct bpf_dummy_ops_state *state; >> + void __user *data_in; >> + >> + size_in = kattr->test.data_size_in; > These are the args for the test functions? Using ctx_in/ctx_size_in > and ctx_out/ctx_size_out instead should be more consistent > with other bpf_prog_test_run* in test_run.c. Yes, there are args. I had think about using ctx_in/ctx_out, but I didn't because I thought the program which using ctx_in/ctx_out only has one argument (namely bpf_context *), but the bpf_dummy_ops::init may have multiple arguments. Anyway I will check it again and use ctx_in/ctx_out if possible. > >> + if (!size_in) >> + return NULL; >> + >> + if (size_in != sizeof(*state)) >> + return ERR_PTR(-EINVAL); >> + >> + state = kzalloc(sizeof(*state), GFP_KERNEL); >> + if (!state) >> + return ERR_PTR(-ENOMEM); >> + >> + data_in = u64_to_user_ptr(kattr->test.data_in); >> + if (copy_from_user(state, data_in, size_in)) { >> + kfree(state); >> + return ERR_PTR(-EFAULT); >> + } >> + >> + return state; >> +} >> + >> +static int copy_dummy_ops_state(struct bpf_dummy_ops_state *state, >> + const union bpf_attr *kattr, >> + union bpf_attr __user *uattr) >> +{ >> + int err = 0; >> + void __user *data_out; >> + >> + if (!state) >> + return 0; >> + >> + data_out = u64_to_user_ptr(kattr->test.data_out); >> + if (copy_to_user(data_out, state, sizeof(*state))) { >> + err = -EFAULT; >> + goto out; > Directly return err; Will do > >> + } >> + if (put_user(sizeof(*state), &uattr->test.data_size_out)) { >> + err = -EFAULT; >> + goto out; > Same here. > >> + } >> +out: >> + return err; >> +} >> + >> +static inline void exit_dummy_ops_state(struct bpf_dummy_ops_state *state) > static is good enough. no need to inline. Allow the compiler to decide. > >> +{ >> + kfree(state); > Probably just remove this helper function and directly call kfree instead. > > Could you help to check if bpf_ctx_init and bpf_ctx_finish can be directly > reused instead? I haven't looked at them closely to compare yet. Will do. > >> +} >> + >> +int bpf_dummy_st_ops_test_run(struct bpf_prog *prog, >> + const union bpf_attr *kattr, >> + union bpf_attr __user *uattr) >> +{ >> + const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops; >> + struct bpf_dummy_ops_state *state = NULL; >> + struct bpf_tramp_progs *tprogs = NULL; >> + void *image = NULL; >> + int err; >> + int prog_ret; >> + >> + /* Now only support to call init(...) */ >> + if (prog->expected_attach_type != 0) { >> + err = -EOPNOTSUPP; >> + goto out; >> + } >> + >> + /* state will be NULL when data_size_in == 0 */ >> + state = init_dummy_ops_state(kattr); >> + if (IS_ERR(state)) { >> + err = PTR_ERR(state); >> + state = NULL; >> + goto out; >> + } >> + >> + tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL); >> + if (!tprogs) { >> + err = -ENOMEM; >> + goto out; >> + } >> + >> + image = bpf_jit_alloc_exec(PAGE_SIZE); >> + if (!image) { >> + err = -ENOMEM; >> + goto out; >> + } >> + set_vm_flush_reset_perms(image); >> + >> + err = bpf_prepare_st_ops_prog(tprogs, prog, &st_ops->func_models[0], >> + image, image + PAGE_SIZE); >> + if (err < 0) >> + goto out; >> + >> + set_memory_ro((long)image, 1); >> + set_memory_x((long)image, 1); >> + prog_ret = ((bpf_dummy_ops_init_t)image)(state); > I would do something like this to avoid creating the > bpf_dummy_ops_init_t typedef. > > struct bpf_dummy_ops ops; > > ops.init = (void *)image; > prog_ret = ops.init(state); Good idea. Will do that. > >> + >> + err = copy_dummy_ops_state(state, kattr, uattr); >> + if (err) >> + goto out; >> + if (put_user(prog_ret, &uattr->test.retval)) >> + err = -EFAULT; >> +out: >> + exit_dummy_ops_state(state); >> + bpf_jit_free_exec(image); >> + kfree(tprogs); >> + return err; >> +} >> + >> static int bpf_dummy_init(struct btf *btf) >> { >> + s32 type_id; >> + >> + type_id = btf_find_by_name_kind(btf, "bpf_dummy_ops_state", >> + BTF_KIND_STRUCT); >> + if (type_id < 0) >> + return -EINVAL; >> + >> + dummy_ops_state = btf_type_by_id(btf, type_id); >> + >> return 0; >> } >> >> +static bool bpf_dummy_ops_is_valid_access(int off, int size, >> + enum bpf_access_type type, >> + const struct bpf_prog *prog, >> + struct bpf_insn_access_aux *info) >> +{ >> + /* init(state) only has one argument */ >> + if (off || type != BPF_READ) >> + return false; >> + >> + return btf_ctx_access(off, size, type, prog, info); >> +} >> + >> +static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, >> + const struct btf *btf, >> + const struct btf_type *t, int off, >> + int size, enum bpf_access_type atype, >> + u32 *next_btf_id) >> +{ >> + size_t end; >> + >> + if (atype == BPF_READ) >> + return btf_struct_access(log, btf, t, off, size, atype, >> + next_btf_id); >> + >> + if (t != dummy_ops_state) { >> + bpf_log(log, "only read is supported\n"); >> + return -EACCES; >> + } > The idea is to only allow writing to dummy_ops_state? > > How about something like this (uncompiled code): > > int ret; > > if (atype != BPF_READ && t != dummy_ops_state) > return -EACCES; > > ret = btf_struct_access(log, btf, t, off, size, atype, > next_btf_id); > if (ret < 0) > return ret; > > return atype == BPF_READ ? ret : NOT_INIT; > > Then the following switch and offset logic can go away. Good idea. Will do that in v2. > >> + >> + switch (off) { >> + case offsetof(struct bpf_dummy_ops_state, val): >> + end = offsetofend(struct bpf_dummy_ops_state, val); >> + break; >> + default: >> + bpf_log(log, "no write support to bpf_dummy_ops_state at off %d\n", >> + off); >> + return -EACCES; >> + } >> + >> + if (off + size > end) { >> + bpf_log(log, >> + "write access at off %d with size %d beyond the member of bpf_dummy_ops_state ended at %zu\n", >> + off, size, end); >> + return -EACCES; >> + } >> + >> + return NOT_INIT; >> +} >> + >> static const struct bpf_verifier_ops bpf_dummy_verifier_ops = { >> + .is_valid_access = bpf_dummy_ops_is_valid_access, >> + .btf_struct_access = bpf_dummy_ops_btf_struct_access, >> }; >> >> static int bpf_dummy_init_member(const struct btf_type *t, >> -- >> 2.29.2 >> > .
On Thu, Sep 30, 2021 at 07:05:41PM +0800, Hou Tao wrote: > >> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > >> index 1249e4bb4ccb..da77736cd093 100644 > >> --- a/net/bpf/bpf_dummy_struct_ops.c > >> +++ b/net/bpf/bpf_dummy_struct_ops.c > >> @@ -10,12 +10,188 @@ > >> > >> extern struct bpf_struct_ops bpf_bpf_dummy_ops; > >> > >> +static const struct btf_type *dummy_ops_state; > >> + > >> +static struct bpf_dummy_ops_state * > >> +init_dummy_ops_state(const union bpf_attr *kattr) > >> +{ > >> + __u32 size_in; > >> + struct bpf_dummy_ops_state *state; > >> + void __user *data_in; > >> + > >> + size_in = kattr->test.data_size_in; > > These are the args for the test functions? Using ctx_in/ctx_size_in > > and ctx_out/ctx_size_out instead should be more consistent > > with other bpf_prog_test_run* in test_run.c. > Yes, there are args. I had think about using ctx_in/ctx_out, but I didn't > because I thought the program which using ctx_in/ctx_out only has > one argument (namely bpf_context *), but the bpf_dummy_ops::init > may have multiple arguments. Anyway I will check it again and use > ctx_in/ctx_out if possible. got it. ctx_in could have multiple args. I was more thinking on the muliple arg test also. Potentially some of them are just integers, e.g. int test2(struct bpf_dummy_ops_state *state, char a, short b, int c, long d) { } All args can be put in ctx_in like bpf_prog_test_run_raw_tp(). Take a look at raw_tp_test_run.c. Although it is not strictly necessary to use u64 for all args in the struct_ops test because the struct_ops test still wants to prepare the trampoline to catch the return value issue...etc, passing an array of u64 args in ctx_in should make it easier to program the userspace and optimizing the ctx_in based on the sizeof each arg seems not gaining much as a test also. For "struct bpf_dummy_ops_state *state", instead of making an exception to pass ptr arg in data_in, the user ptr can be directly passed as a u64 stored in ctx_in also, then there is no need to use data_in or data_size_in. If it is needed, the userspace's sizeof(struct bpf_dummy_ops_state) can be found from the prog->aux->btf. There is no need to use data_out/data_out_size also, just directly copy it back to the same user ptr stored in ctx_in.
diff --git a/include/linux/bpf_dummy_ops.h b/include/linux/bpf_dummy_ops.h index a594ae830eba..5049484e6693 100644 --- a/include/linux/bpf_dummy_ops.h +++ b/include/linux/bpf_dummy_ops.h @@ -5,10 +5,21 @@ #ifndef _BPF_DUMMY_OPS_H #define _BPF_DUMMY_OPS_H -typedef int (*bpf_dummy_ops_init_t)(void); +#include <linux/bpf.h> +#include <linux/filter.h> + +struct bpf_dummy_ops_state { + int val; +}; + +typedef int (*bpf_dummy_ops_init_t)(struct bpf_dummy_ops_state *cb); struct bpf_dummy_ops { bpf_dummy_ops_init_t init; }; +extern int bpf_dummy_st_ops_test_run(struct bpf_prog *prog, + const union bpf_attr *kattr, + union bpf_attr __user *uattr); + #endif diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index 1249e4bb4ccb..da77736cd093 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -10,12 +10,188 @@ extern struct bpf_struct_ops bpf_bpf_dummy_ops; +static const struct btf_type *dummy_ops_state; + +static struct bpf_dummy_ops_state * +init_dummy_ops_state(const union bpf_attr *kattr) +{ + __u32 size_in; + struct bpf_dummy_ops_state *state; + void __user *data_in; + + size_in = kattr->test.data_size_in; + if (!size_in) + return NULL; + + if (size_in != sizeof(*state)) + return ERR_PTR(-EINVAL); + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (!state) + return ERR_PTR(-ENOMEM); + + data_in = u64_to_user_ptr(kattr->test.data_in); + if (copy_from_user(state, data_in, size_in)) { + kfree(state); + return ERR_PTR(-EFAULT); + } + + return state; +} + +static int copy_dummy_ops_state(struct bpf_dummy_ops_state *state, + const union bpf_attr *kattr, + union bpf_attr __user *uattr) +{ + int err = 0; + void __user *data_out; + + if (!state) + return 0; + + data_out = u64_to_user_ptr(kattr->test.data_out); + if (copy_to_user(data_out, state, sizeof(*state))) { + err = -EFAULT; + goto out; + } + if (put_user(sizeof(*state), &uattr->test.data_size_out)) { + err = -EFAULT; + goto out; + } +out: + return err; +} + +static inline void exit_dummy_ops_state(struct bpf_dummy_ops_state *state) +{ + kfree(state); +} + +int bpf_dummy_st_ops_test_run(struct bpf_prog *prog, + const union bpf_attr *kattr, + union bpf_attr __user *uattr) +{ + const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops; + struct bpf_dummy_ops_state *state = NULL; + struct bpf_tramp_progs *tprogs = NULL; + void *image = NULL; + int err; + int prog_ret; + + /* Now only support to call init(...) */ + if (prog->expected_attach_type != 0) { + err = -EOPNOTSUPP; + goto out; + } + + /* state will be NULL when data_size_in == 0 */ + state = init_dummy_ops_state(kattr); + if (IS_ERR(state)) { + err = PTR_ERR(state); + state = NULL; + goto out; + } + + tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL); + if (!tprogs) { + err = -ENOMEM; + goto out; + } + + image = bpf_jit_alloc_exec(PAGE_SIZE); + if (!image) { + err = -ENOMEM; + goto out; + } + set_vm_flush_reset_perms(image); + + err = bpf_prepare_st_ops_prog(tprogs, prog, &st_ops->func_models[0], + image, image + PAGE_SIZE); + if (err < 0) + goto out; + + set_memory_ro((long)image, 1); + set_memory_x((long)image, 1); + prog_ret = ((bpf_dummy_ops_init_t)image)(state); + + err = copy_dummy_ops_state(state, kattr, uattr); + if (err) + goto out; + if (put_user(prog_ret, &uattr->test.retval)) + err = -EFAULT; +out: + exit_dummy_ops_state(state); + bpf_jit_free_exec(image); + kfree(tprogs); + return err; +} + static int bpf_dummy_init(struct btf *btf) { + s32 type_id; + + type_id = btf_find_by_name_kind(btf, "bpf_dummy_ops_state", + BTF_KIND_STRUCT); + if (type_id < 0) + return -EINVAL; + + dummy_ops_state = btf_type_by_id(btf, type_id); + return 0; } +static bool bpf_dummy_ops_is_valid_access(int off, int size, + enum bpf_access_type type, + const struct bpf_prog *prog, + struct bpf_insn_access_aux *info) +{ + /* init(state) only has one argument */ + if (off || type != BPF_READ) + return false; + + return btf_ctx_access(off, size, type, prog, info); +} + +static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, + const struct btf *btf, + const struct btf_type *t, int off, + int size, enum bpf_access_type atype, + u32 *next_btf_id) +{ + size_t end; + + if (atype == BPF_READ) + return btf_struct_access(log, btf, t, off, size, atype, + next_btf_id); + + if (t != dummy_ops_state) { + bpf_log(log, "only read is supported\n"); + return -EACCES; + } + + switch (off) { + case offsetof(struct bpf_dummy_ops_state, val): + end = offsetofend(struct bpf_dummy_ops_state, val); + break; + default: + bpf_log(log, "no write support to bpf_dummy_ops_state at off %d\n", + off); + return -EACCES; + } + + if (off + size > end) { + bpf_log(log, + "write access at off %d with size %d beyond the member of bpf_dummy_ops_state ended at %zu\n", + off, size, end); + return -EACCES; + } + + return NOT_INIT; +} + static const struct bpf_verifier_ops bpf_dummy_verifier_ops = { + .is_valid_access = bpf_dummy_ops_is_valid_access, + .btf_struct_access = bpf_dummy_ops_btf_struct_access, }; static int bpf_dummy_init_member(const struct btf_type *t,
Now only program for bpf_dummy_ops::init() is supported. The following two cases are exercised in bpf_dummy_st_ops_test_run(): (1) test and check the value returned from state arg in init(state) The content of state is copied from data_in before calling init() and copied back to data_out after calling, so test program could use data_in to pass the input state and use data_out to get the output state. (2) test and check the return value of init(NULL) data_in_size is set as 0, so the state will be NULL and there will be no copy-in & copy-out. Signed-off-by: Hou Tao <houtao1@huawei.com> --- include/linux/bpf_dummy_ops.h | 13 ++- net/bpf/bpf_dummy_struct_ops.c | 176 +++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 1 deletion(-)