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

Hi Simon,

On 7/20/26 4:07 PM, Simon Horman wrote:

This ksz8463_set_hwtstamp_config() is largely inspired from the already 
existing ksz_set_hwtstamp_config()? Indeed if the hardware write fails, 
the ts_en flags will remain set while the hardware register isn't modified.
However, the function returns an error in that case, this error will be 
forwarded to userspace and should lead to a failure in the PTP 
initialization process.

Fixing this would imply to save the ts_en flags states at the beginning 
to be able to restore them if this hardware write fails. IMHO, it's not 
worth adding this complexity as the error is well returned and a 
hardware write failure occurring that far in the initialization is quite 
unlikely.

[ ... ]

quoted

+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.

skb_linearize() only fails if no free memory is available. This is very 
unlikely, and if it happens, reading an incorrect timestamp isn't a big 
issue IMO.

quoted

+
+	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);

[ ... ]

quoted

  	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?

I agree on this one, I guess it deserves a separated fix to net.

[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?

ksz8463_set_hwtstamp_config() refuses HWTSTAMP_FILTER_PTP_V2_L4_EVENT so 
userspace isn't supposed to send IPv4 PTP packets. If it does, it will 
indeed block the transmission worker for 100ms but that doesn't seem a 
big deal to me, is it ?

[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.

[ ... ]

I also agree here but same as above, I guess it deserves a separate fix 
to net.

quoted

  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?

I'm not sure this would happen in real use case but I can add the check 
on ts_reg to ensure that we aren't on a SYNC packet.

[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?

The 100ms timeout feels big enough to me to not fall in this issue.


Best regards,
Bastien