Re: [bpf-next v3 10/12] bpf: Implement bpf_prog_test_run for perf event programs

[Stanislav Fomichev <sdf@fomichev.me>]

On 07/08, Krzesimir Nowak wrote:
> As an input, test run for perf event program takes struct
> bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
> data_in. For an output, it basically ignores ctx_out and data_out.
> 
> The implementation sets an instance of struct bpf_perf_event_data_kern
> in such a way that the BPF program reading data from context will
> receive what we passed to the bpf prog test run in ctx_in. Also BPF
> program can call bpf_perf_prog_read_value to receive what was passed
> in data_in.
> 
> Changes since v2:
> - drop the changes in perf event verifier test - they are not needed
>   anymore after reworked ctx size handling
> 
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  kernel/trace/bpf_trace.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ca1255d14576..b870fc2314d0 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -19,6 +19,8 @@
>  #include "trace_probe.h"
>  #include "trace.h"
>  
> +#include <trace/events/bpf_test_run.h>
> +
>  #define bpf_event_rcu_dereference(p)					\
>  	rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
>  
> @@ -1160,7 +1162,65 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
>  	.convert_ctx_access	= pe_prog_convert_ctx_access,
>  };
>  
> +static int pe_prog_test_run(struct bpf_prog *prog,
> +			    const union bpf_attr *kattr,
> +			    union bpf_attr __user *uattr)
> +{
> +	struct bpf_perf_event_data_kern real_ctx = {0, };
> +	struct perf_sample_data sample_data = {0, };
> +	struct bpf_perf_event_data *fake_ctx;
> +	struct bpf_perf_event_value *value;
> +	struct perf_event event = {0, };
> +	u32 retval = 0, duration = 0;
> +	int err;
> +
> +	if (kattr->test.data_size_out || kattr->test.data_out)
> +		return -EINVAL;
> +	if (kattr->test.ctx_size_out || kattr->test.ctx_out)
> +		return -EINVAL;
> +
> +	fake_ctx = bpf_receive_ctx(kattr, sizeof(struct bpf_perf_event_data));
> +	if (IS_ERR(fake_ctx))
> +		return PTR_ERR(fake_ctx);
> +
> +	value = bpf_receive_data(kattr, sizeof(struct bpf_perf_event_value));
> +	if (IS_ERR(value)) {
> +		kfree(fake_ctx);
> +		return PTR_ERR(value);
> +	}
nit: maybe use bpf_test_ prefix for receive_ctx/data:
* bpf_test_receive_ctx
* bpf_test_receive_data

? To signify that they are used for tests only.

> +
> +	real_ctx.regs = &fake_ctx->regs;
> +	real_ctx.data = &sample_data;
> +	real_ctx.event = &event;
> +	perf_sample_data_init(&sample_data, fake_ctx->addr,
> +			      fake_ctx->sample_period);
> +	event.cpu = smp_processor_id();
> +	event.oncpu = -1;
> +	event.state = PERF_EVENT_STATE_OFF;
> +	local64_set(&event.count, value->counter);
> +	event.total_time_enabled = value->enabled;
> +	event.total_time_running = value->running;
> +	/* make self as a leader - it is used only for checking the
> +	 * state field
> +	 */
> +	event.group_leader = &event;
> +	err = bpf_test_run(prog, &real_ctx, kattr->test.repeat,
> +			   BPF_TEST_RUN_PLAIN, &retval, &duration);
> +	if (err) {
> +		kfree(value);
> +		kfree(fake_ctx);
> +		return err;
> +	}
> +
> +	err = bpf_test_finish(uattr, retval, duration);
> +	trace_bpf_test_finish(&err);
Can probably do:

	err = bpf_test_run(...)
	if (!err) {
		err = bpf_test_finish(uattr, retval, duration);
		trace_bpf_test_finish(&err);
	}
	kfree(..);
	kfree(..);
	return err;

So you don't have to copy-paste the error handling.

> +	kfree(value);
> +	kfree(fake_ctx);
> +	return err;
> +}
> +
>  const struct bpf_prog_ops perf_event_prog_ops = {
> +	.test_run	= pe_prog_test_run,
>  };
>  
>  static DEFINE_MUTEX(bpf_event_mutex);
> -- 
> 2.20.1
>