netdev - Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing XDP

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: <Zv18pxsiTGTZSTyO@mini-arch>
Date: Wed, 2 Oct 2024 10:02:31 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>,
	Arthur Fabre <afabre@...udflare.com>,
	Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	Jakub Sitnicki <jakub@...udflare.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
	daniel@...earbox.net, davem@...emloft.net, kuba@...nel.org,
	john.fastabend@...il.com, edumazet@...gle.com, pabeni@...hat.com,
	sdf@...ichev.me, tariqt@...dia.com, saeedm@...dia.com,
	anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
	intel-wired-lan@...ts.osuosl.org, mst@...hat.com,
	jasowang@...hat.com, mcoquelin.stm32@...il.com,
	alexandre.torgue@...s.st.com,
	kernel-team <kernel-team@...udflare.com>,
	Yan Zhai <yan@...udflare.com>
Subject: Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing
 XDP_REDIRECT

On 10/01, Toke Høiland-Jørgensen wrote:
> Lorenzo Bianconi <lorenzo@...nel.org> writes:
> 
> >> On Mon Sep 30, 2024 at 1:49 PM CEST, Lorenzo Bianconi wrote:
> >> > > Lorenzo Bianconi <lorenzo@...nel.org> writes:
> >> > > 
> >> > > >> > We could combine such a registration API with your header format, so
> >> > > >> > that the registration just becomes a way of allocating one of the keys
> >> > > >> > from 0-63 (and the registry just becomes a global copy of the header).
> >> > > >> > This would basically amount to moving the "service config file" into the
> >> > > >> > kernel, since that seems to be the only common denominator we can rely
> >> > > >> > on between BPF applications (as all attempts to write a common daemon
> >> > > >> > for BPF management have shown).
> >> > > >> 
> >> > > >> That sounds reasonable. And I guess we'd have set() check the global
> >> > > >> registry to enforce that the key has been registered beforehand?
> >> > > >> 
> >> > > >> >
> >> > > >> > -Toke
> >> > > >> 
> >> > > >> Thanks for all the feedback!
> >> > > >
> >> > > > I like this 'fast' KV approach but I guess we should really evaluate its
> >> > > > impact on performances (especially for xdp) since, based on the kfunc calls
> >> > > > order in the ebpf program, we can have one or multiple memmove/memcpy for
> >> > > > each packet, right?
> >> > > 
> >> > > Yes, with Arthur's scheme, performance will be ordering dependent. Using
> >> > > a global registry for offsets would sidestep this, but have the
> >> > > synchronisation issues we discussed up-thread. So on balance, I think
> >> > > the memmove() suggestion will probably lead to the least pain.
> >> > > 
> >> > > For the HW metadata we could sidestep this by always having a fixed
> >> > > struct for it (but using the same set/get() API with reserved keys). The
> >> > > only drawback of doing that is that we statically reserve a bit of
> >> > > space, but I'm not sure that is such a big issue in practice (at least
> >> > > not until this becomes to popular that the space starts to be contended;
> >> > > but surely 256 bytes ought to be enough for everybody, right? :)).
> >> >
> >> > I am fine with the proposed approach, but I think we need to verify what is the
> >> > impact on performances (in the worst case??)
> >> 
> >> If drivers are responsible for populating the hardware metadata before
> >> XDP, we could make sure drivers set the fields in order to avoid any
> >> memove() (and maybe even provide a helper to ensure this?).
> >
> > nope, since the current APIs introduced by Stanislav are consuming NIC
> > metadata in kfuncs (mainly for af_xdp) and, according to my understanding,
> > we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash,
> > timestamping, ..) into the packet (this is what Toke is proposing, right?).
> > In this case kfunc calling order makes a difference.
> > We can think even to add single kfunc to store all the info for all the NIC
> > metadata (maybe via a helping struct) but it seems not scalable to me and we
> > are losing kfunc versatility.
> 
> Yes, I agree we should have separate kfuncs for each metadata field.
> Which means it makes a lot of sense to just use the same setter API that
> we use for the user-registered metadata fields, but using reserved keys.
> So something like:
> 
> #define BPF_METADATA_HW_HASH      BIT(60)
> #define BPF_METADATA_HW_TIMESTAMP BIT(61)
> #define BPF_METADATA_HW_VLAN      BIT(62)
> #define BPF_METADATA_RESERVED (0xffff << 48)
> 
> bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);
> 
> 
> As for the internal representation, we can just have the kfunc do
> something like:
> 
> int bpf_packet_metadata_set(field_id, value) {
>   switch(field_id) {
>     case BPF_METADATA_HW_HASH:
>       pkt->xdp_hw_meta.hash = value;
>       break;
>     [...]
>     default:
>       /* do the key packing thing */
>   }
> }
> 
> 
> that way the order of setting the HW fields doesn't matter, only the
> user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields
here? Presumably we are talking about the use-cases where the XDP program
is doing redirect/pass and it doesn't really know who's the final
consumer is (might be another xdp program or might be the xdp->skb
kernel case), so the only sensible option here seems to be store everything?