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

[Yonghong Song <yhs@fb.com>]

On 6/9/21 10:07 AM, Zvi Effron wrote:
> On Sat, Jun 5, 2021 at 11:19 PM Yonghong Song <yhs@fb.com> 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);
>>> +}
[...]