Message-ID: <57a2e223-6fe8-7b7b-1b02-800665673ad1@linux.dev>
Date:   Tue, 24 Jan 2023 11:52:17 -0800
MIME-Version: 1.0
Subject: Re: [PATCH bpf-next v2 4/4] bpf/selftests: Verify struct_ops prog
 sleepable behavior
Content-Language: en-US
To:     David Vernet <void@manifault.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        song@kernel.org, yhs@meta.com, john.fastabend@gmail.com,
        kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@meta.com, tj@kernel.org, bpf@vger.kernel.org
References: <20230124160802.1122124-1-void@manifault.com>
 <20230124160802.1122124-5-void@manifault.com>
From:   Martin KaFai Lau <martin.lau@linux.dev>
In-Reply-To: <20230124160802.1122124-5-void@manifault.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;