[PATCH bpf-next v2 0/3] bpf: support input xdp_md context in BPF_PROG_TEST_RUN

[PATCH bpf-next v2 0/3] bpf: support input xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

This patchset adds support for passing an xdp_md via ctx_in/ctx_out in bpf_attr
for BPF_PROG_TEST_RUN of XDP programs.

Patch 1 adds initial support for passing XDP meta data in addition to packet
data.

Patch 2 adds support for also specifying the ingress interface and rx queue.

Patch 3 adds selftests to ensure functionality is correct.

Changelog:
----------
v1 -> v2
v1: https://lore.kernel.org/bpf/20210524220555.251473-1-zeffron@riotgames.com

 * Fix null pointer dereference with no context
 * Use the BPF skeleton and replace CHECK with ASSERT macros

Zvi Effron (3):
  bpf: support input xdp_md context in BPF_PROG_TEST_RUN
  bpf: support specifying ingress via xdp_md context in
    BPF_PROG_TEST_RUN
  selftests/bpf: Add test for xdp_md context in BPF_PROG_TEST_RUN

 include/uapi/linux/bpf.h                      |   3 -
 net/bpf/test_run.c                            |  96 +++++++++++++--
 .../bpf/prog_tests/xdp_context_test_run.c     | 116 ++++++++++++++++++
 .../bpf/progs/test_xdp_context_test_run.c     |  20 +++
 4 files changed, 225 insertions(+), 10 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c


base-commit: d6a6a55518c16040a369360255b355b7a2a261de
-- 
2.31.1

[PATCH bpf-next v2 1/3] bpf: support input xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

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@riotgames.com>
Signed-off-by: Cody Haas <chaas@riotgames.com>
Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com>
Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com>
Signed-off-by: Zvi Effron <zeffron@riotgames.com>
---
 include/uapi/linux/bpf.h |  3 --
 net/bpf/test_run.c       | 84 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 77 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..3718c8a331dc 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -687,6 +687,45 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	return ret;
 }
 
