On Tue, Jan 24, 2023 at 11:52:17AM -0800, Martin KaFai Lau wrote:
> 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().
Will do. I agree that's better, but was just following the existing
contours of the file. Happy to have an excuse to improve it.
>
> > };
> > 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
Ah, good point. Totally forgot about s390x. Will send out a v3 that
doesn't bother with also including the KF_SLEEPABLE invocation, and
instead just validates that .check_member() is called.
>
> > 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.
Ack, I'll give it a shot. Should be fine to include once we get rid of
the test logic that includes the KF_SLEEPABLE kfunc.
>
> > +
> > +void bpf_kfunc_call_test_sleepable(void) __ksym;
>
