Re: [PATCH net-next v4 03/10] net: ethernet: ravb: Simplify gPTP start and stop

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] For gptp devices, early returns in ravb_stop_dma() skip stopping the PTP clock, leading to Use-After-Free in ravb_close().
- [High] Missing PTP clock cleanup on ravb_open() error path for ccc_gac devices.
- [Medium] PTP clock remains permanently stopped if ravb_stop_dma() fails late during TX timeout recovery.

Pre-existing issues:
- [High] Missing NAPI synchronization before freeing DMA rings can lead to concurrent access.
--

commit cfbb6c5f1a7c830cb10526b29bb3646d146532ea
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Subject: net: ethernet: ravb: Simplify gPTP start and stop

The patch simplifies the start and stop of gPTP clocks for Gen2 (gptp-flag)
and Gen3 (ccc_gac) devices. It ties Gen2's PTP clock to the AVB-DMAC start
and stop, while Gen3's PTP clock remains controlled on interface open/close.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a89f6fe7866fe..2d0022a4f85c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c

[ ... ]

> @@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
>  			netdev_err(ndev, "failed to stop AXI BUS\n");
>  	}
>  
> +	/* Stop PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_stop(ndev);
> +

[Severity: Critical]
Could moving the PTP clock stop to the end of ravb_stop_dma() introduce a use-after-free
if there is a hardware timeout?

ravb_stop_dma() contains multiple early returns if ravb_wait() times out:

static int ravb_stop_dma(struct net_device *ndev)
{
	...
	/* Wait for stopping the hardware TX process */
	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;
	...

If one of these early returns is taken, the new ravb_ptp_stop() call is bypassed.

Later, during device teardown, ravb_close() ignores errors from ravb_stop_dma():

	/* 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");
	/* Clear the timestamp list */

Since ravb_close() continues and frees the device structures, what happens if the PTP clock
was bypassed and remains registered? It appears subsequent ioctls to /dev/ptpX
could dereference the freed pointer.

>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
>  }

[ ... ]

> @@ -1719,9 +1731,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
>  
>  	if (netif_running(ndev)) {
>  		netif_device_detach(ndev);
> -		/* Stop PTP Clock driver */
> -		if (info->gptp)
> -			ravb_ptp_stop(ndev);
> +
>  		/* Wait for DMA stopping */
>  		error = ravb_stop_dma(ndev);

[Severity: High]
This isn't a bug introduced by this patch, but does freeing the rings here race
with NAPI polling?

Just after this block in ravb_set_ringparam(), the code frees the DMA buffers
without disabling NAPI:

		synchronize_irq(ndev->irq);

		/* Free all the skb's in the RX queue and the DMA buffers. */
		ravb_ring_free(ndev, RAVB_BE);

Since synchronize_irq() only waits for hard IRQ handlers to finish, a NAPI poll
could execute concurrently on another CPU while the rings are being freed.
Would it be better to call napi_disable() before freeing the resources?

[ ... ]

> @@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
>  	return 0;
>  
>  out_ptp_stop:
> -	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> -		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);
>  out_set_reset:

[Severity: High]
Is there a resource leak of the PTP clock for ccc_gac devices when removing this
explicit cleanup?

If ravb_open() fails during ravb_phy_start() on a device with the ccc_gac flag,
it jumps to the out_ptp_stop label. With this patch, the cleanup relies entirely
on ravb_stop_dma().

However, ravb_stop_dma() only stops the PTP clock if info->gptp is true. Since
info->gptp and info->ccc_gac are mutually exclusive, it seems the PTP clock will
remain registered when initialization fails for ccc_gac devices.

[ ... ]

> @@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	ravb_emac_init(ndev);
>  
>  out:
> -	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev, priv->pdev);
> -

[Severity: Medium]
Will the PTP clock become permanently disabled here if ravb_stop_dma() fails
late during timeout recovery?

If ravb_stop_dma() fails during its final ravb_set_opmode() call, it returns
an error after it has already stopped the PTP clock.

In ravb_tx_timeout_work(), this failure causes a jump directly to the out label:

	if (ravb_stop_dma(ndev)) {
		ravb_rcv_snd_enable(ndev);
		goto out;
	}

Because ravb_ptp_init() has been removed from the out label, the PTP clock
will remain stopped even though the device resumes passing traffic.

>  	netif_tx_start_all_queues(ndev);
>  
>  out_unlock:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909173027.2201560-1-niklas.soderlund+renesas@ragnatech.se?part=3