Re: [PATCH net-next v3 08/10] net: ethernet: ravb: Add callback for gPTP clock index

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 1 · High: 0 · Medium: 2 · Low: 0

- [Critical] NULL pointer dereference (oops) and post-close use-after-
  free read via the new `.clock_index` callback.
- [Medium] All timestamping capabilities, including the purely software
  SOF_TIMESTAMPING_TX_SOFTWARE bit and the tx_types/rx_filters masks,…
- [Medium] On Gen2 and RZ/V2M the gPTP shutdown is now reached only
  through `.dmac_stop = ravb_ptp_stop` at the tail of `ravb_stop_dma()`…

--

net: ethernet: ravb: Add callback for gPTP clock index

This adds a .clock_index callback to struct ravb_gptp_info and uses its
return value in ravb_get_ts_info() to decide whether the driver reports
gPTP support. Gen2, Gen3 and RZ/V2M all get ravb_gen2_ptp_clock_index()
wired up.

A few questions below.

[Severity: Medium]
Is it intended that the whole capability block, and not just phc_index, is
now gated on the clock index?

RAVB has "depends on PTP_1588_CLOCK_OPTIONAL" in
drivers/net/ethernet/renesas/Kconfig, so CONFIG_PTP_1588_CLOCK=n is a
supported build. In that configuration ptp_clock_index() is the stub in
include/linux/ptp_clock_kernel.h:

  static inline int ptp_clock_index(struct ptp_clock *ptp)
  { return -1; }

so ravb_gen2_ptp_clock_index() always returns -1, "if (index >= 0)" never
fires, and ravb_get_ts_info() leaves so_timestamping, tx_types and
rx_filters all at 0.

Before this change the gate was "if (hw_info->ptp)", which advertised
SOF_TIMESTAMPING_TX_SOFTWARE together with the hardware bits and the
tx_types/rx_filters masks while leaving phc_index at -1. Commit
1f77af0aaf27 ("net: ravb: avoid dereferencing an invalid PTP clock")
described that as preserving "the static timestamping capabilities".

ravb_start_xmit() still calls skb_tx_timestamp(), and
__ethtool_get_ts_info() in net/ethtool/common.c only adds the RX and
generic software bits:

  info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE |
			   SOF_TIMESTAMPING_SOFTWARE;

so SOF_TIMESTAMPING_TX_SOFTWARE disappears from ethtool -T entirely. Should
the software capability bits and the filter masks be reported
unconditionally, with only phc_index depending on the callback?

[Severity: Critical]
Can this dereference priv->ptp.clock when no clock is registered?

With CONFIG_PTP_1588_CLOCK=y, ptp_clock_index() in drivers/ptp/ptp_clock.c
is an unconditional dereference:

  int ptp_clock_index(struct ptp_clock *ptp)
  {
	  return ptp->index;
  }

The PHC is only registered at open time (.dmac_start / .ndev_open =
ravb_ptp_init) and unregistered at close (.dmac_stop / .ndev_close =
ravb_ptp_stop), so priv->ptp.clock is NULL before the first open and stale
after close, since ravb_ptp_stop() no longer clears it:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_stop() {
	...
	if (priv->ptp.clock)
		ptp_clock_unregister(priv->ptp.clock);
}

ravb_get_ts_info() is reached from ethtool -T / ETHTOOL_MSG_TSINFO_GET, and
__ethtool_get_ts_info() in net/ethtool/common.c has no netif_running() or
CAP_NET_ADMIN gate. So a query on a never-opened interface would call
ptp_clock_index(NULL), and a query after "ip link set eth0 down" would read
memory freed by ptp_clock_unregister() and hand the result back to userspace
as phc_index. Is that reachable as written?

The "if (index >= 0)" test and the "@clock_index: Get the PTP clock index,
if any" kernel-doc suggest the callback is expected to be able to report
absence, but this implementation cannot return a negative value on
Gen2/Gen3/RZ/V2M when CONFIG_PTP_1588_CLOCK=y. The sibling helper in the
same driver family does carry the guard:

drivers/net/ethernet/renesas/rcar_gen4_ptp.c:rcar_gen4_ptp_clock_index() {
	if (!priv->initialized)
		return -1;

	return ptp_clock_index(priv->clock);
}

Note the baseline before this series cached the value instead
(info->phc_index = READ_ONCE(priv->ptp.phc_index), initialised to -1 in
ravb_probe() and reset to -1 in ravb_ptp_stop()) and cleared the pointer
with clock = xchg(&priv->ptp.clock, NULL). Those came from commit
1f77af0aaf27 ("net: ravb: avoid dereferencing an invalid PTP clock", Cc:
stable, Fixes: a0d2f20650e8) and commit 1cb9663789c5 ("net: ravb: serialize
PTP clock teardown", Cc: stable). The earlier patch in this series ("net:
ethernet: ravb: Propagate error from ptp_clock_register()") removed both,
and the later "net: ethernet: ravb: Add gPTP support for Gen4" only adds a
registration-state guard for Gen4, so Gen2/Gen3/RZ/V2M still look exposed at
the end of the series. Would a registration-state check here, plus clearing
priv->ptp.clock in ravb_ptp_stop(), restore the previous behaviour?

[Severity: Medium]
Since the clock index now depends on whether the PHC is registered, is the
Gen2/RZ/V2M teardown path via .dmac_stop always taken?

On these variants ravb_ptp_stop() is only reached from the tail of
ravb_stop_dma(), after three earlier error returns:

drivers/net/ethernet/renesas/ravb_main.c:ravb_stop_dma() {
	...
	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;
	...
	/* Stop PTP Clock driver */
	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);
	...
}

and ravb_close() continues regardless:

drivers/net/ethernet/renesas/ravb_main.c:ravb_close() {
	...
	/* Set the config mode to stop the AVB-DMAC's processes */
	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");
	...
}

If any of the TCCR / CSR(TPO*) / CSR(RPO) polls time out, the PHC stays
registered while ravb_close() goes on to free the rings, set CCC_OPC_RESET
and call pm_runtime_put_autosuspend(). Can /dev/ptpN then still drive
ravb_ptp_gettime64()/settime64()/adjfine() and issue ravb_read()/
ravb_write() against a device in reset with clocks gated?

A following "ip link set eth0 up" also runs ravb_dmac_init() ->
.dmac_start = ravb_ptp_init -> ptp_clock_register() and overwrites
priv->ptp.clock, which looks like it leaks the previously registered
ptp_clock whose ptp_clock_info points into the netdev private data.

The baseline called ravb_ptp_stop() before ravb_stop_dma() in ravb_close(),
so a DMA-stop timeout could not skip the gPTP teardown. The commit that
moved it ("net: ethernet: ravb: Simplify gPTP start and stop") states "There
are no functional change" — does that hold for this error path?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se