Re: [PATCH net-next v3 07/10] net: ethernet: ravb: Add callback for gPTP probe

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

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

- [Critical] ravb_get_ts_info()
  (drivers/net/ethernet/renesas/ravb_main.c:1771-1790) gates on…
- [High] ravb_ptp_stop()
  (drivers/net/ethernet/renesas/ravb_ptp.c:368-377) unregisters…
- [High] For the ccc_gac-style generations the PTP clock is registered
  from ravb_open() via info->ptp->ndev_open (ravb_main.c:1953-1956,…
- [Medium] The newly propagated ptp_clock_register() error creates
  ravb_open() failure paths that leak all DMA ring resources.

--

net: ethernet: ravb: Add callback for gPTP probe

This adds a probe callback to struct ravb_gptp_info so each RAVB
generation can acquire its own gPTP timer clock, removes the last gPTP
specific flag (gptp_ref_clk), and moves the GTI computation into the new
per-generation probe callbacks.

A few questions below about the gPTP clock lifecycle that the ptp callback
structs in this patch select.

[ ... ]

[Severity: High]
With dmac_stop = ravb_ptp_stop wired up here, can priv->ptp.clock end up
pointing at a freed object after the interface goes down?

ravb_ptp_stop() in ravb_ptp.c unregisters the clock but never clears the
pointer:

	if (priv->ptp.clock)
		ptp_clock_unregister(priv->ptp.clock);

The baseline cleared it before unregistering:

	WRITE_ONCE(priv->ptp.phc_index, -1);
	clock = xchg(&priv->ptp.clock, NULL);

After ip link set <if> down on Gen2/RZ-V2M, ravb_close() -> ravb_stop_dma()
-> dmac_stop = ravb_ptp_stop() leaves the stale pointer, and
ravb_get_ts_info() then reads the freed memory:

	info->phc_index = ptp_clock_index(priv->ptp.clock);

drivers/ptp/ptp_clock.c:ptp_clock_index() is just:

	return ptp->index;

ETHTOOL_GET_TS_INFO is in the list in dev_ethtool() that skips the
ns_capable(CAP_NET_ADMIN) check, so this read is reachable by any local
user once the interface is down.

Can the same stale pointer also be unregistered twice? In
ravb_tx_timeout_work(), ravb_stop_dma() runs ravb_ptp_stop() (unregister),
and if the following ravb_dmac_init() fails (ravb_ring_init() returning
-ENOMEM, or the OPERATION opmode wait timing out) the function returns via
out_unlock without re-registering, so the later ravb_close() ->
ravb_stop_dma() -> ravb_ptp_stop() calls ptp_clock_unregister() on the
already freed clock.

This was introduced earlier in the series by "net: ethernet: ravb:
Propagate error from ptp_clock_register()" and ravb_ptp_stop() is still
unchanged at the end of the series (941e688), where
ravb_gen2_ptp_clock_index() likewise still does
return ptp_clock_index(priv->ptp.clock).

[Severity: High]
Since ravb_gen3_ptp_info registers the clock from ndev_open and only tears
it down from ndev_close, is the clock leaked when ravb_open() fails after
that registration?

ravb_open() calls info->ptp->ndev_open() and then ravb_phy_start(), whose
failure path is:

out_ptp_stop:
	ravb_stop_dma(ndev);

ravb_stop_dma() only stops PTP through info->ptp->dmac_stop, and
ravb_gen3_ptp_info has no dmac_stop, so nothing unregisters the clock.
ravb_gen4_hw_info uses ravb_gen3_ptp_info at this commit as well.

ravb_phy_start() can fail concretely: ravb_phy_init() returns -ENOENT when
of_phy_connect() fails, and it also propagates
of_phy_register_fixed_link() errors. As ndo_open returned an error the
netdev never goes IFF_UP, so ndo_stop and therefore ndev_close =
ravb_ptp_stop() are never called.

Consequences of that: /dev/ptpN stays registered; a retried open
overwrites priv->ptp.clock in ravb_ptp_init(), losing the old registration
for good; and a later unbind runs ravb_remove() -> free_netdev(ndev)
while the PTP core still has ptp->info pointing into the freed netdev
private area.

The baseline out_ptp_stop label did:

	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);

