Re: [PATCH net-next v7 08/15] net: ethernet: oa_tc6: Support for hardware timestamp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Use-After-Free in TX path due to `tsc` wraparound in hardware timestamping queue.
- [High] Kernel panic due to storing and dereferencing `ERR_PTR` on PTP clock registration failure.
- [High] RX frame corruption and packet drops when hardware timestamping is disabled or timestamp is invalid.
- [Medium] Incomplete updates to standard `kernel_hwtstamp_config` structure and missing capabilities in ethtool info.
--

--- Patch [8]: [PATCH net-next v7 08/15] net: ethernet: oa_tc6: Support for hardware timestamp ---
commit 731981c874dbe9e51dcf5c38facf74ebc8f25066
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
Date:   Wed Jul 8 17:12:38 2026 +0000

    net: ethernet: oa_tc6: Support for hardware timestamp

This commit adds hardware timestamping support to the OPEN Alliance 10BASE-T1S
MAC-PHY Serial Interface framework. It implements PTP register/unregister
calls and provides ioctl and ethtool callbacks. Helper functions handle
header and footer timestamp data processing in the rx and tx paths.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index bf96e8d1ccb9a..939935cb170d6 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
[ ... ]
> @@ -48,6 +57,156 @@ static int oa_tc6_get_parity(u32 p)
>  	return !((p >> 28) & 1);
>  }
>  
[ ... ]
> +static int oa_tc6_process_deferred_skb(struct oa_tc6 *tc6, u8 tsc)
> +{
> +	struct skb_shared_hwtstamps tstamp;
> +	struct oa_tc6_ts_info_tx *ski;
> +	struct sk_buff *skb, *tmp;
> +	bool found = false;
> +	int ret = 0;
> +
> +	/* Size of data must match OA_TC6_TSTAMP_SZ */
> +	u32 data[2];
> +
> +	list_for_each_entry_safe(skb, tmp, &tc6->tx_ts_skb_q, list) {
> +		ski = oa_tc6_tsinfo_tx(skb);
> +		if (ski->tsc != tsc)
> +			continue;
> +		if (found) {
> +			dev_warn_ratelimited(&tc6->spi->dev,
> +					     "Multiple skbs. tsc = %d\n",
> +					     tsc);
> +			tc6->tx_hwtstamp_err++;
> +		}
> +		found = true;
> +		list_del(&skb->list);
> +
> +		/* Retrieve the timestamping info */
> +		ret = oa_tc6_read_registers(tc6,
> +					    OA_TC6_REG_TTSCA_HIGH +
> +					    2 * (tsc - 1), &data[0], 2);
> +
> +		if (!ret) {
> +			tstamp.hwtstamp = ktime_set(data[0], data[1]);
> +			skb_tstamp_tx(skb, &tstamp);
> +			tc6->tx_hwtstamp_pkts++;
> +		}
> +
> +		dev_kfree_skb(skb);
> +	}

[Severity: High]
Can this loop free an SKB that is still actively being processed?

Looking at oa_tc6_defer_for_hwtstamp(), the hardware only provides 3
timestamp tracking IDs (tsc), and tx_ts_idx wraps around. If more than 3
packets are transmitted before the MAC-PHY generates interrupts, a newly
queued SKB could be forced to reuse the same tsc as an older waiting SKB.

When the interrupt fires, this loop doesn't stop after finding one match.
It will unconditionally free all SKBs matching the tsc. Could this free the
newly queued SKB while it is still referenced by tc6->ongoing_tx_skb and
actively being copied in oa_tc6_add_tx_skb_to_spi_buf(), triggering a
use-after-free?

> +	return ret;
> +}
[ ... ]
> +static void oa_tc6_update_ts_in_rx_skb(struct oa_tc6 *tc6)
> +{
> +	struct sk_buff *skb = tc6->rx_skb;
> +	struct oa_tc6_ts_info_rx *ski;
> +	__be32 ts_val[2];
> +	u32 ts[2];
> +
> +	if (!tc6->hw_tstamp_enabled)
> +		return;
> +	ski = oa_tc6_tsinfo_rx(skb);
> +	if (!ski->rtsa)
> +		return;
> +
> +	memcpy(&ts_val[0], skb->data, 4);
> +	memcpy(&ts_val[1], (u32 *)skb->data + 1, 4);
> +
> +	ts[0] = be32_to_cpu(ts_val[0]);
> +	ts[1] = be32_to_cpu(ts_val[1]);
> +
> +	/* Check parity */
> +	if ((oa_tc6_get_parity(ts[0]) ^ oa_tc6_get_parity(ts[1])) ==
> +	    !ski->rtsp) {
> +		struct skb_shared_hwtstamps *hw_ts;
> +
> +		/* Report timestamp to the upper layers */
> +		hw_ts = skb_hwtstamps(skb);
> +		memset(hw_ts, 0, sizeof(*hw_ts));
> +		hw_ts->hwtstamp = ktime_set(ts[0], ts[1]);
> +	}
> +	skb_pull(skb, sizeof(ts));
> +}

