Re: [RFCv2 07/16] bpf: enable non-core use of the verfier

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Fri, 26 Aug 2016 16:29:05 -0700, Alexei Starovoitov wrote:
> On Fri, Aug 26, 2016 at 07:06:06PM +0100, Jakub Kicinski wrote:
> > Advanced JIT compilers and translators may want to use
> > eBPF verifier as a base for parsers or to perform custom
> > checks and validations.
> > 
> > Add ability for external users to invoke the verifier
> > and provide callbacks to be invoked for every intruction
> > checked.  For now only add most basic callback for
> > per-instruction pre-interpretation checks is added.  More
> > advanced users may also like to have per-instruction post
> > callback and state comparison callback.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>  
> 
> I like the apporach. Making verifier into 'bytecode parser'
> that JITs can reuse is a good design choice.
> The only thing I would suggest is to tweak the verifier to
> avoid in-place state recording. Then I think patch 8 for
> clone/unclone of the program won't be needed, since verifier
> will be read-only from bytecode point of view and patch 9
> also will be slightly cleaner.
> I think there are very few places in verifier that do this
> state keeping inside insn. It was bugging me for some time.
> Good time to clean that up.
> Unless I misunderstand the patches 7,8,9...

Agreed, I think the verifier only modifies the program to
store pointer types in imm field.  I will try to come up
a way around this, any suggestions?  Perhaps state_equal()
logic could be modified to downgrade pointers to UNKONWNs
when it detects other state had incompatible pointer type.

> There is also small concern for patches 5 and 6 that add
> more register state information. Potentially that extra
> state can prevent states_equal() to recognize equivalent
> states. Only patch 9 uses that info, right?

5 and 6 recognize more constant loads, those can only
upgrade some UNKNOWN_VALUEs to CONST_IMMs.  So yes, if the
verifier hits the CONST first and then tries with UNKNOWN
it will have to reverify the path.  

> Another question is do you need all state walking that
> verifier does or single linear pass through insns
> would have worked?
> Looks like you're only using CONST_IMM and PTR_TO_CTX
> state, right?

I think I need all the parsing.  Right now I mostly need
the verification to check that exit codes are specific
CONST_IMMs.  Clang quite happily does this:

r0 <- 0
if (...)
	r0 <- 1
exit

> The rest looks very good. Thanks a lot!

Thanks for the review!  FWIW my use of parsing is isolated
to the nfp_bpf_verifier.c file, at the very end of patch 9.