On Tue, 21 Jul 2026 12:11:27 +0000
Mark Blasko <[email protected]> wrote:
> +xdp_meta_rx_ts_offset
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +The ``xdp_meta_rx_ts_offset`` argument specifies the byte offset of the
> 64-bit RX
> +timestamp within the XDP metadata headroom area (prepended before packet
> data).
> +
> +.. code-block:: console
> +
> + --vdev net_af_xdp,iface=ens786f1,xdp_meta_rx_ts_offset=8
> +
> +xdp_meta_valid_hint_offset
> +~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The ``xdp_meta_valid_hint_offset`` argument specifies the byte offset of the
> validity
> +flag byte within the XDP metadata headroom area.
> +
> +.. code-block:: console
> +
> + --vdev
> net_af_xdp,iface=ens786f1,xdp_meta_rx_ts_offset=8,xdp_meta_valid_hint_offset=4
> +
> +xdp_meta_rx_ts_valid_mask
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The ``xdp_meta_rx_ts_valid_mask`` argument specifies the bitmask (in hex or
> decimal) used
> +to verify timestamp validity at ``xdp_meta_valid_hint_offset``.
> +
> +.. code-block:: console
> +
> + --vdev
> net_af_xdp,iface=ens786f1,xdp_meta_rx_ts_offset=8,xdp_meta_valid_hint_offset=4,xdp_meta_rx_ts_valid_mask=0x1
> +
Awkward to have so many new metadata devargs, but I guess if XDP has no
metadata standard there
is no better way. Maybe combine into one devarg if all are required?
> + if (rxq->rx_timestamp_enabled &&
> + timestamp_dynfield_offset >= 0) {
You could simplify this if rx_timestamp_enabled was not set unless
timestamp_dynfield_offset was known.
> @@ -398,6 +415,29 @@ af_xdp_rx_zc(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
>
> rte_pktmbuf_pkt_len(bufs[i]) = len;
> rte_pktmbuf_data_len(bufs[i]) = len;
> +
> + if (rxq->rx_timestamp_enabled &&
> + timestamp_dynfield_offset >= 0) {
> + /*
> + * Copy timestamp if validity offset is not defined or
> + * flag is valid.
> + */
> + if (rxq->rx_timestamp_valid_offset < 0 ||
> + (*(uint8_t *)((char *)rte_pktmbuf_mtod(bufs[i],
> void *) -
> + rxq->rx_timestamp_valid_offset) &
Use rte_pktmbuf_mtod_offset() here and below.
More findings from detailed AI review
Review of [PATCH v3 0/2] net/af_xdp: rx timestamping and read_clock
Verified against main at 38f72e5 (26.11.0-rc0). Driver hunks apply
cleanly; the release notes hunk does not (see 1/2 Warning 6). Both
patches build with -Dwerror=true, and 1/2 builds standalone, so bisect
is fine. I also compiled the copy-mode branch by undefining
XDP_UMEM_UNALIGNED_CHUNK_FLAG -- that path is clean too.
The overall approach is sound: XDP metadata does sit immediately below
the packet data in the umem chunk in both zero-copy and copy mode, so
reading backwards from the data pointer is the right mechanism. The
problems below are about validating the offsets and about where the
PTP fd lives.
Patch 1/2: net/af_xdp: add af_xdp rx metadata and dynamic timestamping
Error 1: metadata offsets are unvalidated, giving an out-of-bounds read
in the Rx fast path.
xdp_meta_rx_ts_offset and xdp_meta_valid_hint_offset are only checked
for >= 0 (parse_integer_arg rejects negatives; probe checks the mask
and the offset/mask consistency). Nothing bounds them from above,
and both are used as raw backward pointer arithmetic:
memcpy(&ts, (char *)pkt - rxq->rx_timestamp_offset, sizeof(ts));
The window that actually exists is XDP_PACKET_HEADROOM (256) plus the
umem headroom. In copy mode xdp_umem_configure() sets
frame_headroom = 0 and fills the ring with chunk-aligned addresses
(i * ETH_AF_XDP_FRAME_SIZE), so for chunk 0 any offset above 256
reads before umem->mz->addr -- outside the memzone. In zero-copy
mode an oversized offset walks back through RTE_PKTMBUF_HEADROOM into
the rte_mbuf header and then past the chunk base.
The kernel caps metadata at 255 bytes in bpf_xdp_adjust_meta(), so a
hard upper bound is available. Reject at probe:
if (rx_timestamp_offset > XDP_PACKET_HEADROOM) ...
if (rx_timestamp_valid_offset > XDP_PACKET_HEADROOM) ...
Error 2: xdp_meta_rx_ts_offset of 0-7 silently reads packet data as a
timestamp.
The read is 8 bytes ending at (data - offset + 8), so the offset must
be at least sizeof(uint64_t) for the read to stay in the metadata
area. offset=0 is accepted today (parse_integer_arg only rejects < 0,
and eth_dev_start() only rejects < 0), and the PMD then reports the
first 8 bytes of the Ethernet header as a hardware timestamp with the
dynflag set. Require >= 8 for the timestamp offset and >= 1 for the
valid-hint offset.
Warning 3: changing parse_integer_arg() from base 10 to base 0 silently
changes six existing devargs.
parse_integer_arg() is shared by start_queue, queue_count,
shared_umem, force_copy, use_cni and use_pinned_map. Confirmed
empirically: strtol("08", &end, 10) is 8, strtol("08", &end, 0) is 0
with "8" left in end. end is assigned but never checked, so this is a
silent misparse rather than an error. Only the validity mask needs
hex; please add a separate parse function for it rather than changing
the shared one, or check *end == '\0' in the same patch.
Warning 4: rx_offload_capa advertises a capability the device may not
have.
dev_info->rx_offload_capa = RTE_ETH_RX_OFFLOAD_TIMESTAMP is
unconditional, but eth_dev_start() returns -EINVAL when
xdp_meta_rx_ts_offset was not supplied. eth_dev_configure() accepts
the offload, so the application only discovers the mismatch at start.
Advertise it only when internals->rx_timestamp_offset >= 0. Also use
|= rather than = so a later offload addition does not clobber this,
and set rx_queue_offload_capa as well -- features.rst lists both under
"Timestamp offload".
Warning 5: doc/guides/nics/features/af_xdp.ini is not updated. Add
"Timestamp offload = Y".
Warning 6: the release notes entry targets doc/guides/rel_notes/
release_26_07.rst. Main is now 26.11.0-rc0 and 26.07 is closed -- the
hunk no longer applies. Move the entry to release_26_11.rst.
Warning 7: eth_af_xdp_enable_hw_timestamping() clobbers another
application's Tx timestamping configuration.
The function reads the current config with SIOCGHWTSTAMP, then
unconditionally sets config.tx_type = HWTSTAMP_TX_OFF before
SIOCSHWTSTAMP. If ptp4l (or anything else) had Tx timestamping on
while rx_filter was NONE, this turns it off underneath them. Preserve
tx_type from the SIOCGHWTSTAMP result.
Related: the hwtstamp config is never restored on dev_stop or
dev_close, so the interface is left in HWTSTAMP_FILTER_ALL after the
DPDK application exits. Worth either restoring the saved config or
documenting the side effect explicitly.
Info 8: on socket() failure the function returns -1, and the caller logs
strerror(-rc), which prints "Operation not permitted". Return -errno.
Info 9: the new ioctl helpers use (caddr_t) casts for ifr_data. The
existing xdp_get_channels_info() in this file uses (void *). caddr_t is
a BSD-ism; please match the surrounding code.
Info 10: ops_afxdp_dp (the unprivileged AF_XDP device-plugin path)
shares eth_dev_start, and SIOCSHWTSTAMP needs CAP_NET_ADMIN. Requesting
the timestamp offload in that mode will fail the start with -EPERM.
Worth a line in af_xdp.rst.
Patch 2/2: net/af_xdp: add read_clock support to AF_XDP PMD
Error 1: ptp_fd is stored in pmd_internals, which is dev_private and
therefore shared memory, but a file descriptor is process-local.
This driver already has struct pmd_process_private for exactly this
reason -- rxq_xsk_fds[] lives there, and afxdp_mp_request_fds() /
afxdp_mp_send_fds() pass those descriptors between primary and
secondary over SCM_RIGHTS. af_xdp.ini declares "Multiprocess aware =
Y", so this is not hypothetical. Two concrete consequences:
- A secondary calling rte_eth_read_clock() derives a clockid from a
descriptor number that is only meaningful in the primary. It
either fails or, worse, reads whatever the secondary happens to
have open at that number.
- A secondary calling rte_eth_dev_stop() runs
close(internals->ptp_fd) on an unrelated descriptor in its own
table, then stores -1 into the shared field, so the primary leaks
its real PTP fd and read_clock stops working there.
Move ptp_fd into pmd_process_private, and if secondary read_clock is
meant to work, extend the existing IPC to pass it.
Warning 2: read_clock is only functional when
RTE_ETH_RX_OFFLOAD_TIMESTAMP is enabled, since the PHC is opened inside
that branch of eth_dev_start(). That couples two independent features:
an application that only wants the PHC time must also enable Rx
timestamp offload, which (per 1/2) reprograms the NIC's hwtstamp filter.
Opening the PHC unconditionally at start would decouple them.
Warning 3: features matrix -- as with 1/2, af_xdp.ini needs "Timestamp
offload = Y". features.rst lists read_clock as "[related] eth_dev_ops"
under that entry, so the one key covers both patches.
Warning 4: in af_xdp.rst the read_clock subsection is placed under
"Options", where every other ~~~ subsection is a devarg name.
read_clock is a feature, not a devarg -- it belongs in its own section.
Info 5: CLOCKFD and FD_TO_CLOCKID are defined unconditionally with no
driver prefix and no #ifndef guard. I checked the arithmetic against
the kernel's posix-timers.h definition across a range of descriptor
values and it agrees, so this is only a namespace point -- consider
AF_XDP_FD_TO_CLOCKID or an #ifndef.
