netdev - Re: [PATCH v14 bpf-next 00/18] mvneta: introduce XDP multi-buffer support

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: <87ilyu50kl.fsf@toke.dk>
Date:   Tue, 21 Sep 2021 00:44:42 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, shayagr@...zon.com,
        David Ahern <dsahern@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Saeed Mahameed <saeed@...nel.org>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        tirthendu.sarkar@...el.com
Subject: Re: [PATCH v14 bpf-next 00/18] mvneta: introduce XDP multi-buffer
 support

Jakub Kicinski <kuba@...nel.org> writes:

> On Mon, 20 Sep 2021 23:01:48 +0200 Toke Høiland-Jørgensen wrote:
>> > In fact I don't think there is anything infra can do better for
>> > flushing than the prog itself:
>> >
>> > 	bool mod = false;
>> >
>> > 	ptr = bpf_header_pointer(...);
>> > 	...
>> > 	if (some_cond(...)) {
>> > 		change_packet(...);
>> > 		mod = true;
>> > 	}
>> > 	...
>> > 	if (mod)  
>> 
>> to have an additional check like:
>> 
>> if (mod && ptr == stack)
>> 
>> (or something to that effect). No?
>
> Good point. Do you think we should have the kernel add/inline this
> optimization or have the user do it explicitly.

Hmm, good question. On the one hand it seems like an easy optimisation
to add, but on the other hand maybe the caller has other logic that can
better know how/when to omit the check.

Hmm, but the helper needs to check it anyway, doesn't it? At least it
can't just blindly memcpy() if the source and destination would be the
same...

> The draft API was:
>
> void *xdp_mb_pointer_flush(struct xdp_buff *xdp_md, u32 flags, 
>                            u32 offset, u32 len, void *stack_buf)
>
> Which does not take the ptr returned by header_pointer(), but that's
> easy to add (well, easy other than the fact it'd be the 6th arg).

I guess we could play some trickery with stuffing offset/len/flags into
one or two u64s to save an argument or two?

> BTW I drafted the API this way to cater to the case where flush()
> is called without a prior call to header_pointer(). For when packet
> trailer or header is populated directly from a map value. Dunno if
> that's actually useful, either.

Ah, didn't think of that; so then it really becomes a generic
xdp_store_bytes()-type helper? Might be useful, I suppose. Adding
headers is certainly a fairly common occurrence, but dunno to what
extent they'd be copied wholesale from a map (hadn't thought about doing
that before either).

-Toke