mbox series

[v2,bpf-next,0/4] bpf: Improve verifier test coverage on sparc64 et al.

Message ID 20181130.210754.282429049580711488.davem@davemloft.net
Headers show
Series bpf: Improve verifier test coverage on sparc64 et al. | expand

Message

David Miller Dec. 1, 2018, 5:07 a.m. UTC
On sparc64 a ton of test cases in test_verifier.c fail because
the memory accesses in the test case are unaligned (or cannot
be proven to be aligned by the verifier).

Perhaps we can eventually try to (carefully) modify each test case
which has this problem to not use unaligned accesses but:

1) That is delicate work.

2) The changes might not fully respect the original
   intention of the testcase.

3) In some cases, such a transformation might not even
   be feasible at all.

So add an "any alignment" flag to tell the verifier to forcefully
disable it's alignment checks completely.

test_verifier.c is then annotated to use this flag when necessary.

The presence of the flag in each test case is good documentation to
anyone who wants to actually tackle the job of eliminating the
unaligned memory accesses in the test cases.

I've also seen several weird things in test cases, like trying to
access __skb->mark in a packet buffer.

This gets rid of 104 test_verifier.c failures on sparc64.

Changes since v1:

1) Explain the new BPF_PROG_LOAD flag in easier to understand terms.
   Suggested by Alexei.

2) Make bpf_verify_program() just take a __u32 prog_flags instead of
   just accumulating boolean arguments over and over.  Also suggested
   by Alexei.

Changes since RFC:

1) Only the admin can allow the relaxation of alignment restrictions
   on inefficient unaligned access architectures.

2) Use F_NEEDS_EFFICIENT_UNALIGNED_ACCESS instead of making a new
   flag.

3) Annotate in the output, when we have a test case that the verifier
   accepted but we did not try to execute because we are on an
   inefficient unaligned access platform.  Maybe with some arch
   machinery we can avoid this in the future.

Signed-off-by: David S. Miller <davem@davemloft.net>

Comments

Alexei Starovoitov Dec. 1, 2018, 5:55 a.m. UTC | #1
On Fri, Nov 30, 2018 at 09:07:54PM -0800, David Miller wrote:
> 
> On sparc64 a ton of test cases in test_verifier.c fail because
> the memory accesses in the test case are unaligned (or cannot
> be proven to be aligned by the verifier).
> 
> Perhaps we can eventually try to (carefully) modify each test case
> which has this problem to not use unaligned accesses but:
> 
> 1) That is delicate work.
> 
> 2) The changes might not fully respect the original
>    intention of the testcase.
> 
> 3) In some cases, such a transformation might not even
>    be feasible at all.
> 
> So add an "any alignment" flag to tell the verifier to forcefully
> disable it's alignment checks completely.
> 
> test_verifier.c is then annotated to use this flag when necessary.
> 
> The presence of the flag in each test case is good documentation to
> anyone who wants to actually tackle the job of eliminating the
> unaligned memory accesses in the test cases.
> 
> I've also seen several weird things in test cases, like trying to
> access __skb->mark in a packet buffer.
> 
> This gets rid of 104 test_verifier.c failures on sparc64.
> 
> Changes since v1:
> 
> 1) Explain the new BPF_PROG_LOAD flag in easier to understand terms.
>    Suggested by Alexei.
> 
> 2) Make bpf_verify_program() just take a __u32 prog_flags instead of
>    just accumulating boolean arguments over and over.  Also suggested
>    by Alexei.
> 
> Changes since RFC:
> 
> 1) Only the admin can allow the relaxation of alignment restrictions
>    on inefficient unaligned access architectures.
> 
> 2) Use F_NEEDS_EFFICIENT_UNALIGNED_ACCESS instead of making a new
>    flag.
> 
> 3) Annotate in the output, when we have a test case that the verifier
>    accepted but we did not try to execute because we are on an
>    inefficient unaligned access platform.  Maybe with some arch
>    machinery we can avoid this in the future.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

The patch 2 didn't apply as-is to bpf-next, since I applied your earlier fix
"bpf: Fix verifier log string check for bad alignment" to bpf tree.
So I applied that fix to bpf-next as well and then pushed your series on top.
I think git should do the right thing when bpf and bpf-next trees converge.
But... let me know if I should drop that fix from bpf tree...
just to be on a safe side.
David Miller Dec. 1, 2018, 5:57 a.m. UTC | #2
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 30 Nov 2018 21:55:02 -0800

> The patch 2 didn't apply as-is to bpf-next, since I applied your
> earlier fix "bpf: Fix verifier log string check for bad alignment"
> to bpf tree.  So I applied that fix to bpf-next as well and then
> pushed your series on top.

That seems best, thanks for resolving this.