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

[John Fastabend <john.fastabend@gmail.com>]

Toke Høiland-Jørgensen wrote:
> 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
> 

Agree, I don't see any real issue with it. Just wnated to be sure we
thought through it.

Thanks!
John