|SUCCESS| [RFC, v3, 6/6] examples/ptpclient: use shared PTP library definitions

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163553

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-04

I'll review this DPDK patch series introducing a PTP protocol library.

commit [git sha from patch header]
Author: Rajesh Kumar <rajesh3.kumar at intel.com>

[RFC v3 1/6] ptp: introduce PTP protocol library

This commit adds a new library for IEEE 1588-2019 Precision Time Protocol
packet processing, providing classification across L2, VLAN, UDP/IPv4, and
UDP/IPv6 transports.

> diff --git a/lib/ptp/rte_ptp.c b/lib/ptp/rte_ptp.c
> new file mode 100644
> index 0000000000..5e4fc666fb
> --- /dev/null
> +++ b/lib/ptp/rte_ptp.c

[ ... ]

> +static struct rte_ptp_hdr *
> +ptp_hdr_find(const struct rte_mbuf *m)
> +{
> +	const struct rte_ether_hdr *eth;
> +	uint16_t ether_type;
> +	uint32_t offset;
> +
> +	if (rte_pktmbuf_data_len(m) < sizeof(struct rte_ether_hdr))
> +		return NULL;

Does this code handle multi-segment mbufs correctly? The documentation
states that multi-segment support is not implemented, but this function
only checks rte_pktmbuf_data_len() which returns the length of the first
segment. If headers span segments, does ptp_hdr_find() fail silently?

> +	eth = rte_pktmbuf_mtod(m, const struct rte_ether_hdr *);
> +	ether_type = rte_be_to_cpu_16(eth->ether_type);
> +	offset = sizeof(struct rte_ether_hdr);
> +
> +	/* Strip VLAN / QinQ tags to reach the inner EtherType */
> +	if (ether_type == RTE_ETHER_TYPE_VLAN ||
> +	    ether_type == RTE_ETHER_TYPE_QINQ) {
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_vlan_hdr))
> +			return NULL;
> +		const struct rte_vlan_hdr *vlan =
> +			rte_pktmbuf_mtod_offset(m,
> +				const struct rte_vlan_hdr *, offset);

Can aligned whitespace at the start of continuation lines cause issues?
The coding style guide requires tabs for indentation and spaces for
alignment, but this continuation line appears to use only spaces.

> +		ether_type = rte_be_to_cpu_16(vlan->eth_proto);
> +		offset += sizeof(struct rte_vlan_hdr);
> +
> +		/* Second tag (QinQ inner or stacked VLAN) */
> +		if (ether_type == RTE_ETHER_TYPE_VLAN ||
> +		    ether_type == RTE_ETHER_TYPE_QINQ) {
> +			if (rte_pktmbuf_data_len(m) <
> +			    offset + sizeof(struct rte_vlan_hdr))
> +				return NULL;
> +			vlan = rte_pktmbuf_mtod_offset(m,
> +				const struct rte_vlan_hdr *, offset);
> +			ether_type = rte_be_to_cpu_16(vlan->eth_proto);
> +			offset += sizeof(struct rte_vlan_hdr);
> +		}
> +	}
> +
> +	/* L2 PTP: EtherType 0x88F7 (plain, VLAN, or QinQ) */
> +	if (ether_type == RTE_PTP_ETHERTYPE) {
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ptp_hdr))
> +			return NULL;
> +		return rte_pktmbuf_mtod_offset(m,
> +			struct rte_ptp_hdr *, offset);
> +	}
> +
> +	/* PTP over UDP/IPv4 (plain or VLAN-tagged) */
> +	if (ether_type == RTE_ETHER_TYPE_IPV4) {
> +		const struct rte_ipv4_hdr *iph;
> +		uint16_t ihl;
> +
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ipv4_hdr))
> +			return NULL;
> +
> +		iph = rte_pktmbuf_mtod_offset(m,
> +			const struct rte_ipv4_hdr *, offset);
> +		if (iph->next_proto_id != IPPROTO_UDP)
> +			return NULL;
> +
> +		ihl = (iph->version_ihl & 0x0F) * 4;
> +		if (ihl < 20)
> +			return NULL;

