Re: [PATCH v7 net-next 2/3] netdevsim: Implement basic ptp support

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260720181113.3293-2-maciek@machnikowski.net?part=2
---
netdevsim: Implement basic ptp support

This change adds virtual TX/RX hardware timestamping to netdevsim using
two attached ptp_mock clocks, wires up ndo_hwtstamp_get / ndo_hwtstamp_set,
and updates ethtool ts_info to advertise HW timestamping capabilities.

[ ... ]

[Medium]
The tstamp_config fields consulted here (peer_ns->tstamp_config.rx_filter
and ns->tstamp_config.tx_type) are read from the xmit path under only
rcu_read_lock().

The writer side, nsim_set_ts_config(), runs from the ndo_hwtstamp_set
path under rtnl_lock and modifies these fields as plain stores with no
matching READ_ONCE()/WRITE_ONCE() and no shared lock or RCU protection:

	ns->tstamp_config.tx_type = HWTSTAMP_TX_ON;
	...
	ns->tstamp_config.rx_filter = HWTSTAMP_FILTER_ALL;

Is this pairing intentional?  KCSAN would flag this as a data race, and
because tx_type and rx_filter are updated in two separate unlocked
stores, a concurrent xmit can observe a new tx_type combined with the
old rx_filter (or vice versa) for a single packet.

Would READ_ONCE()/WRITE_ONCE() on these fields, or making tstamp_config
RCU-managed, be more appropriate here?

+
+	/* If TX hardware timestamping is enabled, sample our PHC and report
+	 * the TX timestamp back.
+	 */
+	if (ptp_info_tx) {
+		ptp_info_tx->gettime64(ptp_info_tx, &tx_ts);
+		if (likely(ptp_info_rx))
+			ptp_info_rx->gettime64(ptp_info_rx, &rx_ts);

[Low]
Is the likely() here justified?  ptp_info_rx being non-NULL means the
peer port has an rx_filter other than HWTSTAMP_FILTER_NONE, and
ptp_info_tx being non-NULL means the local port has tx_type ==
HWTSTAMP_TX_ON.

These are two independent user configurations on two independent
netdevsim ports; one does not imply the other, so the branch hint does
not seem to reflect an actual statistical bias.

+		shhwtstamps.hwtstamp = timespec64_to_ktime(tx_ts);
+		skb_tstamp_tx(skb_orig, &shhwtstamps);
+
+		skb = skb_copy(skb_orig, GFP_ATOMIC);
+		if (skb)
+			consume_skb(skb_orig);
+		else
+			skb = skb_orig;
+	} else if (ptp_info_rx) {
+		ptp_info_rx->gettime64(ptp_info_rx, &rx_ts);
+	}
+
+	if (ptp_info_rx)
+		skb_hwtstamps(skb)->hwtstamp = timespec64_to_ktime(rx_ts);

[High]
When skb_copy() fails under memory pressure and the code falls back to
skb = skb_orig, does the following skb_hwtstamps(skb)->hwtstamp write
corrupt the TX timestamp already reported to userspace?

skb_tstamp_tx() -> __skb_tstamp_tx() clones skb_orig via skb_clone(),
which shares the underlying skb_shared_info with the original.  In the
common non-TSONLY path __skb_tstamp_tx() writes the tx timestamp into
that shared shinfo before queueing the clone on the socket error queue:

	*skb_hwtstamps(clone) = *hwtstamps;
	__skb_complete_tx_timestamp(clone, sk, tstype, opt_stats);

If skb_copy(skb_orig, GFP_ATOMIC) then fails and skb is set back to
skb_orig, the subsequent

	skb_hwtstamps(skb)->hwtstamp = timespec64_to_ktime(rx_ts);

overwrites the same shinfo that the queued TX-timestamp clone points
at.  Userspace reading SCM_TSTAMP_SND via MSG_ERRQUEUE would then see
rx_ts instead of tx_ts.

Would it be safer to skip the shinfo write on skb_copy() failure, or to
drop the packet on that path, rather than silently corrupting the
reported TX timestamp?

Simon says: I believe the issue raised below has already been discussed.

[Low]
Relatedly, could the skb_copy(GFP_ATOMIC) approach be avoided
altogether?  The copy exists purely so the later rx_ts store lands on a
private shinfo that is not shared with the TX-report clone, but this
dependency on __skb_tstamp_tx() cloning semantics is not documented at
the call site.

Writing rx_ts into skb_hwtstamps(skb) before invoking skb_tstamp_tx()
(and passing only the local shhwtstamps to skb_tstamp_tx()) would avoid
the atomic allocation in the hot path and would also make the previous
fallback question moot.  If the copy stays, would a comment explaining
why it is necessary help future readers?

[Low, Low]
Two questions about this switch.

First, is it intentional that ns->tstamp_config.tx_type has already
been committed here before config->rx_filter has been validated?  If
the caller passes a valid tx_type paired with an unsupported rx_filter,
the function returns -ERANGE via the second switch's default, but
ns->tstamp_config.tx_type has silently been updated to the new value.
Userspace sees an error while the device is left in a partially-applied
state.

Would validating both fields up front (or using a scratch
kernel_hwtstamp_config that is copied into ns->tstamp_config only on
success) match the pattern used by most in-tree drivers?

Second, on the default: return -ERANGE, would -EOPNOTSUPP be more in
line with kernel convention here?  Drivers such as igb, i40e, and mlx5
reserve -ERANGE for values that are outside the enum's numeric range
and return -EOPNOTSUPP for values that are within range but unsupported
by the driver.  Since netdevsim is often used as a reference target for
ptp4l, hwstamp_ctl, and ethtool selftests, the errno value tends to
propagate into test expectations.

+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		ns->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
+	case HWTSTAMP_FILTER_ALL:
+		ns->tstamp_config.rx_filter = HWTSTAMP_FILTER_ALL;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	config->rx_filter = ns->tstamp_config.rx_filter;
+
+	return 0;
+}