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