netdev - Re: [PATCH bpf-next 1/2] selftests/bpf: add loop test 4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63f123d2-b35f-a775-e414-004c90b4f4b7@fb.com>
Date:   Sun, 4 Aug 2019 05:29:42 +0000
From:   Yonghong Song <yhs@...com>
To:     Alexei Starovoitov <ast@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "Kernel Team" <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] selftests/bpf: add loop test 4



On 8/2/19 4:33 PM, Alexei Starovoitov wrote:
> Add a test that returns a 'random' number between [0, 2^20)
> If state pruning is not working correctly for loop body the number of
> processed insns will be 2^20 * num_of_insns_in_loop_body and the program
> will be rejected.

The maximum processed insns will be 2^20 or 2^20 * 
num_of_insns_in_loop_body? I thought the verifier will
stop processing once processed insns reach 1M?

Could you elaborate which potential issues in verifier
you try to cover with this test case? Extra tests are
always welcome. We already have scale/loop tests and some
(e.g., strobemeta tests) are more complex than this one.
Maybe you have something in mind for this particular
test? Putting in the commit message may help people understand
the concerns.

> 
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---
>   .../bpf/prog_tests/bpf_verif_scale.c          |  1 +
>   tools/testing/selftests/bpf/progs/loop4.c     | 23 +++++++++++++++++++
>   2 files changed, 24 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/loop4.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> index b4be96162ff4..757e39540eda 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> @@ -71,6 +71,7 @@ void test_bpf_verif_scale(void)
>   
>   		{ "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
>   		{ "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
> +		{ "loop4.o", BPF_PROG_TYPE_RAW_TRACEPOINT },

The program is more like a BPF_PROG_TYPE_SCHED_CLS type than
a BPF_PROG_TYPE_RAW_TRACEPOINT?

>   
>   		/* partial unroll. 19k insn in a loop.
>   		 * Total program size 20.8k insn.
> diff --git a/tools/testing/selftests/bpf/progs/loop4.c b/tools/testing/selftests/bpf/progs/loop4.c
> new file mode 100644
> index 000000000000..3e7ee14fddbd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/loop4.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Facebook
> +#include <linux/sched.h>
> +#include <linux/ptrace.h>

Since the program is a networking type,
the above two headers are probably unneeded.

> +#include <stdint.h>
> +#include <stddef.h>
> +#include <stdbool.h>
> +#include <linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("socket")
> +int combinations(volatile struct __sk_buff* skb)
> +{
> +	int ret = 0, i;
> +
> +#pragma nounroll
> +	for (i = 0; i < 20; i++)
> +		if (skb->len)
> +			ret |= 1 << i;
> +	return ret;
> +}
>