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] [day] [month] [year] [list]

Date:   Wed, 23 Jan 2019 15:47:09 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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 Wed, 23 Jan 2019 15:32:05 -0800, Alexei Starovoitov wrote:
> 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?

I'd vote for option (1).  The two spaces don't seem to provide good
enough visual separation, IMHO we'd either have to go to 4, or put the
opening bracket on it's own line GNU style.