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: <Zu_gvkXe4RYjJXtq@lore-desk>
Date: Sun, 22 Sep 2024 11:17:50 +0200
From: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: Alexander Lobakin <aleksander.lobakin@...el.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Toke Høiland-Jørgensen <toke@...nel.org>,
	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,
	toke@...e.dk, 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 21/09/2024 22.17, Alexander Lobakin wrote:
> > From: Lorenzo Bianconi <lorenzo@...nel.org>
> > Date: Sat, 21 Sep 2024 18:52:56 +0200
> > 
> > > This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame
> > 
> > &xdp_buff is on the stack.
> > &xdp_frame consumes headroom.
> > 
> > IOW they're size-sensitive and putting metadata directly there might
> > play bad; if not now, then later.
> > 
> > Our idea (me + Toke) was as follows:
> > 
> > - new BPF kfunc to build generic meta. If called, the driver builds a
> >    generic meta with hash, csum etc., in the data_meta area.
> 
> I do agree that it should be the XDP prog (via a new BPF kfunc) that
> decide if xdp_frame should be updated to contain a generic meta struct.
> *BUT* I think we should use the xdp_frame area, and not the
> xdp->data_meta area.

ack, I will add a new kfunc for it.

> 
> A details is that I think this kfunc should write data directly into
> xdp_frame area, even then we are only operating on the xdp_buff, as we
> do have access to the area xdp_frame will be created in.

this would avoid to copy it when we convert from xdp_buff to xdp_frame, nice :)

> 
> 
> When using data_meta area, then netstack encap/decap needs to move the
> data_meta area (extra cycles).  The xdp_frame area (live in top) don't
> have this issue.
> 
> It is easier to allow xdp_frame area to survive longer together with the
> SKB. Today we "release" this xdp_frame area to be used by SKB for extra
> headroom (see xdp_scrub_frame).  I can imagine that we can move SKB
> fields to this area, and reduce the size of the SKB alloc. (This then
> becomes the mini-SKB we discussed a couple of years ago).
> 
> 
> >    Yes, this also consumes headroom, but only when the corresponding func
> >    is called. Introducing new fields like you're doing will consume it
> >    unconditionally;
> 
> We agree on the kfunc call marks area as consumed/in-use.  We can extend
> xdp_frame statically like Lorenzo does (with struct xdp_rx_meta), but
> xdp_frame->flags can be used for marking this area as used or not.

the only downside with respect to a TLV approach would be to consume all the
xdp_rx_meta as soon as we set a single xdp rx hw hint in it, right?
The upside is it is easier and it requires less instructions.

> 
> 
> > - when &xdp_frame gets converted to sk_buff, the function checks whether
> >    data_meta contains a generic structure filled with hints.
> > 
> 
> Agree, but take data from xdp_frame->xdp_rx_meta.
> 
> When XDP returns XDP_PASS, then I also want to see this data applied to
> the SKB. In patchset[1] Yan called this xdp_frame_fixup_skb_offloading()
> and xdp_buff_fixup_skb_offloading(). (Perhaps "fixup" isn't the right
> term, "apply" is perhaps better).  Having this generic-name allow us to
> extend with newer offloads, and eventually move members out of SKB.
> 
> We called it "fixup", because our use-case is that our XDP load-balancer
> (Unimog) XDP_TX bounce packets with in GRE header encap, and on the
> receiving NIC (due to encap) we lost the HW hash/csum, which we want to
> transfer from the original NIC, decap in XDP and apply the original HW
> hash/csum via this "fixup" call.

I already set skb metadata converting xdp_frame into a skb in
__xdp_build_skb_from_frame() but I can collect all this logic in a single
routine.

Regards,
Lorenzo

> 
> --Jesper
> 
> [1] https://lore.kernel.org/all/cover.1718919473.git.yan@cloudflare.com/
> 
> > We also thought about &skb_shared_info, but it's also size-sensitive as
> > it consumes tailroom.
> > 
> > > one as a container to store the already supported xdp rx hw hints (rx_hash
> > > and rx_vlan, rx_timestamp will be stored in skb_shared_info area) when the
> > > eBPF program running on the nic performs XDP_REDIRECT. Doing so, we are able
> > > to set the skb metadata converting the xdp_buff/xdp_frame to a skb.
> > 
> > Thanks,
> > Olek
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)