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.kicin...@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.