Does this code validate that the computed ihl value is within the packet
bounds before using it as an offset? If ihl is corrupted to a large
value, does offset arithmetic overflow?

> +		offset += ihl;

Does this addition check for overflow when offset is accumulated through
multiple layers?

> +
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_udp_hdr))
> +			return NULL;
> +
> +		const struct rte_udp_hdr *udp =
> +			rte_pktmbuf_mtod_offset(m,
> +				const struct rte_udp_hdr *, offset);
> +		uint16_t dst_port = rte_be_to_cpu_16(udp->dst_port);
> +
> +		if (dst_port != RTE_PTP_EVENT_PORT &&
> +		    dst_port != RTE_PTP_GENERAL_PORT)
> +			return NULL;
> +
> +		offset += sizeof(struct rte_udp_hdr);
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ptp_hdr))
> +			return NULL;
> +
> +		return rte_pktmbuf_mtod_offset(m,
> +			struct rte_ptp_hdr *, offset);
> +	}
> +
> +	/* PTP over UDP/IPv6 (plain or VLAN-tagged) */
> +	if (ether_type == RTE_ETHER_TYPE_IPV6) {
> +		const struct rte_ipv6_hdr *ip6h;
> +
> +		if (rte_pktmbuf_data_len(m) <
> +		    offset + sizeof(struct rte_ipv6_hdr))
> +			return NULL;
> +
> +		ip6h = rte_pktmbuf_mtod_offset(m,
> +			const struct rte_ipv6_hdr *, offset);
> +		if (ip6h->proto != IPPROTO_UDP)
> +			return NULL;

Does this code handle IPv6 extension headers? The documentation states
that extension headers are not traversed, but should the code reject
packets where proto is not IPPROTO_UDP, or should it check if proto
indicates an extension header chain that might eventually lead to UDP?

> +
> +		offset += sizeof(struct rte_ipv6_hdr);
> +
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_udp_hdr))
> +			return NULL;
> +
> +		const struct rte_udp_hdr *udp =
> +			rte_pktmbuf_mtod_offset(m,
> +				const struct rte_udp_hdr *, offset);
> +		uint16_t dst_port = rte_be_to_cpu_16(udp->dst_port);
> +
> +		if (dst_port != RTE_PTP_EVENT_PORT &&
> +		    dst_port != RTE_PTP_GENERAL_PORT)
> +			return NULL;
> +
> +		offset += sizeof(struct rte_udp_hdr);
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ptp_hdr))
> +			return NULL;
> +
> +		return rte_pktmbuf_mtod_offset(m,
> +			struct rte_ptp_hdr *, offset);
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> diff --git a/lib/ptp/rte_ptp.h b/lib/ptp/rte_ptp.h
> new file mode 100644
> index 0000000000..b13dc54d43
> --- /dev/null
> +++ b/lib/ptp/rte_ptp.h

[ ... ]

> +static inline void
> +rte_ptp_add_correction(struct rte_ptp_hdr *hdr, int64_t residence_ns)
> +{
> +	int64_t cf = (int64_t)rte_be_to_cpu_64(hdr->correction);
> +	cf += (int64_t)((uint64_t)residence_ns << 16);
> +	hdr->correction = rte_cpu_to_be_64(cf);
> +}

Does this code handle overflow when adding large residence times? If cf
is near INT64_MAX and residence_ns is large, can the addition overflow?

commit [git sha from patch header]
Author: Rajesh Kumar <rajesh3.kumar at intel.com>

[RFC v3 3/6] examples/ptp_tap_relay_sw: add software PTP relay example