+static int convert_xdpmd_to_xdpb(struct xdp_buff *xdp, struct xdp_md *xdp_md)
+{
+	void *data;
+	u32 metalen;
+	struct net_device *device;
+	struct netdev_rx_queue *rxqueue;
+
+	if (!xdp_md)
+		return 0;
+
+	if (xdp_md->egress_ifindex != 0)
+		return -EINVAL;
+
+	metalen = xdp_md->data - xdp_md->data_meta;
+	data = xdp->data_meta + metalen;
+	if (data > xdp->data_end)
+		return -EINVAL;
+	xdp->data = data;
+
+	if (xdp_md->data_end - xdp_md->data != xdp->data_end - xdp->data)
+		return -EINVAL;
+
+	if (xdp_md->ingress_ifindex != 0 || xdp_md->rx_queue_index != 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void convert_xdpb_to_xdpmd(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 +735,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;
 	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)
+			return -EINVAL;
+		metalen = ctx->data - ctx->data_meta;
+		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 = convert_xdpmd_to_xdpb(&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;
+
+	convert_xdpb_to_xdpmd(&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 +880,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

[PATCH bpf-next v2 2/3] bpf: support specifying ingress via xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

Support specifying the ingress_ifindex and rx_queue_index of xdp_md
contexts for BPF_PROG_TEST_RUN.

The intended use case is to allow testing XDP programs that make decisions
based on the ingress interface or RX queue.

If ingress_ifindex is specified, look up the device by the provided index
in the current namespace and use its xdp_rxq for the xdp_buff. If the
rx_queue_index is out of range, or is non-zero when the ingress_ifindex is
0, return EINVAL.

Co-developed-by: Cody Haas <chaas@riotgames.com>
Signed-off-by: Cody Haas <chaas@riotgames.com>
Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com>
Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com>
Signed-off-by: Zvi Effron <zeffron@riotgames.com>
---
 net/bpf/test_run.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 3718c8a331dc..e14797eb1e14 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -709,9 +709,21 @@ static int convert_xdpmd_to_xdpb(struct xdp_buff *xdp, struct xdp_md *xdp_md)
 	if (xdp_md->data_end - xdp_md->data != xdp->data_end - xdp->data)
 		return -EINVAL;
 
-	if (xdp_md->ingress_ifindex != 0 || xdp_md->rx_queue_index != 0)
+	if (!xdp_md->ingress_ifindex && xdp_md->rx_queue_index)
 		return -EINVAL;
 
+	if (xdp_md->ingress_ifindex) {
+		device = dev_get_by_index(current->nsproxy->net_ns, xdp_md->ingress_ifindex);
+		if (!device)
+			return -EINVAL;
+
+		if (xdp_md->rx_queue_index >= device->real_num_rx_queues)
+			return -EINVAL;
+
+		rxqueue = __netif_get_rx_queue(device, xdp_md->rx_queue_index);
+		xdp->rxq = &rxqueue->xdp_rxq;
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH bpf-next v2 3/3] selftests/bpf: Add test for xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

Add a test for using xdp_md as a context to BPF_PROG_TEST_RUN for XDP
programs.

The test uses a BPF program that takes in a return value from XDP
metadata, then reduces the size of the XDP metadata by 4 bytes.

Test cases validate the possible failure cases for passing in invalid
xdp_md contexts, that the return value is successfully passed
in, and that the adjusted metadata is successfully copied out.

Co-developed-by: Cody Haas <chaas@riotgames.com>
Signed-off-by: Cody Haas <chaas@riotgames.com>
Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com>
Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com>
Signed-off-by: Zvi Effron <zeffron@riotgames.com>
---
 .../bpf/prog_tests/xdp_context_test_run.c     | 116 ++++++++++++++++++
 .../bpf/progs/test_xdp_context_test_run.c     |  20 +++
 2 files changed, 136 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
new file mode 100644
index 000000000000..f6d312005b7c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+#include "test_xdp_context_test_run.skel.h"
+
+void test_xdp_context_test_run(void)
+{
+	struct test_xdp_context_test_run *skel = NULL;
+	char data[sizeof(pkt_v4) + sizeof(__u32)];
+	char buf[128];
+	char bad_ctx[sizeof(struct xdp_md)];
+	struct xdp_md ctx_in, ctx_out;
+	struct bpf_test_run_opts tattr = {
+		.sz = sizeof(struct bpf_test_run_opts),
+		.data_in = &data,
+		.data_out = buf,
+		.data_size_in = sizeof(data),
+		.data_size_out = sizeof(buf),
+		.ctx_out = &ctx_out,
+		.ctx_size_out = sizeof(ctx_out),
+		.repeat = 1,
+	};
+	int err, prog_fd;
+
+
+	skel = test_xdp_context_test_run__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel"))
+		return;
+	prog_fd = bpf_program__fd(skel->progs._xdp_context);
+
+	*(__u32 *)data = XDP_PASS;
+	*(struct ipv4_packet *)(data + sizeof(__u32)) = pkt_v4;
+
+	memset(&ctx_in, 0, sizeof(ctx_in));
+	tattr.ctx_in = &ctx_in;
+	tattr.ctx_size_in = sizeof(ctx_in);
+
+	tattr.ctx_in = &ctx_in;
+	tattr.ctx_size_in = sizeof(ctx_in);
+	ctx_in.data_meta = 0;
+	ctx_in.data = sizeof(__u32);
+	ctx_in.data_end = ctx_in.data + sizeof(pkt_v4);
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_OK(err, "bpf_prog_test_run(test1)");
+	ASSERT_EQ(tattr.retval, XDP_PASS, "test1-retval");
+	ASSERT_EQ(tattr.data_size_out, sizeof(pkt_v4), "test1-datasize");
+	ASSERT_EQ(tattr.ctx_size_out, tattr.ctx_size_in, "test1-ctxsize");
+	ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta");
+	ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data");
+	ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend");
+
+	/* Data past the end of the kernel's struct xdp_md must be 0 */
+	bad_ctx[sizeof(bad_ctx) - 1] = 1;
+	tattr.ctx_in = bad_ctx;
+	tattr.ctx_size_in = sizeof(bad_ctx);
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test2)");
+	ASSERT_EQ(errno, 22, "test2-errno");
+
+	/* The egress cannot be specified */
+	ctx_in.egress_ifindex = 1;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test3)");
+	ASSERT_EQ(errno, 22, "test3-errno");
+
+	/* data_meta must reference the start of data */
+	ctx_in.data_meta = sizeof(__u32);
+	ctx_in.data = ctx_in.data_meta;
+	ctx_in.data_end = ctx_in.data + sizeof(pkt_v4);
+	ctx_in.egress_ifindex = 0;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test4)");
+	ASSERT_EQ(errno, 22, "test4-errno");
+
+	/* Metadata must be 32 bytes or smaller */
+	ctx_in.data_meta = 0;
+	ctx_in.data = sizeof(__u32)*9;
+	ctx_in.data_end = ctx_in.data + sizeof(pkt_v4);
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test5)");
+	ASSERT_EQ(errno, 22, "test5-errno");
+
+	/* Metadata's size must be a multiple of 4 */
+	ctx_in.data = 3;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test6)");
+	ASSERT_EQ(errno, 22, "test6-errno");
+
+	/* Total size of data must match data_end - data_meta */
+	ctx_in.data = 0;
+	ctx_in.data_end = sizeof(pkt_v4) - 4;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test7)");
+	ASSERT_EQ(errno, 22, "test7-errno");
+
+	ctx_in.data_end = sizeof(pkt_v4) + 4;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test8)");
+	ASSERT_EQ(errno, 22, "test8-errno");
+
+	/* RX queue cannot be specified without specifying an ingress */
+	ctx_in.data_end = sizeof(pkt_v4);
+	ctx_in.ingress_ifindex = 0;
+	ctx_in.rx_queue_index = 1;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test9)");
+	ASSERT_EQ(errno, 22, "test9-errno");
+
+	ctx_in.ingress_ifindex = 1;
+	ctx_in.rx_queue_index = 1;
+	err = bpf_prog_test_run_opts(prog_fd, &tattr);
+	ASSERT_ERR(err, "bpf_prog_test_run(test10)");
+	ASSERT_EQ(errno, 22, "test10-errno");
+
+	test_xdp_context_test_run__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c
new file mode 100644
index 000000000000..56fd0995b67c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp")
+int _xdp_context(struct xdp_md *xdp)
+{
+	void *data = (void *)(unsigned long)xdp->data;
+	__u32 *metadata = (void *)(unsigned long)xdp->data_meta;
+	__u32 ret;
+
+	if (metadata + 1 > data)
+		return XDP_ABORTED;
+	ret = *metadata;
+	if (bpf_xdp_adjust_meta(xdp, 4))
+		return XDP_ABORTED;
+	return ret;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.31.1

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add test for xdp_md context in BPF_PROG_TEST_RUN

[Andrii Nakryiko]

On Thu, May 27, 2021 at 1:14 PM Zvi Effron <zeffron@riotgames.com> wrote:
>
> Add a test for using xdp_md as a context to BPF_PROG_TEST_RUN for XDP
> programs.
>
> The test uses a BPF program that takes in a return value from XDP
> metadata, then reduces the size of the XDP metadata by 4 bytes.
>
> Test cases validate the possible failure cases for passing in invalid
> xdp_md contexts, that the return value is successfully passed
> in, and that the adjusted metadata is successfully copied out.
>
> Co-developed-by: Cody Haas <chaas@riotgames.com>
> Signed-off-by: Cody Haas <chaas@riotgames.com>
> Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com>
> Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com>
> Signed-off-by: Zvi Effron <zeffron@riotgames.com>
> ---
>  .../bpf/prog_tests/xdp_context_test_run.c     | 116 ++++++++++++++++++
>  .../bpf/progs/test_xdp_context_test_run.c     |  20 +++
>  2 files changed, 136 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> new file mode 100644
> index 000000000000..f6d312005b7c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include "test_xdp_context_test_run.skel.h"
> +
> +void test_xdp_context_test_run(void)
> +{
> +       struct test_xdp_context_test_run *skel = NULL;
> +       char data[sizeof(pkt_v4) + sizeof(__u32)];
> +       char buf[128];
> +       char bad_ctx[sizeof(struct xdp_md)];
> +       struct xdp_md ctx_in, ctx_out;
> +       struct bpf_test_run_opts tattr = {

see LIBBPF_DECLARE_OPTS, please use it

and please call it opts, it's not attribute

> +               .sz = sizeof(struct bpf_test_run_opts),
> +               .data_in = &data,
> +               .data_out = buf,
> +               .data_size_in = sizeof(data),
> +               .data_size_out = sizeof(buf),
> +               .ctx_out = &ctx_out,
> +               .ctx_size_out = sizeof(ctx_out),
> +               .repeat = 1,
> +       };
> +       int err, prog_fd;
> +
> +

extra empty line

> +       skel = test_xdp_context_test_run__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "skel"))
> +               return;
> +       prog_fd = bpf_program__fd(skel->progs._xdp_context);
> +
> +       *(__u32 *)data = XDP_PASS;
> +       *(struct ipv4_packet *)(data + sizeof(__u32)) = pkt_v4;
> +
> +       memset(&ctx_in, 0, sizeof(ctx_in));
> +       tattr.ctx_in = &ctx_in;
> +       tattr.ctx_size_in = sizeof(ctx_in);
> +
> +       tattr.ctx_in = &ctx_in;
> +       tattr.ctx_size_in = sizeof(ctx_in);
> +       ctx_in.data_meta = 0;
> +       ctx_in.data = sizeof(__u32);
> +       ctx_in.data_end = ctx_in.data + sizeof(pkt_v4);
> +       err = bpf_prog_test_run_opts(prog_fd, &tattr);
> +       ASSERT_OK(err, "bpf_prog_test_run(test1)");
> +       ASSERT_EQ(tattr.retval, XDP_PASS, "test1-retval");
> +       ASSERT_EQ(tattr.data_size_out, sizeof(pkt_v4), "test1-datasize");
> +       ASSERT_EQ(tattr.ctx_size_out, tattr.ctx_size_in, "test1-ctxsize");
> +       ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta");
> +       ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data");
> +       ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend");
> +
> +       /* Data past the end of the kernel's struct xdp_md must be 0 */
> +       bad_ctx[sizeof(bad_ctx) - 1] = 1;
> +       tattr.ctx_in = bad_ctx;
> +       tattr.ctx_size_in = sizeof(bad_ctx);
> +       err = bpf_prog_test_run_opts(prog_fd, &tattr);
> +       ASSERT_ERR(err, "bpf_prog_test_run(test2)");
> +       ASSERT_EQ(errno, 22, "test2-errno");

