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

[netdev-bot+sashiko@kernel.org]

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.

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index b788e8f5260dd..27e2b31955ba9 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1042,6 +1042,7 @@ struct ravb_ptp {
>   * critical points in the RAVB driver.
>   *
>   * @probe:		Probe the gPTP clock
> + * @clock_index:	Get the PTP clock index, if any
>   * @set_config_mode:	Enter config mode
>   * @dmac_start:		Called when the DMAC starts
>   * @dmac_stop:		Called when the DMAC stops
> @@ -1050,6 +1051,7 @@ struct ravb_ptp {
>   */
>  struct ravb_gptp_info {
>  	int (*probe)(struct net_device *ndev);
> +	int (*clock_index)(struct net_device *ndev);
>  	int (*set_config_mode)(struct net_device *ndev);
>  	int (*dmac_start)(struct net_device *ndev);
>  	void (*dmac_stop)(struct net_device *ndev);

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7e53eca5327f5..402370e1be634 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1773,8 +1773,13 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *hw_info = priv->info;
> +	int index = -1;
>  
> -	if (hw_info->ptp) {
> +	if (hw_info->ptp && hw_info->ptp->clock_index)
> +		index = hw_info->ptp->clock_index(ndev);
> +
> +	/* Only advertise ptp clock if present. */
> +	if (index >= 0) {
>  		info->so_timestamping =
>  			SOF_TIMESTAMPING_TX_SOFTWARE |

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

>  			SOF_TIMESTAMPING_TX_HARDWARE |
> @@ -1785,7 +1790,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = ptp_clock_index(priv->ptp.clock);
> +		info->phc_index = index;
>  	}
>  
>  	return 0;
> @@ -2653,6 +2658,13 @@ static int ravb_gen2_ptp_probe(struct net_device *ndev)
>  	return ravb_compute_gti(ndev, priv->clk);
>  }
>  
> +static int ravb_gen2_ptp_clock_index(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	return ptp_clock_index(priv->ptp.clock);
> +}
> +

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

>  static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
>  {
>  	int ret;
> @@ -2669,6 +2681,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
>  
>  static const struct ravb_gptp_info ravb_gen2_ptp_info = {
>  	.probe = ravb_gen2_ptp_probe,
> +	.clock_index = ravb_gen2_ptp_clock_index,
>  	.set_config_mode = ravb_gen2_ptp_set_config_mode,
>  	.dmac_start = ravb_ptp_init,
>  	.dmac_stop = ravb_ptp_stop,

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

> @@ -2706,6 +2719,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
>  
>  static const struct ravb_gptp_info ravb_gen3_ptp_info = {
>  	.probe = ravb_gen2_ptp_probe,
> +	.clock_index = ravb_gen2_ptp_clock_index,
>  	.set_config_mode = ravb_gen3_ptp_set_config_mode,
>  	.ndev_open = ravb_ptp_init,
>  	.ndev_close = ravb_ptp_stop,

[ ... ]

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