On Sat, Jun 05, 2021 at 08:17:00PM -0700, Yonghong Song wrote:
>
>
> On 6/4/21 3:02 PM, Zvi Effron wrote:
> > Support passing a xdp_md via ctx_in/ctx_out in bpf_attr for
> > BPF_PROG_TEST_RUN.
> >
> > The intended use case is to pass some XDP meta data to the test runs of
> > XDP programs that are used as tail calls.
> >
> > For programs that use bpf_prog_test_run_xdp, support xdp_md input and
> > output. Unlike with an actual xdp_md during a non-test run, data_meta must
> > be 0 because it must point to the start of the provided user data. From
> > the initial xdp_md, use data and data_end to adjust the pointers in the
> > generated xdp_buff. All other non-zero fields are prohibited (with
> > EINVAL). If the user has set ctx_out/ctx_size_out, copy the (potentially
> > different) xdp_md back to the userspace.
> >
> > We require all fields of input xdp_md except the ones we explicitly
> > support to be set to zero. The expectation is that in the future we might
> > add support for more fields and we want to fail explicitly if the user
> > runs the program on the kernel where we don't yet support them.
> >
> > Co-developed-by: Cody Haas <chaas@xxxxxxxxxxxxx>
> > Signed-off-by: Cody Haas <chaas@xxxxxxxxxxxxx>
> > Co-developed-by: Lisa Watanabe <lwatanabe@xxxxxxxxxxxxx>
> > Signed-off-by: Lisa Watanabe <lwatanabe@xxxxxxxxxxxxx>
> > Signed-off-by: Zvi Effron <zeffron@xxxxxxxxxxxxx>
> > ---
> > include/uapi/linux/bpf.h | 3 --
> > net/bpf/test_run.c | 77 ++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 70 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 2c1ba70abbf1..a9dcf3d8c85a 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -324,9 +324,6 @@ union bpf_iter_link_info {
> > * **BPF_PROG_TYPE_SK_LOOKUP**
> > * *data_in* and *data_out* must be NULL.
> > *
> > - * **BPF_PROG_TYPE_XDP**
> > - * *ctx_in* and *ctx_out* must be NULL.
> > - *
> > * **BPF_PROG_TYPE_RAW_TRACEPOINT**,
> > * **BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE**
> > *
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index aa47af349ba8..698618f2b27e 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -687,6 +687,38 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> > return ret;
> > }
> > +static int xdp_convert_md_to_buff(struct xdp_buff *xdp, struct xdp_md *xdp_md)
>
> Should the order of parameters be switched to (xdp_md, xdp)?
> This will follow the convention of below function xdp_convert_buff_to_md().
>
> > +{
> > + void *data;
> > +
> > + if (!xdp_md)
> > + return 0;
> > +
> > + if (xdp_md->egress_ifindex != 0)
> > + return -EINVAL;
> > +
> > + if (xdp_md->data > xdp_md->data_end)
> > + return -EINVAL;
> > +
> > + xdp->data = xdp->data_meta + xdp_md->data;
> > +
> > + if (xdp_md->ingress_ifindex != 0 || xdp_md->rx_queue_index != 0)
> > + return -EINVAL;
>
> It would be good if you did all error checking before doing xdp->data
> assignment. Also looks like xdp_md error checking happens here and
> bpf_prog_test_run_xdp(). If it is hard to put all error checking
> in bpf_prog_test_run_xdp(), at least put "xdp_md->data > xdp_md->data_end)
> in bpf_prog_test_run_xdp(),
+1 on at least all data_meta/data/data_end checks should be in one place
in bpf_prog_test_run_xdp().
> so this function only
> checks *_ifindex and rx_queue_index?
>
>
> > +
> > + return 0;
> > +}
> > +
> > +static void xdp_convert_buff_to_md(struct xdp_buff *xdp, struct xdp_md *xdp_md)
> > +{
> > + if (!xdp_md)
> > + return;
> > +
> > + /* xdp_md->data_meta must always point to the start of the out buffer */
> > + xdp_md->data_meta = 0;
Is this necessary? data_meta should not have been changed.
> > + xdp_md->data = xdp->data - xdp->data_meta;
> > + xdp_md->data_end = xdp->data_end - xdp->data_meta;
> > +}
> > +
