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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 11 Dec 2022 12:09:28 +0100 From: Jesper Dangaard Brouer <jbrouer@...hat.com> To: Stanislav Fomichev <sdf@...gle.com>, Jesper Dangaard Brouer <jbrouer@...hat.com> Cc: brouer@...hat.com, bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com, jolsa@...nel.org, David Ahern <dsahern@...il.com>, Jakub Kicinski <kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>, Anatoly Burakov <anatoly.burakov@...el.com>, Alexander Lobakin <alexandr.lobakin@...el.com>, Magnus Karlsson <magnus.karlsson@...il.com>, Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net, netdev@...r.kernel.org Subject: Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs On 09/12/2022 18.47, Stanislav Fomichev wrote: > On Fri, Dec 9, 2022 at 3:11 AM Jesper Dangaard Brouer > <jbrouer@...hat.com> wrote: >> >> >> On 06/12/2022 03.45, Stanislav Fomichev wrote: >>> There is an ndo handler per kfunc, the verifier replaces a call to the >>> generic kfunc with a call to the per-device one. >>> >>> For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which >>> implements all possible metatada kfuncs. Not all devices have to >>> implement them. If kfunc is not supported by the target device, >>> the default implementation is called instead. >>> >>> Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags, >>> we treat prog_index as target device for kfunc resolution. >>> >> >> [...cut...] >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>> index 5aa35c58c342..2eabb9157767 100644 >>> --- a/include/linux/netdevice.h >>> +++ b/include/linux/netdevice.h >>> @@ -74,6 +74,7 @@ struct udp_tunnel_nic_info; >>> struct udp_tunnel_nic; >>> struct bpf_prog; >>> struct xdp_buff; >>> +struct xdp_md; >>> >>> void synchronize_net(void); >>> void netdev_set_default_ethtool_ops(struct net_device *dev, >>> @@ -1611,6 +1612,10 @@ struct net_device_ops { >>> ktime_t (*ndo_get_tstamp)(struct net_device *dev, >>> const struct skb_shared_hwtstamps *hwtstamps, >>> bool cycles); >>> + bool (*ndo_xdp_rx_timestamp_supported)(const struct xdp_md *ctx); >>> + u64 (*ndo_xdp_rx_timestamp)(const struct xdp_md *ctx); >>> + bool (*ndo_xdp_rx_hash_supported)(const struct xdp_md *ctx); >>> + u32 (*ndo_xdp_rx_hash)(const struct xdp_md *ctx); >>> }; >>> >> >> Would it make sense to add a 'flags' parameter to ndo_xdp_rx_timestamp >> and ndo_xdp_rx_hash ? >> >> E.g. we could have a "STORE" flag that asks the kernel to store this >> information for later. This will be helpful for both the SKB and >> redirect use-cases. >> For redirect e.g into a veth, then BPF-prog can use the same function >> bpf_xdp_metadata_rx_hash() to receive the RX-hash, as it can obtain the >> "stored" value (from the BPF-prog that did the redirect). >> >> (p.s. Hopefully a const 'flags' variable can be optimized when unrolling >> to eliminate store instructions when flags==0) > > Are we concerned that doing this without a flag and with another > function call will be expensive? Yes, but if we can unroll (to avoid the function calls) it would be more flexible and explicit API with below instead. > For xdp->skb path, I was hoping we would be to do something like: > > timestamp = bpf_xdp_metadata_rx_hash(ctx); > bpf_xdp_metadata_export_rx_hash_to_skb(ctx, timestamp); > > This should also let the users adjust the metadata before storing it. > Am I missing something here? Why would the flag be preferable? I do like this ability to let the users adjust the metadata before storing it. This would be a more flexible API for the BPF-programmer. I like your "export" suggestion. The main concern for me was performance overhead of the extra function call, which I guess can be removed via unrolling later. Unrolling these 'export' functions might be easier to accept from a maintainer perspective, as it is not device driver specific, thus we can place that in the core BPF code. --Jesper
Powered by blists - more mailing lists