netdev - RE: [PATCH bpf-next 6/8] bpf: Add XDP_REDIRECT support to XDP for bpf_prog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61b25147bc136_6bfb208c5@john.notmuch>
Date:   Thu, 09 Dec 2021 10:56:07 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: RE: [PATCH bpf-next 6/8] bpf: Add XDP_REDIRECT support to XDP for
 bpf_prog_run()

Toke Høiland-Jørgensen wrote:
> Toke Høiland-Jørgensen <toke@...hat.com> writes:
> 
> > John Fastabend <john.fastabend@...il.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@...hat.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?
> 
> Another thought is that we could of course do the opposite here: instead
> of updating the xdp_frame when the program resizes the packet, just
> reset the pointers so that the next invocation will get the original
> size again? The data would still be changed, but maybe that behaviour is
> less surprising? WDYT?

Should read my email from newest to oldest :)

I think resetting it back to the original size is less surprising. And
if I want to benchmark a helper that moves the pointers it will be
easier. For example benchmarking shrinking a packet with current
code wouldn't really work because eventually the packet will be 0
and my test will stop doing what I expect.

Lets do the reset back to original size.

Thanks,
John

> 
> -Toke
>