Message-ID: <20160711165140.GB22729@gmail.com>
Date:	Mon, 11 Jul 2016 09:51:42 -0700
From:	Brenden Blanco <bblanco@...mgrid.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Martin KaFai Lau <kafai@...com>,
	Jesper Dangaard Brouer <brouer@...hat.com>,
	Ari Saha <as754m@....com>,
	Alexei Starovoitov <alexei.starovoitov@...il.com>,
	Or Gerlitz <gerlitz.or@...il.com>,
	john fastabend <john.fastabend@...il.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Thomas Graf <tgraf@...g.ch>,
	Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH v6 01/12] bpf: add XDP prog type for early driver filter

On Sun, Jul 10, 2016 at 03:56:02PM -0500, Tom Herbert wrote:
> On Thu, Jul 7, 2016 at 9:15 PM, Brenden Blanco <bblanco@...mgrid.com> wrote:
> > Add a new bpf prog type that is intended to run in early stages of the
> > packet rx path. Only minimal packet metadata will be available, hence a
> > new context type, struct xdp_md, is exposed to userspace. So far only
> > expose the packet start and end pointers, and only in read mode.
> >
> > An XDP program must return one of the well known enum values, all other
> > return codes are reserved for future use. Unfortunately, this
> > restriction is hard to enforce at verification time, so take the
> > approach of warning at runtime when such programs are encountered. The
> > driver can choose to implement unknown return codes however it wants,
> > but must invoke the warning helper with the action value.
> >
> > Signed-off-by: Brenden Blanco <bblanco@...mgrid.com>
> > ---
> >  include/linux/filter.h   | 18 ++++++++++
> >  include/uapi/linux/bpf.h | 19 ++++++++++
> >  kernel/bpf/verifier.c    |  1 +
> >  net/core/filter.c        | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 129 insertions(+)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 6fc31ef..522dbc9 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -368,6 +368,11 @@ struct bpf_skb_data_end {
> >         void *data_end;
> >  };
> >
> > +struct xdp_buff {
> > +       void *data;
> > +       void *data_end;
> > +};
> > +
> >  /* compute the linear packet data range [data, data_end) which
> >   * will be accessed by cls_bpf and act_bpf programs
> >   */
> > @@ -429,6 +434,18 @@ static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
> >         return BPF_PROG_RUN(prog, skb);
> >  }
> >
> > +static inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
> > +                                  struct xdp_buff *xdp)
> > +{
> > +       u32 ret;
> > +
> > +       rcu_read_lock();
> > +       ret = BPF_PROG_RUN(prog, (void *)xdp);
> > +       rcu_read_unlock();
> > +
> > +       return ret;
> > +}
> > +
> >  static inline unsigned int bpf_prog_size(unsigned int proglen)
> >  {
> >         return max(sizeof(struct bpf_prog),
> > @@ -509,6 +526,7 @@ bool bpf_helper_changes_skb_data(void *func);
> >
> >  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
> >                                        const struct bpf_insn *patch, u32 len);
> > +void bpf_warn_invalid_xdp_action(int act);
> >
> >  #ifdef CONFIG_BPF_JIT
> >  extern int bpf_jit_enable;
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c14ca1c..5b47ac3 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -94,6 +94,7 @@ enum bpf_prog_type {
> >         BPF_PROG_TYPE_SCHED_CLS,
> >         BPF_PROG_TYPE_SCHED_ACT,
> >         BPF_PROG_TYPE_TRACEPOINT,
> > +       BPF_PROG_TYPE_XDP,
> >  };
> >
> >  #define BPF_PSEUDO_MAP_FD      1
> > @@ -430,4 +431,22 @@ struct bpf_tunnel_key {
> >         __u32 tunnel_label;
> >  };
> >
> > +/* User return codes for XDP prog type.
> > + * A valid XDP program must return one of these defined values. All other
> > + * return codes are reserved for future use. Unknown return codes will result
> > + * in driver-dependent behavior.
> > + */
> > +enum xdp_action {
> > +       XDP_DROP,
> > +       XDP_PASS,
> > +};
> > +
> > +/* user accessible metadata for XDP packet hook
> > + * new fields must be added to the end of this structure
> > + */
> > +struct xdp_md {
> > +       __u32 data;
> > +       __u32 data_end;
> > +};
> > +
> >  #endif /* _UAPI__LINUX_BPF_H__ */
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e206c21..a8d67d0 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -713,6 +713,7 @@ static int check_ptr_alignment(struct verifier_env *env, struct reg_state *reg,
> >         switch (env->prog->type) {
> >         case BPF_PROG_TYPE_SCHED_CLS:
> >         case BPF_PROG_TYPE_SCHED_ACT:
> > +       case BPF_PROG_TYPE_XDP:
> >                 break;
> >         default:
> >                 verbose("verifier is misconfigured\n");
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 10c4a2f..4ba446f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2369,6 +2369,12 @@ tc_cls_act_func_proto(enum bpf_func_id func_id)
> >         }
> >  }
> >
> > +static const struct bpf_func_proto *
> > +xdp_func_proto(enum bpf_func_id func_id)
> > +{
> > +       return sk_filter_func_proto(func_id);
> > +}
> > +
> >  static bool __is_valid_access(int off, int size, enum bpf_access_type type)
> >  {
> >         if (off < 0 || off >= sizeof(struct __sk_buff))
> 
> Can off be size_t to eliminate <0 check?
No, this is coming directly from struct bpf_insn, where it is a __s16.
This function signature shows up in lots of different places and would
be a pain to fixup.
> 
> > @@ -2436,6 +2442,56 @@ static bool tc_cls_act_is_valid_access(int off, int size,
> >         return __is_valid_access(off, size, type);
> >  }
> >
> > +static bool __is_valid_xdp_access(int off, int size,
> > +                                 enum bpf_access_type type)
> > +{
> > +       if (off < 0 || off >= sizeof(struct xdp_md))
> > +               return false;
> > +       if (off % size != 0)
> 
> off & 3 != 0
Feasible, but was intending to keep with the surrounding style. What do
the other bpf maintainers think?
> 
> > +               return false;
> > +       if (size != 4)
> > +               return false;
> 
> If size must always be 4 why is it even an argument?
Because this is the first time that the verifier has a chance to check
it, and size == 4 could potentially be a prog_type-specific requirement.
> 
> > +
> > +       return true;
> > +}
> > +
> > +static bool xdp_is_valid_access(int off, int size,
> > +                               enum bpf_access_type type,
> > +                               enum bpf_reg_type *reg_hint)
> > +{
> > +       if (type == BPF_WRITE)
> > +               return false;
> > +
> > +       switch (off) {
> > +       case offsetof(struct xdp_md, data):
> > +               *reg_hint = PTR_TO_PACKET;
> > +               break;
> > +       case offsetof(struct xdp_md, data_end):
> > +               *reg_hint = PTR_TO_PACKET_END;
> > +               break;
> case sizeof(int) for below?
> 
> > +       }
> > +
> > +       return __is_valid_xdp_access(off, size, type);
> > +}
> > +
> > +void bpf_warn_invalid_xdp_action(int act)
> > +{
> > +       WARN_ONCE(1, "\n"
> > +                    "*****************************************************\n"
> > +                    "**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n"
> > +                    "**                                               **\n"
> > +                    "** XDP program returned unknown value %-10u **\n"
> > +                    "**                                               **\n"
> > +                    "** XDP programs must return a well-known return  **\n"
> > +                    "** value. Invalid return values will result in   **\n"
> > +                    "** undefined packet actions.                     **\n"
> > +                    "**                                               **\n"
> > +                    "**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n"
> > +                    "*****************************************************\n",
> > +                 act);
> 
> Seems a little verbose to be. Just do a simple WARN_ONCE and probably
> more important to bump a counter.
The verbosity is intentional, modeled after the warning in
trace_printk_init_buffers(). Do you feel strongly on this?

A counter where? It would have to be outside of this function, there is
no common bpf infra for such things. Driver ethtool aware stats were
already mentioned.

> Also function should take the skb in
> question as an argument in case we want to do more inspection or
> logging in the future.
There is no skb to pass in, we're operating on dma'ed memory.
> 
> > +}
> > +EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
> > +
> >  static u32 bpf_net_convert_ctx_access(enum bpf_access_type type, int dst_reg,
> >                                       int src_reg, int ctx_off,
> >                                       struct bpf_insn *insn_buf,
> > @@ -2587,6 +2643,29 @@ static u32 bpf_net_convert_ctx_access(enum bpf_access_type type, int dst_reg,
> >         return insn - insn_buf;
> >  }
> >
> > +static u32 xdp_convert_ctx_access(enum bpf_access_type type, int dst_reg,
> > +                                 int src_reg, int ctx_off,
> > +                                 struct bpf_insn *insn_buf,
> > +                                 struct bpf_prog *prog)
> > +{
> > +       struct bpf_insn *insn = insn_buf;
> > +
> > +       switch (ctx_off) {
> > +       case offsetof(struct xdp_md, data):
> > +               *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(FIELD_SIZEOF(struct xdp_buff, data)),
> > +                                     dst_reg, src_reg,
> > +                                     offsetof(struct xdp_buff, data));
> > +               break;
> > +       case offsetof(struct xdp_md, data_end):
> > +               *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(FIELD_SIZEOF(struct xdp_buff, data_end)),
> > +                                     dst_reg, src_reg,
> > +                                     offsetof(struct xdp_buff, data_end));
> > +               break;
> > +       }
> > +
> > +       return insn - insn_buf;
> > +}
> > +
> >  static const struct bpf_verifier_ops sk_filter_ops = {
> >         .get_func_proto         = sk_filter_func_proto,
> >         .is_valid_access        = sk_filter_is_valid_access,
> > @@ -2599,6 +2678,12 @@ static const struct bpf_verifier_ops tc_cls_act_ops = {
> >         .convert_ctx_access     = bpf_net_convert_ctx_access,
> >  };
> >
> > +static const struct bpf_verifier_ops xdp_ops = {
> > +       .get_func_proto         = xdp_func_proto,
> > +       .is_valid_access        = xdp_is_valid_access,
> > +       .convert_ctx_access     = xdp_convert_ctx_access,
> > +};
> > +
> >  static struct bpf_prog_type_list sk_filter_type __read_mostly = {
> >         .ops    = &sk_filter_ops,
> >         .type   = BPF_PROG_TYPE_SOCKET_FILTER,
> > @@ -2614,11 +2699,17 @@ static struct bpf_prog_type_list sched_act_type __read_mostly = {
> >         .type   = BPF_PROG_TYPE_SCHED_ACT,
> >  };
> >
> > +static struct bpf_prog_type_list xdp_type __read_mostly = {
> > +       .ops    = &xdp_ops,
> > +       .type   = BPF_PROG_TYPE_XDP,
> > +};
> > +
> >  static int __init register_sk_filter_ops(void)
> >  {
> >         bpf_register_prog_type(&sk_filter_type);
> >         bpf_register_prog_type(&sched_cls_type);
> >         bpf_register_prog_type(&sched_act_type);
> > +       bpf_register_prog_type(&xdp_type);
> >
> >         return 0;
> >  }
> > --
> > 2.8.2
> >

Powered by blists - more mailing lists