Re: [PATCH v3 1/2] net/af_xdp: add af_xdp rx metadata and dynamic timestamping support

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 21 Jul 2026 12:11:27 +0000
Mark Blasko <blasko@google.com> 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.