On 6/9/21 10:07 AM, Zvi Effron wrote:
On Sat, Jun 5, 2021 at 11:19 PM Yonghong Song <yhs@xxxxxx> wrote:
On 6/4/21 3:02 PM, Zvi Effron wrote:
+ opts.ctx_in = &ctx_in;
+ opts.ctx_size_in = sizeof(ctx_in);
+
+ opts.ctx_in = &ctx_in;
+ opts.ctx_size_in = sizeof(ctx_in);
The above two assignments are redundant.
Good catch.
+ 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, &opts);
+ ASSERT_OK(err, "bpf_prog_test_run(test1)");
+ ASSERT_EQ(opts.retval, XDP_PASS, "test1-retval");
+ ASSERT_EQ(opts.data_size_out, sizeof(pkt_v4), "test1-datasize");
+ ASSERT_EQ(opts.ctx_size_out, opts.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");
I suggest just to test ctx_out.data == 0. It just happens
the input data - meta = 4 and bpf program adjuested by 4.
If they are not the same, the result won't be equal to data_meta.
Sure.
+ 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;
+ opts.ctx_in = bad_ctx;
+ opts.ctx_size_in = sizeof(bad_ctx);
+ err = bpf_prog_test_run_opts(prog_fd, &opts);
+ ASSERT_EQ(errno, 22, "test2-errno");
+ ASSERT_ERR(err, "bpf_prog_test_run(test2)");
I suggest to drop this test. Basically you did here
is to have non-zero egress_ifindex which is not allowed.
You have a test below.
We think the actual correction here is that bad_ctx is supposed to be one byte
larger than than struct xdp_md. It is misdeclared. We'll correct that.
+
+ /* The egress cannot be specified */
+ ctx_in.egress_ifindex = 1;
+ err = bpf_prog_test_run_opts(prog_fd, &opts);
+ ASSERT_EQ(errno, 22, "test3-errno");
Use EINVAL explicitly? The same for below a few other cases.
Good suggestion.
+ ASSERT_ERR(err, "bpf_prog_test_run(test3)");
+
+ /* 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, &opts);
+ ASSERT_EQ(errno, 22, "test4-errno");
+ ASSERT_ERR(err, "bpf_prog_test_run(test4)");
+
+ /* 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, &opts);
+ ASSERT_EQ(errno, 22, "test5-errno");
+ ASSERT_ERR(err, "bpf_prog_test_run(test5)");
This test is not necessary if ctx size should be
<= sizeof(struct xdp_md). So far, I think we can
require it must be sizeof(struct xdp_md). If
in the future, kernel struct xdp_md is extended,
it may be changed to accept both old and new
xdp_md's similar to other uapi data strcture
like struct bpf_prog_info if there is a desire.
In my opinion, the kernel should just stick
to sizeof(struct xdp_md) size since the functionality
is implemented as a *testing* mechanism.
You might be confusing the context (struct xdp_md) with the XDP metadata (data
just before the frame data). XDP allows at most 32 bytes of metadata. This test
is verifying that a metadata size >32 bytes is rejected.
Right, you can keep this test. Previously I suggested to enforce
sizeof(struct xdp_md) as the ctx size, but that may be too restrictive.
+ ctx_in.ingress_ifindex = 1;
+ ctx_in.rx_queue_index = 1;
+ err = bpf_prog_test_run_opts(prog_fd, &opts);
+ ASSERT_EQ(errno, 22, "test10-errno");
+ ASSERT_ERR(err, "bpf_prog_test_run(test10)");
Why this failure? I guess it is due to device search failure, right?
So this test MAY succeed if the underlying host happens with
a proper configuration with ingress_ifindex = 1 and rx_queue_index = 1,
right?
I may be making incorrect assumptions, but my understanding is that interface
index 1 is always the loopback interface, and the loopback interface only ever
(in current kernels) has one rx queue. If that's not the case, we'll need to
adjust (or remove) the test.
You could be correct. Please add some comments though.
+
+ test_xdp_context_test_run__destroy(skel);
+}
[...]