[Severity: High]
Will this cause frames to be corrupted if hardware timestamping is disabled?

When the MAC-PHY prepends the 8-byte timestamp to every frame, it appears
this function is responsible for removing it via skb_pull(). If
tc6->hw_tstamp_enabled is false, or if ski->rtsa is false, the function
returns early without pulling those 8 bytes. Does this leave 8 bytes of
garbage at the start of the payload that the network stack will misinterpret
as the Ethernet MAC header?

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
> new file mode 100644
> index 0000000000000..0cde5545ee27a
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
[ ... ]
> +int oa_tc6_ptp_register(struct oa_tc6 *tc6, struct ptp_clock_info *info)
> +{
> +	/* Not supporting hardware timestamp isn't an error */
> +	if (!tc6->hw_tstamp_supported)
> +		return 0;
> +
> +	snprintf(info->name, sizeof(info->name), "%s",
> +		 "OA TC6 PTP clock");
> +	tc6->ptp_clock = ptp_clock_register(info, &tc6->spi->dev);
> +	if (IS_ERR(tc6->ptp_clock)) {
> +		dev_err(&tc6->spi->dev, "Registration of %s failed",
> +			info->name);
> +		return -EFAULT;
> +	}

[Severity: High]
Should tc6->ptp_clock be set to NULL on failure?

If ptp_clock_register() fails, it returns an ERR_PTR which is stored
directly into tc6->ptp_clock. Later, functions like oa_tc6_ptp_unregister()
or oa_tc6_get_ts_info() check if (tc6->ptp_clock) before using it. Since
an ERR_PTR is not NULL, they will attempt to dereference the error pointer,
which can cause a kernel panic.

> +	dev_info(&tc6->spi->dev, "%s registered. index %d", info->name,
> +		 ptp_clock_index(tc6->ptp_clock));
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
> new file mode 100644
> index 0000000000000..f80f1a4916a3e
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
[ ... ]
> +int oa_tc6_hwtstamp_set(struct oa_tc6 *tc6,
> +			struct kernel_hwtstamp_config *cfg)
> +{
[ ... ]
> +	hwtstamp_config_from_kernel(&tc6->ts_config, cfg);
> +
> +	/* Supports timestamping all traffic */
> +	if (cfg->rx_filter != HWTSTAMP_FILTER_NONE)
> +		tc6->ts_config.rx_filter = HWTSTAMP_FILTER_ALL;
> +	return oa_tc6_set_hwtstamp_settings(tc6);
> +}

[Severity: Medium]
Should this updated filter be copied back to the user configuration?

The standard network API expects the finalized configuration to be written
back to the caller-provided structure. If the filter is broadened to
HWTSTAMP_FILTER_ALL, does cfg->rx_filter need to be updated so that
the caller is aware of the applied state?

[ ... ]
> +int oa_tc6_get_ts_info(struct oa_tc6 *tc6,
> +		       struct kernel_ethtool_ts_info *info)
> +{
> +	if (!tc6->ptp_clock)
> +		return ethtool_op_get_ts_info(tc6->netdev, info);
> +
> +	info->so_timestamping = SOF_TIMESTAMPING_RAW_HARDWARE |
> +				SOF_TIMESTAMPING_TX_HARDWARE |
> +				SOF_TIMESTAMPING_RX_HARDWARE;
> +	info->phc_index = ptp_clock_index(tc6->ptp_clock);
> +	info->tx_types = BIT(HWTSTAMP_TX_ON);
> +	info->rx_filters = BIT(HWTSTAMP_FILTER_ALL);
> +	return 0;
> +}

[Severity: Medium]
Are we omitting the supported OFF and NONE capabilities here?

The info structure is unconditionally hardcoded to only the ON and ALL bits.
Since the interface supports having timestamping disabled, should
BIT(HWTSTAMP_TX_OFF) and BIT(HWTSTAMP_FILTER_NONE) be included to accurately
report the interface's capabilities to userspace tools like ethtool?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-s2500-mac-phy-support-v7-0-478c877aa1a9@onsemi.com?part=8