Re: [PATCH net-next v2 10/10] net: dsa: microchip: add two-steps PTP support for KSZ8463

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260713-ksz-new-ptp-v2-1-7209c05058ef@bootlin.com?part=10
---
--- Patch [10]: [PATCH net-next v2 10/10] net: dsa: microchip: add two-steps PTP support for KSZ8463 ---
commit 36964d81014567278bc46ae45085179f4779f40a
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
Subject: net: dsa: microchip: add two-steps PTP support for KSZ8463

This patch adds L2 two-step PTP support for the KSZ8463 switch. It
implements the get_ts_info and port_hwtstamp_set callbacks and tracks
the last transmitted event type to read the correct timestamp register.

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index be6b8240ac03d..1cd2940c7befa 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
[ ... ]
> +static int ksz8463_set_hwtstamp_config(struct ksz_device *dev,
> +				       struct ksz_port *prt,
> +				       struct kernel_hwtstamp_config *config)
> +{
> +	const u16 *regs = dev->info->regs;
> +	int ret;
> +
> +	if (config->flags)
> +		return -EINVAL;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_OFF:
> +		prt->ptpmsg_irq[KSZ8463_SYNC_MSG].ts_en  = false;
> +		prt->ptpmsg_irq[KSZ8463_XDREQ_PDRES_MSG].ts_en = false;
> +		prt->hwts_tx_en = false;
> +		break;
> +	case HWTSTAMP_TX_ON:
> +		prt->ptpmsg_irq[KSZ8463_SYNC_MSG].ts_en  = true;
> +		prt->ptpmsg_irq[KSZ8463_XDREQ_PDRES_MSG].ts_en = true;
> +		prt->hwts_tx_en = true;
> +
> +		ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_1STEP, 0);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If the hardware register configuration fails, does the port software state
remain enabled?

In ksz8463_set_hwtstamp_config(), prt->hwts_tx_en and the ts_en flag are
set to true before attempting to write to the hardware register. If the
hardware write fails, the function returns an error without reverting the
software state. Could this lead to subsequent PTP packets being intercepted
but never hardware-timestamped?

[ ... ]
> +static void ksz8463_set_pdelayresp_flag(struct ksz_port *prt,
> +					struct sk_buff *skb)
> +{
> +	struct ptp_header *hdr;
> +	unsigned int type;
> +	u8 ptp_msg_type;
> +
> +	if (!ksz_is_ksz8463(prt->ksz_dev))
> +		return;
> +
> +	if (skb_linearize(skb))
> +		return;

[Severity: Medium]
If skb_linearize() fails, it returns early without updating
prt->last_tx_is_pdelayresp. Will the caller ignore this failure and
proceed to transmit the packet anyway?

When the hardware interrupt fires, ksz_read_ts() could use the stale flag
from a previous packet to calculate the register offset, resulting in
reading an incorrect timestamp.

> +
> +	type = ptp_classify_raw(skb);
> +	if (type == PTP_CLASS_NONE)
> +		return;
> +
> +	hdr = ptp_parse_header(skb, type);
> +	if (!hdr)
> +		return;
> +
> +	ptp_msg_type = ptp_get_msgtype(hdr, type);
> +	prt->last_tx_is_pdelayresp = (ptp_msg_type == PTP_MSGTYPE_PDELAY_RESP);
> +}
> +
>  void ksz_port_deferred_xmit(struct kthread_work *work)
>  {
>  	struct ksz_deferred_xmit_work *xmit_work = work_to_xmit_work(work);
[ ... ]
>  	reinit_completion(&prt->tstamp_msg_comp);
>  
> +	ksz8463_set_pdelayresp_flag(prt, skb);
> +
>  	dsa_enqueue_skb(skb, skb->dev);
>  
>  	ksz_ptp_txtstamp_skb(dev, prt, clone);

[Severity: High]
This is a pre-existing issue, but does ksz_ptp_txtstamp_skb() leak the
cloned SKB if the wait for the TX timestamp times out?

In ksz_ptp_txtstamp_skb(), the driver blocks waiting for the hardware TX
timestamp interrupt. If the timeout expires, the function returns early
without freeing the clone:

static void ksz_ptp_txtstamp_skb(...) {
    ...
    ret = wait_for_completion_timeout(&prt->tstamp_msg_comp, ...);
    if (!ret)
        return;
}

The caller ksz_port_deferred_xmit() then frees the work item, leaving the
cloned SKB permanently leaked. Could this lead to a system memory leak?

[Severity: High]
Does the shared TX path intercept unsupported network layers for the
KSZ8463?

The commit message notes that IPv4 and IPv6 layers aren't supported by the
hardware. However, the shared ksz_port_txtstamp() relies on
ptp_classify_raw() to intercept packets, which does not filter out
IPv4/IPv6 PTP packets.

If a user sends an IPv4/IPv6 PTP packet, does the driver intercept and queue
it for transmission, only to needlessly block the transmission worker for
100ms because the hardware ignores it and no interrupt will fire?

[Severity: High]
This is a pre-existing issue, but can dereferencing the PTP header without
verifying linearity cause page faults?

In the shared ksz_port_txtstamp() path, ptp_parse_header() is called to
retrieve the PTP header, which only verifies that the header fits within
the total SKB length.

Dereferencing this pointer in ptp_get_msgtype() without first calling
skb_linearize() or pskb_may_pull() could result in reading past the
linear allocation if the packet is non-linear.

[ ... ]
>  static int ksz_read_ts(struct ksz_port *port, u16 reg, u32 *ts)
>  {
> -	return ksz_read32(port->ksz_dev, reg, ts);
> +	u16 ts_reg = reg;
> +
> +	/**
> +	 * On KSZ8463 DREQ and DRESP timestamps share one interrupt line
> +	 * so we have to check the nature of the latest event sent to know
> +	 * where the timestamp is located
> +	 */
> +	if (ksz_is_ksz8463(port->ksz_dev) && port->last_tx_is_pdelayresp)
> +		ts_reg += KSZ8463_DRESP_TS_OFFSET;
> +
> +	return ksz_read32(port->ksz_dev, ts_reg, ts);
>  }

[Severity: High]
Is KSZ8463_DRESP_TS_OFFSET erroneously added to the SYNC_TS register
address here?

The ksz_read_ts() function is used for both the DREQ interrupt and the SYNC
interrupt. If a SYNC_MSG interrupt is processed while
port->last_tx_is_pdelayresp happens to be true, the offset appears to be
unconditionally added to the ts_reg. Does this cause the driver to read
from an invalid hardware register offset?

[Severity: High]
This is a pre-existing issue, but can late TX timestamp interrupts
mistakenly complete the wait for a subsequent, unrelated packet?

If a packet transmission times out, the worker thread moves on to queue the
next packet and reinitializes the completion. In ksz_ptp_msg_thread_fn(),
the interrupt handler calls complete() without verifying sequence IDs:

if (ptpmsg_irq->ts_en) {
    ...
    complete(&port->tstamp_msg_comp);
}

If a delayed interrupt from the first packet arrives, could it falsely
wake up the worker waiting for the second packet, improperly assigning
the first packet's timestamp to the second?