On Wed, Jun 02, 2021 at 07:08:13PM +0000, Zvi Effron wrote:
[ ... ]
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index aa47af349ba8..ae8741dd2a54 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -687,6 +687,43 @@ 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)
> +{
> + void *data;
> + u32 metalen;
> +
> + if (!xdp_md)
> + return 0;
> +
> + if (xdp_md->egress_ifindex != 0)
> + return -EINVAL;
> +
> + metalen = xdp_md->data - xdp_md->data_meta;
Remove "- xdp_md->data_meta". It is 0.
> + data = xdp->data_meta + metalen;
> + if (data > xdp->data_end)
This test and...
> + return -EINVAL;
> + xdp->data = data;
> +
> + if (xdp_md->data_end - xdp_md->data != xdp->data_end - xdp->data)
this one. It is because the user input "xdp_md->data_end" does not
match with kattr->test.data_size_in? These tests are disconnected from
where the actual invalid input is. How about direclty testing
xdp_md->data_end in bpf_prog_test_run_xdp() instead?
> + return -EINVAL;
> +
> + if (xdp_md->ingress_ifindex != 0 || xdp_md->rx_queue_index != 0)
> + return -EINVAL;
> +
> + 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;
> + xdp_md->data = xdp->data - xdp->data_meta;
> + xdp_md->data_end = xdp->data_end - xdp->data_meta;
> +}
> +
> int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> union bpf_attr __user *uattr)
> {
> @@ -696,36 +733,68 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> u32 repeat = kattr->test.repeat;
> struct netdev_rx_queue *rxqueue;
> struct xdp_buff xdp = {};
> + struct xdp_md *ctx = NULL;
"= NULL" is not needed.
> u32 retval, duration;
> u32 max_data_sz;
> + u32 metalen;
> void *data;
> int ret;
>
> - if (kattr->test.ctx_in || kattr->test.ctx_out)
> - return -EINVAL;
> + ctx = bpf_ctx_init(kattr, sizeof(struct xdp_md));
> + if (IS_ERR(ctx))
> + return PTR_ERR(ctx);
> +
> + /* There can't be user provided data before the metadata */
> + if (ctx) {
> + if (ctx->data_meta != 0)
nit. "if (ctx->data_meta)".
> + return -EINVAL;
> + metalen = ctx->data - ctx->data_meta;
It is confusing. "ctx->data_meta != 0" has just been checked.
> + if (unlikely((metalen & (sizeof(__u32) - 1)) ||
> + metalen > 32))
> + return -EINVAL;
> + /* Metadata is allocated from the headroom */
> + headroom -= metalen;
> + }
>
> /* XDP have extra tailroom as (most) drivers use full page */
> max_data_sz = 4096 - headroom - tailroom;
>
> data = bpf_test_init(kattr, max_data_sz, headroom, tailroom);
> - if (IS_ERR(data))
> + if (IS_ERR(data)) {
> + kfree(ctx);
> return PTR_ERR(data);
> + }
>
> rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
> xdp_init_buff(&xdp, headroom + max_data_sz + tailroom,
> &rxqueue->xdp_rxq);
> xdp_prepare_buff(&xdp, data, headroom, size, true);
>
> + ret = xdp_convert_md_to_buff(&xdp, ctx);
> + if (ret) {
> + kfree(data);
> + kfree(ctx);
> + return ret;
> + }
> +
> bpf_prog_change_xdp(NULL, prog);
> ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration, true);
> if (ret)
> goto out;
> - if (xdp.data != data + headroom || xdp.data_end != xdp.data + size)
> - size = xdp.data_end - xdp.data;
> - ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration);
> +
> + if (xdp.data_meta != data + headroom || xdp.data_end != xdp.data_meta + size)
> + size = xdp.data_end - xdp.data_meta;
> +
> + xdp_convert_buff_to_md(&xdp, ctx);
> +
> + ret = bpf_test_finish(kattr, uattr, xdp.data_meta, size, retval, duration);
> + if (!ret)
> + ret = bpf_ctx_finish(kattr, uattr, ctx,
> + sizeof(struct xdp_md));
> out:
> bpf_prog_change_xdp(prog, NULL);
> kfree(data);
> + kfree(ctx);
> return ret;
> }
>
> @@ -809,7 +878,6 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> if (!ret)
> ret = bpf_ctx_finish(kattr, uattr, user_ctx,
> sizeof(struct bpf_flow_keys));
> -
> out:
> kfree(user_ctx);
> kfree(data);
> --
> 2.31.1
>
