Re: [PATCH bpf-next 3/3] bpf/selftests: add bpf_get_task_stack retval bounds test_prog

[Song Liu <songliubraving@fb.com>]

> On Apr 15, 2021, at 7:55 PM, Dave Marchevsky <davemarchevsky@fb.com> wrote:
> 
> Add a libbpf test prog which feeds bpf_get_task_stack's return value
> into seq_write after confirming it's positive. No attempt to bound the
> value from above is made.
> 
> Load will fail if verifier does not refine retval range based on buf sz
> input to bpf_get_task_stack.
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

With one nit below. 


> ---
> .../selftests/bpf/prog_tests/bpf_iter.c       |  1 +
> .../selftests/bpf/progs/bpf_iter_task_stack.c | 22 +++++++++++++++++++
> 2 files changed, 23 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 74c45d557a2b..2d3590cfb5e1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -147,6 +147,7 @@ static void test_task_stack(void)
> 		return;
> 
> 	do_dummy_read(skel->progs.dump_task_stack);
> +	do_dummy_read(skel->progs.get_task_user_stacks);
> 
> 	bpf_iter_task_stack__destroy(skel);
> }
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c
> index 50e59a2e142e..c60048ed226f 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c
> @@ -35,3 +35,25 @@ int dump_task_stack(struct bpf_iter__task *ctx)
> 
> 	return 0;
> }
> +
> +SEC("iter/task")
> +int get_task_user_stacks(struct bpf_iter__task *ctx)
> +{
> +	struct seq_file *seq = ctx->meta->seq;
> +	struct task_struct *task = ctx->task;
> +	uint64_t buf_sz = 0;
> +	int64_t res;
> +
> +	if (task == (void *)0)
> +		return 0;
> +
> +	res = bpf_get_task_stack(task, entries,
> +			MAX_STACK_TRACE_DEPTH * SIZE_OF_ULONG, BPF_F_USER_STACK);
> +	if (res <= 0)
> +		return 0;
> +
> +	buf_sz += res;
> +
nit: When the test fails because of missing the verifier change, a comment here
would help the debug effort. 

> +	bpf_seq_write(seq, &entries, buf_sz);
> +	return 0;
> +}
> -- 
> 2.30.2
>