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: <87r1alwwk4.fsf@toke.dk>
Date:   Thu, 09 Dec 2021 17:51:55 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     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 <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?

-Toke