Re: [PATCHv3 net-next 05/15] bpf: enable non-core use of the verfier

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

On Wed, 14 Sep 2016 16:05:51 -0700, Alexei Starovoitov wrote:
> On Wed, Sep 14, 2016 at 08:00:13PM +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>
> > ---
> >  include/linux/bpf_parser.h |  89 ++++++++++++++++++++++++++++++
> >  kernel/bpf/verifier.c      | 134 +++++++++++++++++++++++----------------------
> >  2 files changed, 158 insertions(+), 65 deletions(-)
> >  create mode 100644 include/linux/bpf_parser.h
> > 
> > diff --git a/include/linux/bpf_parser.h b/include/linux/bpf_parser.h
> > new file mode 100644
> > index 000000000000..daa53b204f4d
> > --- /dev/null
> > +++ b/include/linux/bpf_parser.h  
> 
> 'bpf parser' is a bit misleading name, since it can be interpreted
> as parser written in bpf.
> Also the header file containes verifier bits, therefore I think
> the better name would be bpf_verifier.h ?
> 
> > +#define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */
> > +
> > +struct verifier_env;
> > +struct bpf_ext_parser_ops {
> > +	int (*insn_hook)(struct verifier_env *env,
> > +			 int insn_idx, int prev_insn_idx);
> > +};  
> 
> How about calling this bpf_ext_analyzer_ops
> and main entry bpf_analyzer() ?
> I think it will better convey what it's doing.
> 
> > +
> > +/* single container for all structs
> > + * one verifier_env per bpf_check() call
> > + */
> > +struct verifier_env {
> > +	struct bpf_prog *prog;		/* eBPF program being verified */
> > +	struct verifier_stack_elem *head; /* stack of verifier states to be processed */
> > +	int stack_size;			/* number of states to be processed */
> > +	struct verifier_state cur_state; /* current verifier state */
> > +	struct verifier_state_list **explored_states; /* search pruning optimization */
> > +	const struct bpf_ext_parser_ops *pops; /* external parser ops */
> > +	void *ppriv; /* pointer to external parser's private data */  
> 
> a bit hard to review, since move and addition is in one patch.

Agreed, I'll do move+prefix with bpf_ to one patch since they're both
"no functional changes" and additions to a separate one.

> I think ppriv and pops are too obscure names.
> May be analyzer_ops and analyzer_priv ?

I'll rename everything as suggested.
 
> Conceptually looks good.

Thanks!