netdev - Re: [PATCH bpf-next 2/3] selftests: bpf: break up test

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: <20190123233203.jzxw2xkqwygraqui@ast-mbp.dhcp.thefacebook.com>
Date:   Wed, 23 Jan 2019 15:32:05 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     daniel@...earbox.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next 2/3] selftests: bpf: break up test_verifier

On Mon, Jan 21, 2019 at 10:43:38AM -0800, Jakub Kicinski wrote:
> Break up the first 10 kLoC of test verifier test cases
> out into smaller files.  Looks like git line counting
> gets a little flismy above 16 bit integers, so we need
> two commits to break up test_verifier.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Acked-by: Jiong Wang <jiong.wang@...ronome.com>
> ---
>  tools/testing/selftests/bpf/test_verifier.c   | 10100 ----------------
>  tools/testing/selftests/bpf/verifier/and.c    |    53 +
>  .../selftests/bpf/verifier/array_access.c     |   238 +
...
> -	{
> -		"DIV64 by 0, zero check",
> -		.insns = {
> -			BPF_MOV32_IMM(BPF_REG_0, 42),
> -			BPF_MOV32_IMM(BPF_REG_1, 0),
> -			BPF_MOV32_IMM(BPF_REG_2, 1),
> -			BPF_ALU64_REG(BPF_DIV, BPF_REG_2, BPF_REG_1),
> -			BPF_EXIT_INSN(),
> -		},
> -		.result = ACCEPT,
> -		.retval = 42,
> -	},
...
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/and.c
> @@ -0,0 +1,53 @@
> +{
> +	"invalid and of negative number",
> +	.insns = {
> +		BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +		BPF_LD_MAP_FD(BPF_REG_1, 0),
> +		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> +			     BPF_FUNC_map_lookup_elem),
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> +		BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> +		BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> +		BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> +		BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> +		BPF_ST_MEM(BPF_DW, BPF_REG_0, 0,
> +			   offsetof(struct test_val, foo)),
> +		BPF_EXIT_INSN(),
> +	},
> +	.fixup_map_hash_48b = { 3 },
> +	.errstr = "R0 max value is outside of the array range",
> +	.result = REJECT,
> +	.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> +},

I like the removal of one tab, but since we're refactoring the whole thing
can we remove another tab from instructions?
In many cases we wrap the lines which makes tests a bit harder to read/write
since jmp offsets don't easily add from current line number.
In addition I would propose to represet LD_MAP_FD as two lines too,
but that's optional. I'm thinking to try that for new tests.
So how about the following indent:
{
	"invalid and of negative number",
	.insns = {
	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
	BPF_LD_MAP_FD(BPF_REG_1,
		      0),
	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
	BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
	BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
	BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
	BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
	BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
	BPF_EXIT_INSN(),
	},
	.fixup_map_hash_48b = { 3 },
	.errstr = "R0 max value is outside of the array range",
	.result = REJECT,
	.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},

Notice how calls and offsetof() fits into 80 char.
Another alternative is to switch to two spaces instead of tab:
{
  "invalid and of negative number",
  .insns = {
    BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
    BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
    BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
    BPF_LD_MAP_FD(BPF_REG_1,
                  0),
    BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
    BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
    BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
    BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
    BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
    BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
    BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
    BPF_EXIT_INSN(),
  },
  .fixup_map_hash_48b = { 3 },
  .errstr = "R0 max value is outside of the array range",
  .result = REJECT,
  .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},

Thoughts?