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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sat, Aug 27, 2016 at 12:40:04PM +0100, Jakub Kicinski wrote:
> 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()

probably array_of_insn_aux_data[num_insns] should do it.
Unlike reg_state that is forked on branches, this array
is only one.

> 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

I see. Indeed then you'd need the verifier to walk all paths
to make sure constant return values.
If you only need yes/no check then such info can probably be
collected unconditionally during initial program load.
Like prog->cb_access flag.