Messages in this thread | | | Date | Tue, 24 Jan 2023 11:52:17 -0800 | Subject | Re: [PATCH bpf-next v2 4/4] bpf/selftests: Verify struct_ops prog sleepable behavior | From | Martin KaFai Lau <> |
| |
On 1/24/23 8:08 AM, David Vernet wrote: > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 50123afab9bf..64034311c5f7 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1474,6 +1474,7 @@ struct bpf_dummy_ops { > int (*test_1)(struct bpf_dummy_ops_state *cb); > int (*test_2)(struct bpf_dummy_ops_state *cb, int a1, unsigned short a2, > char a3, unsigned long a4); > + int (*test_3)(struct bpf_dummy_ops_state *cb);
nit. May be a self describe name like test_sleepable().
> }; > > int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr, > diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > index 1ac4467928a9..46099737d1da 100644 > --- a/net/bpf/bpf_dummy_struct_ops.c > +++ b/net/bpf/bpf_dummy_struct_ops.c > @@ -154,6 +154,23 @@ static bool bpf_dummy_ops_is_valid_access(int off, int size, > return bpf_tracing_btf_ctx_access(off, size, type, prog, info); > } > > +static int bpf_dummy_ops_check_member(const struct btf_type *t, > + const struct btf_member *member, > + const struct bpf_prog *prog) > +{ > + u32 moff = __btf_member_bit_offset(t, member) / 8; > + > + switch (moff) { > + case offsetof(struct bpf_dummy_ops, test_3): > + break; > + default: > + if (prog->aux->sleepable) > + return -EINVAL; > + } > + > + return 0; > +} > + > static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, > const struct bpf_reg_state *reg, > int off, int size, enum bpf_access_type atype, > @@ -208,6 +225,7 @@ static void bpf_dummy_unreg(void *kdata) > struct bpf_struct_ops bpf_bpf_dummy_ops = { > .verifier_ops = &bpf_dummy_verifier_ops, > .init = bpf_dummy_init, > + .check_member = bpf_dummy_ops_check_member, > .init_member = bpf_dummy_init_member, > .reg = bpf_dummy_reg, > .unreg = bpf_dummy_unreg, > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 8da0d73b368e..33ea57d34c0b 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -730,6 +730,10 @@ noinline void bpf_kfunc_call_test_destructive(void) > { > } > > +noinline void bpf_kfunc_call_test_sleepable(void) > +{ > +} > + > __diag_pop(); > > BTF_SET8_START(bpf_test_modify_return_ids) > @@ -767,6 +771,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1) > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2) > BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS) > BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE) > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_sleepable, KF_SLEEPABLE)
KF_SLEEPABLE kfunc is not specific to the struct_ops prog. I hope a test has already covered that KF_SLEEPABLE kfunc can only be called from sleepable prog. eg. there is bpf_fentry_test1.
This new kfunc could then be omitted and make the test simpler. There is no need to add the test to the DENYLIST.s390x: https://github.com/kernel-patches/bpf/actions/runs/3998188872/jobs/6861920516
> diff --git a/tools/testing/selftests/bpf/progs/dummy_st_ops_common.h b/tools/testing/selftests/bpf/progs/dummy_st_ops_common.h > new file mode 100644 > index 000000000000..7d0761594b69 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/dummy_st_ops_common.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */ > + > +#ifndef _DUMMY_ST_OPS_COMMON_H > +#define _DUMMY_ST_OPS_COMMON_H > + > +struct bpf_dummy_ops_state { > + int val; > +} __attribute__((preserve_access_index)); > + > +struct bpf_dummy_ops { > + int (*test_1)(struct bpf_dummy_ops_state *state); > + int (*test_2)(struct bpf_dummy_ops_state *state, int a1, unsigned short a2, > + char a3, unsigned long a4); > + int (*test_3)(struct bpf_dummy_ops_state *state); > +};
Instead of adding a new dummy_st_ops_common.h header, try to directly include vmlinux.h in the dummy_st_ops_{success,fail}.c.
> + > +void bpf_kfunc_call_test_sleepable(void) __ksym;
| |