by the time you are checking errno it might get overwritten. If you
want to check errno, you have to remember it right after the function
returns

> +
> +       /* The egress cannot be specified */
> +       ctx_in.egress_ifindex = 1;
> +       err = bpf_prog_test_run_opts(prog_fd, &tattr);
> +       ASSERT_ERR(err, "bpf_prog_test_run(test3)");
> +       ASSERT_EQ(errno, 22, "test3-errno");
> +

[...]

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add test for xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

On Thu, May 27, 2021 at 6:28 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, May 27, 2021 at 1:14 PM Zvi Effron <zeffron@riotgames.com> wrote:
> >
> > +
> > +       /* Data past the end of the kernel's struct xdp_md must be 0 */
> > +       bad_ctx[sizeof(bad_ctx) - 1] = 1;
> > +       tattr.ctx_in = bad_ctx;
> > +       tattr.ctx_size_in = sizeof(bad_ctx);
> > +       err = bpf_prog_test_run_opts(prog_fd, &tattr);
> > +       ASSERT_ERR(err, "bpf_prog_test_run(test2)");
> > +       ASSERT_EQ(errno, 22, "test2-errno");
>
> by the time you are checking errno it might get overwritten. If you
> want to check errno, you have to remember it right after the function
> returns

Is it sufficient to simply make the errno ASSERT the first thing after
the function returns? Or would we still need to preserve it into a
local variable?

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add test for xdp_md context in BPF_PROG_TEST_RUN

