| 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
| ||
|
Message-ID: <b3c05e8d-bb93-0287-2a96-e8d7f690be83@intel.com>
Date: Thu, 19 Aug 2021 15:32:55 -0400
From: Kishen Maloor <kishen.maloor@...el.com>
To: Jesper Dangaard Brouer <jbrouer@...hat.com>, <bpf@...r.kernel.org>,
<netdev@...r.kernel.org>, <hawk@...nel.org>,
<magnus.karlsson@...el.com>,
Björn Töpel <bjorn@...nel.org>,
Andrii Nakryiko <andrii@...nel.org>,
Jithu Joseph <jithu.joseph@...el.com>
CC: <brouer@...hat.com>
Subject: Re: [RFC bpf-next 2/5] libbpf: SO_TXTIME support in AF_XDP
On 8/18/21 5:49 AM, Jesper Dangaard Brouer wrote:
>
>
> On 03/08/2021 19.10, Kishen Maloor wrote:
>> This change adds userspace support for SO_TXTIME in AF_XDP
>> to include a specific TXTIME (aka "Launch Time")
>> with XDP frames issued from userspace XDP applications.
>>
>> The userspace API has been expanded with two helper functons:
>>
>> - int xsk_socket__enable_so_txtime(struct xsk_socket *xsk, bool enable)
>> Sets the SO_TXTIME option on the AF_XDP socket (using setsockopt()).
>>
>> - void xsk_umem__set_md_txtime(void *umem_area, __u64 chunkAddr,
>> __s64 txtime)
>> Packages the application supplied TXTIME into struct xdp_user_tx_metadata:
>> struct xdp_user_tx_metadata {
>
> Struct name is important and becomes UAPI. I'm not 100% convinced this is a good name.
Sure, we can choose a better name. Open to suggestions.
>
> For BPF programs libbpf can at load-time lookup the 'btf_id' via:
>
> btf_id = bpf_core_type_id_kernel(struct xdp_user_tx_metadata);
>
> Example see[1]
> [1] https://github.com/xdp-project/bpf-examples/commit/2390b4b11079
>
> I know this is AF_XDP userspace, but I hope Andrii can help guide us howto expose the bpf_core_type_id_kernel() API via libbpf, to be used by the AF_XDP userspace program.
>
>
>> __u64 timestamp;
>> __u32 md_valid;
>> __u32 btf_id;
>> };
>
> I assume this struct is intended to be BTF "described".
Yes, the intention here was to be future-proof in a sense, as once all the related infrastructure/tooling for BTF comes into existence, it should hopefully be a
simple matter to adapt this code to work with that.
>
> Struct member *names* are very important for BTF. (E.g. see how 'spinlock' have special meaning and is matched internally by kernel).
>
> The member name 'timestamp' seems too generic. This is a very specific 'LaunchTime' feature, which could be reflected in the name.
Yes, LaunchTime is the specific use case addressed by this RFC, so open to suggestions on the struct member name (e.g. launch_time?).
>
> Later it looks like you are encoding the "type" in md_valid, which I guess it is needed as timestamps can have different "types".
No, the purpose of md_valid is to flag all those md fields that have been set/conveyed by this metadata. So, XDP_METADATA_USER_TX_TIMESTAMP is
meant as a reference to the 'timestamp' field in struct xdp_user_tx_metadata.
> E.g. some of the clockid_t types from clock_gettime(2):
> CLOCK_REALTIME
> CLOCK_TAI
> CLOCK_MONOTONIC
> CLOCK_BOOTTIME
>
> Which of these timestamp does XDP_METADATA_USER_TX_TIMESTAMP represent?
> Or what timestamp type is the expected one?
CLOCK_TAI is what we use for LaunchTime, and supported by IGC/i225.
>
> In principle we could name the member 'Launch_Time_CLOCK_TAI' to encoded the clockid_t type in the name, but I think that would be too much (and require too advanced BTF helpers to extract type, having a clock_type member is easier to understand/consume from C).
>
>
>> and stores it in the XDP metadata area, which precedes the XDP frame.
>>
>> Signed-off-by: Kishen Maloor <kishen.maloor@...el.com>
>> ---
>> tools/include/uapi/linux/if_xdp.h | 2 ++
>> tools/include/uapi/linux/xdp_md_std.h | 14 ++++++++++++++
>> tools/lib/bpf/xsk.h | 27 ++++++++++++++++++++++++++-
>> 3 files changed, 42 insertions(+), 1 deletion(-)
>> create mode 100644 tools/include/uapi/linux/xdp_md_std.h
>>
>> diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h
>> index a78a8096f4ce..31f81f82ed86 100644
>> --- a/tools/include/uapi/linux/if_xdp.h
>> +++ b/tools/include/uapi/linux/if_xdp.h
>> @@ -106,6 +106,8 @@ struct xdp_desc {
>> __u32 options;
>> };
>> +#define XDP_DESC_OPTION_METADATA (1 << 0)
>> +
>> /* UMEM descriptor is __u64 */
>> #endif /* _LINUX_IF_XDP_H */
>> diff --git a/tools/include/uapi/linux/xdp_md_std.h b/tools/include/uapi/linux/xdp_md_std.h
>> new file mode 100644
>> index 000000000000..f00996a61639
>> --- /dev/null
>> +++ b/tools/include/uapi/linux/xdp_md_std.h
>> @@ -0,0 +1,14 @@
>> +#ifndef _UAPI_LINUX_XDP_MD_STD_H
>> +#define _UAPI_LINUX_XDP_MD_STD_H
>> +
>> +#include <linux/types.h>
>> +
>> +#define XDP_METADATA_USER_TX_TIMESTAMP 0x1
>> +
>> +struct xdp_user_tx_metadata {
>> + __u64 timestamp;
>> + __u32 md_valid;
>> + __u32 btf_id;
>> +};
>> +
>> +#endif /* _UAPI_LINUX_XDP_MD_STD_H */
>> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
>> index 01c12dca9c10..1b52ffe1c9a3 100644
>> --- a/tools/lib/bpf/xsk.h
>> +++ b/tools/lib/bpf/xsk.h
>> @@ -16,7 +16,8 @@
>> #include <stdint.h>
>> #include <stdbool.h>
>> #include <linux/if_xdp.h>
>> -
>> +#include <linux/xdp_md_std.h>
>> +#include <errno.h>
>> #include "libbpf.h"
>> #ifdef __cplusplus
>> @@ -248,6 +249,30 @@ static inline __u64 xsk_umem__add_offset_to_addr(__u64 addr)
>> LIBBPF_API int xsk_umem__fd(const struct xsk_umem *umem);
>> LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk);
>> +/* Helpers for SO_TXTIME */
>> +
>> +static inline void xsk_umem__set_md_txtime(void *umem_area, __u64 addr, __s64 txtime)
>> +{
>> + struct xdp_user_tx_metadata *md;
>> +
>> + md = (struct xdp_user_tx_metadata *)&((char *)umem_area)[addr];
>> +
>> + md->timestamp = txtime;
>> + md->md_valid |= XDP_METADATA_USER_TX_TIMESTAMP;
>
> Is this encoding the "type" of the timestamp?
No, it is hinting at the field (timestamp) being conveyed in this metadata.
>
> I don't see the btf_id being updated. Does that happen in another patch?
No. As I understand, BTF support and it's overall applicability (particularly from AF_XDP userspace) is still a WIP. So, btf_id currently exists as a
placeholder to facilitate future BTF integration.
Meanwhile, this RFC re-purposes SO_TXTIME on the control path from AF_XDP userspace to exercise the LaunchTime feature in the presented pattern.
>
> As I note above we are current;y lacking an libbpf equivalent bpf_core_type_id_kernel() lookup function in userspace.
>
>> +}
>> +
>> +static inline int xsk_socket__enable_so_txtime(struct xsk_socket *xsk, bool enable)
>> +{
>> + unsigned int val = (enable) ? 1 : 0;
>> + int err;
>> +
>> + err = setsockopt(xsk_socket__fd(xsk), SOL_XDP, SO_TXTIME, &val, sizeof(val));
>> +
>> + if (err)
>> + return -errno;
>> + return 0;
>> +}
>> +
>> #define XSK_RING_CONS__DEFAULT_NUM_DESCS 2048
>> #define XSK_RING_PROD__DEFAULT_NUM_DESCS 2048
>> #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */
>>
>
Powered by blists - more mailing lists