> diff --git a/examples/ptp_tap_relay_sw/ptp_tap_relay_sw.c b/examples/ptp_tap_relay_sw/ptp_tap_relay_sw.c
> new file mode 100644
> index 0000000000..591007b1d5
> --- /dev/null
> +++ b/examples/ptp_tap_relay_sw/ptp_tap_relay_sw.c

[ ... ]

> +static void
> +relay_burst(uint16_t src_port, uint16_t dst_port,
> +	    uint64_t *rx_cnt, uint64_t *rx_ptp_cnt,
> +	    uint64_t *tx_cnt, uint64_t *corr_cnt)
> +{
> +	struct rte_mbuf *bufs[BURST_SIZE];
> +	struct rte_ptp_hdr *ptp_hdrs[BURST_SIZE];
> +	uint64_t rx_ts;
> +	uint16_t nb_rx, nb_tx, i;
> +
> +	nb_rx = rte_eth_rx_burst(src_port, 0, bufs, BURST_SIZE);
> +	if (nb_rx == 0)
> +		return;
> +
> +	/* Record a single RX software timestamp for the whole burst.
> +	 * All packets in one burst arrived at essentially the same instant
> +	 * from rte_eth_rx_burst()'s perspective.
> +	 */
> +	rx_ts = sw_timestamp_ns();
> +
> +	*rx_cnt += nb_rx;
> +
> +	/*
> +	 * Pass 1: Parse each packet once and remember PTP event headers.
> +	 * This avoids taking the TX timestamp too early -- we want it as
> +	 * close to the actual rte_eth_tx_burst() call as possible.
> +	 */
> +	memset(ptp_hdrs, 0, sizeof(ptp_hdrs[0]) * nb_rx);
> +	for (i = 0; i < nb_rx; i++) {
> +		struct rte_ptp_hdr *hdr = rte_ptp_hdr_get(bufs[i]);
> +
> +		if (hdr == NULL)
> +			continue;
> +
> +		(*rx_ptp_cnt)++;
> +
> +		/* Only event messages carry timestamps that need correction */
> +		if (!rte_ptp_is_event(rte_ptp_msg_type(hdr)))
> +			continue;
> +
> +		ptp_hdrs[i] = hdr;
> +	}
> +
> +	/*
> +	 * Pass 2: Take a single TX timestamp right before transmission.
> +	 * This minimises the gap between the measured tx_ts and the
> +	 * actual kernel write inside rte_eth_tx_burst(), giving the
> +	 * most accurate residence time we can achieve with SW timestamps.
> +	 *
> +	 * residence_time = tx_ts - rx_ts
> +	 *
> +	 * Remaining untracked delays:
> +	 *   - Pre-RX:  NIC DMA - rx_burst return  (~1-5 us, unavoidable)
> +	 *   - Post-TX:  tx_ts - kernel TAP write   (~1-2 us)
> +	 * Both are symmetric for Sync and Delay_Req so they largely
> +	 * cancel in the ptp4l offset calculation.
> +	 */
> +	uint64_t tx_ts = sw_timestamp_ns();
> +	int64_t residence_ns = (int64_t)(tx_ts - rx_ts);

Can this subtraction produce negative values if CLOCK_MONOTONIC wraps or
if the system clock is adjusted between the two calls?

> +
> +	for (i = 0; i < nb_rx; i++) {
> +		if (ptp_hdrs[i] == NULL)
> +			continue;
> +		rte_ptp_add_correction(ptp_hdrs[i], residence_ns);
> +		(*corr_cnt)++;
> +	}
> +
> +	/* Forward the burst */
> +	nb_tx = rte_eth_tx_burst(dst_port, 0, bufs, nb_rx);
> +	*tx_cnt += nb_tx;
> +
> +	/* Free any unsent packets */
> +	for (i = nb_tx; i < nb_rx; i++)
> +		rte_pktmbuf_free(bufs[i]);
> +}

Does this code leak packets from ptp_hdrs[] array if nb_tx is less than
nb_rx? The pointers in ptp_hdrs[] reference mbufs that might already be
freed by the loop above.