[Andrii Nakryiko]

On Thu, May 27, 2021 at 7:11 PM Zvi Effron <zeffron@riotgames.com> wrote:
>
> On Thu, May 27, 2021 at 6:28 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, May 27, 2021 at 1:14 PM Zvi Effron <zeffron@riotgames.com> wrote:
> > >
> > > +
> > > +       /* Data past the end of the kernel's struct xdp_md must be 0 */
> > > +       bad_ctx[sizeof(bad_ctx) - 1] = 1;
> > > +       tattr.ctx_in = bad_ctx;
> > > +       tattr.ctx_size_in = sizeof(bad_ctx);
> > > +       err = bpf_prog_test_run_opts(prog_fd, &tattr);
> > > +       ASSERT_ERR(err, "bpf_prog_test_run(test2)");
> > > +       ASSERT_EQ(errno, 22, "test2-errno");
> >
> > by the time you are checking errno it might get overwritten. If you
> > want to check errno, you have to remember it right after the function
> > returns
>
> Is it sufficient to simply make the errno ASSERT the first thing after
> the function returns? Or would we still need to preserve it into a
> local variable?

Yes, doing ASSERT_EQ(errno, 22, "test2-errno") right after
bpf_prog_test_run_opts() will work reliably.

Re: [PATCH bpf-next v2 1/3] bpf: support input xdp_md context in BPF_PROG_TEST_RUN

