RE: [PATCH bpf-next 6/8] bpf: Add XDP_REDIRECT support to XDP for bpf_prog_run()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

John Fastabend <john.fastabend@gmail.com> writes:

> Toke Høiland-Jørgensen wrote:
>> This adds support for doing real redirects when an XDP program returns
>> XDP_REDIRECT in bpf_prog_run(). To achieve this, we create a page pool
>> instance while setting up the test run, and feed pages from that into the
>> XDP program. The setup cost of this is amortised over the number of
>> repetitions specified by userspace.
>> 
>> To support performance testing use case, we further optimise the setup step
>> so that all pages in the pool are pre-initialised with the packet data, and
>> pre-computed context and xdp_frame objects stored at the start of each
>> page. This makes it possible to entirely avoid touching the page content on
>> each XDP program invocation, and enables sending up to 11.5 Mpps/core on my
>> test box.
>> 
>> Because the data pages are recycled by the page pool, and the test runner
>> doesn't re-initialise them for each run, subsequent invocations of the XDP
>> program will see the packet data in the state it was after the last time it
>> ran on that particular page. This means that an XDP program that modifies
>> the packet before redirecting it has to be careful about which assumptions
>> it makes about the packet content, but that is only an issue for the most
>> naively written programs.
>> 
>> Previous uses of bpf_prog_run() for XDP returned the modified packet data
>> and return code to userspace, which is a different semantic then this new
>> redirect mode. For this reason, the caller has to set the new
>> BPF_F_TEST_XDP_DO_REDIRECT flag when calling bpf_prog_run() to opt in to
>> the different semantics. Enabling this flag is only allowed if not setting
>> ctx_out and data_out in the test specification, since it means frames will
>> be redirected somewhere else, so they can't be returned.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> [...]
>
>> +static int bpf_test_run_xdp_redirect(struct bpf_test_timer *t,
>> +				     struct bpf_prog *prog, struct xdp_buff *orig_ctx)
>> +{
>> +	void *data, *data_end, *data_meta;
>> +	struct xdp_frame *frm;
>> +	struct xdp_buff *ctx;
>> +	struct page *page;
>> +	int ret, err = 0;
>> +
>> +	page = page_pool_dev_alloc_pages(t->xdp.pp);
>> +	if (!page)
>> +		return -ENOMEM;
>> +
>> +	ctx = ctx_from_page(page);
>> +	data = ctx->data;
>> +	data_meta = ctx->data_meta;
>> +	data_end = ctx->data_end;
>> +
>> +	ret = bpf_prog_run_xdp(prog, ctx);
>> +	if (ret == XDP_REDIRECT) {
>> +		frm = (struct xdp_frame *)(ctx + 1);
>> +		/* if program changed pkt bounds we need to update the xdp_frame */
>
> Because this reuses the frame repeatedly is there any issue with also
> updating the ctx each time? Perhaps if the prog keeps shrinking
> the pkt it might wind up with 0 len pkt? Just wanted to ask.

Sure, it could. But the data buffer comes from userspace anyway, and
there's nothing preventing userspace from passing a 0-length packet
anyway, so I just mentally put this in the "don't do that, then" bucket :)

At least I don't *think* there's actually any problem with this that we
don't have already? A regular XDP program can also shrink an incoming
packet to zero, then redirect it, no?

-Toke