That was dropped by the series commit "net: ethernet: ravb: Simplify gPTP
start and stop", which added the stop only inside ravb_stop_dma(), so the
"no functional change" claim there does not seem to hold for the ccc_gac
(Gen3/Gen4) case. The gap is still present at 941e688, where out_ptp_stop
only calls ravb_stop_dma() and ravb_gen3_ptp_info still has no dmac_stop.

[Severity: Medium]
With dmac_start = ravb_ptp_init, does a failing ptp_clock_register() leak
the DMA rings?

ravb_dmac_init() first runs info->dmac_init = ravb_dmac_init_rcar(), which
allocates both queues in ravb_ring_init():

	priv->rx_pool[q] = page_pool_create(&params);
	...
	priv->tx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
					      &priv->tx_desc_dma[q], GFP_KERNEL);

and only afterwards does:

	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->dmac_start)
		return info->ptp->dmac_start(ndev);

If ptp_clock_register() in ravb_ptp_init() fails (for example -ENOMEM under
memory pressure), that error reaches ravb_open(), which jumps to
out_set_reset. Neither out_set_reset nor out_ptp_stop calls
ravb_ring_free(), and since ndo_open returned an error, ravb_close() (the
only ring freeing path) never runs; ravb_remove() does not free them
either.

A later open re-enters ravb_ring_init() and overwrites priv->rx_pool[q],
priv->rx_buffers[q], priv->tx_skb[q], priv->rx_ring[q] and
priv->tx_ring[q], so the previous page pools with their DMA mappings and
the coherent descriptor memory are lost.

For Gen3/Gen4 the equivalent ndev_open failure goes to out_ptp_stop, which
also does not call ravb_ring_free(). The same leak shape already existed
for a failing ravb_set_opmode(CCC_OPC_OPERATION); the new trigger comes
from the series commit "net: ethernet: ravb: Propagate error from
ptp_clock_register()", and the ravb_open() error labels are unchanged at
941e688.

[Severity: Critical]
hw_info->ptp is what ravb_get_ts_info() tests, and it only says the IP
supports gPTP. Can that path dereference a NULL priv->ptp.clock?

ravb_get_ts_info() does:

	if (hw_info->ptp) {
		...
		info->phc_index = ptp_clock_index(priv->ptp.clock);
	}

priv->ptp.clock is the zero initialised NULL from alloc_etherdev_mqs()
until the clock is registered, which only happens in the open lifecycle
(Gen2/RZ-V2M via ravb_dmac_init() -> dmac_start = ravb_ptp_init(),
Gen3/Gen4 via ravb_open() -> ndev_open = ravb_ptp_init()), and
ravb_ptp_init() explicitly stores NULL on registration failure:

	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
	if (IS_ERR(priv->ptp.clock)) {
		ret = PTR_ERR(priv->ptp.clock);
		priv->ptp.clock = NULL;

With CONFIG_PTP_1588_CLOCK=y, ptp_clock_index() is
return ptp->index;, so ethtool -T on an interface that has never been
opened (the state right after probe and register_netdev) oopses.
ETHTOOL_GET_TS_INFO does not require CAP_NET_ADMIN in dev_ethtool(), so an
unprivileged user can reach it; only CONFIG_PTP_1588_CLOCK=n is safe
because of the inline stub returning -1.

The baseline kept a separate priv->ptp.phc_index initialised to -1 in
ravb_probe() and read it with READ_ONCE(); the series commit "net:
ethernet: ravb: Propagate error from ptp_clock_register()" removed that
field and its -1 initialisation. At 941e688 the callback
ravb_gen2_ptp_clock_index() still does
return ptp_clock_index(priv->ptp.clock) and is installed for gen2, gen3
and rzv2m.

 	.nc_queues = 1,
 	.magic_pkt = 1,
 };

[ ... ]

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