[Maciej Fijalkowski]

On Thu, May 27, 2021 at 08:13:39PM +0000, 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.

How about providing an actual selftests that will showcase the above so
reviewers could get in an easier way a grasp of what this set is about?

> 
> 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@riotgames.com>
> Signed-off-by: Cody Haas <chaas@riotgames.com>
> Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com>
> Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com>
> Signed-off-by: Zvi Effron <zeffron@riotgames.com>
> ---
>  include/uapi/linux/bpf.h |  3 --
>  net/bpf/test_run.c       | 84 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 77 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..3718c8a331dc 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -687,6 +687,45 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	return ret;
>  }
>  
> +static int convert_xdpmd_to_xdpb(struct xdp_buff *xdp, struct xdp_md *xdp_md)

Maybe xdp_convert_md_to_buff ?

> +{
> +	void *data;
> +	u32 metalen;
> +	struct net_device *device;
> +	struct netdev_rx_queue *rxqueue;

rxqueue is unused in here, maybe it with 2nd patch?

> +
> +	if (!xdp_md)
> +		return 0;
> +
> +	if (xdp_md->egress_ifindex != 0)
> +		return -EINVAL;
> +
> +	metalen = xdp_md->data - xdp_md->data_meta;
> +	data = xdp->data_meta + metalen;
> +	if (data > xdp->data_end)
> +		return -EINVAL;
> +	xdp->data = data;
> +
> +	if (xdp_md->data_end - xdp_md->data != xdp->data_end - xdp->data)
> +		return -EINVAL;
> +
> +	if (xdp_md->ingress_ifindex != 0 || xdp_md->rx_queue_index != 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void convert_xdpb_to_xdpmd(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 +735,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;
>  	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)
> +			return -EINVAL;
> +		metalen = ctx->data - ctx->data_meta;
> +		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 = convert_xdpmd_to_xdpb(&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;
> +
> +	convert_xdpb_to_xdpmd(&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 +880,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
> 

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add test for xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

On Thu, May 27, 2021 at 6:28 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, May 27, 2021 at 1:14 PM Zvi Effron <zeffron@riotgames.com> wrote:
> >
> > +       /* Data past the end of the kernel's struct xdp_md must be 0 */
> > +       bad_ctx[sizeof(bad_ctx) - 1] = 1;
> > +       tattr.ctx_in = bad_ctx;
> > +       tattr.ctx_size_in = sizeof(bad_ctx);
> > +       err = bpf_prog_test_run_opts(prog_fd, &tattr);
> > +       ASSERT_ERR(err, "bpf_prog_test_run(test2)");
> > +       ASSERT_EQ(errno, 22, "test2-errno");
>
> by the time you are checking errno it might get overwritten. If you
> want to check errno, you have to remember it right after the function
> returns
>

I just noticed that the CHECK macro (which the ASSERT macro wraps)
already saves/restores errno. Is this not behavior that can be relied
on?

Re: [PATCH bpf-next v2 1/3] bpf: support input xdp_md context in BPF_PROG_TEST_RUN

[Zvi Effron]

On Fri, May 28, 2021 at 2:43 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Thu, May 27, 2021 at 08:13:39PM +0000, 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.
>
> How about providing an actual selftests that will showcase the above so
> reviewers could get in an easier way a grasp of what this set is about?
>

The very first test case in the added selftests (patch 3) does exactly
that. The return code is passed via XDP meta data to the tested
program (a program that normally would be invoked via tail call). This
is the exact use case we have that prompted this patch set.