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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dangling pointer in `priv->ptp.clock` leads to double free and use-after-free on error recovery paths.
- [High] Early shared IRQ registration in `ravb_probe` leads to Use-After-Free and NULL pointer dereferences.
--

commit 252e91f926cfc67c49e9dd06194245579401d492
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Add callback for gPTP probe

This commit introduces a gPTP probe callback to accommodate different
generations of the RAVB IP and removes the gptp_ref_clk flag. It also
moves GTI computation into the callbacks to prepare for Gen4 support
where the clock is external.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[ ... ]
> @@ -2976,12 +2995,10 @@ static int ravb_probe(struct platform_device *pdev)
>  		goto out_reset_assert;
>  	}
>  
> -	if (info->gptp_ref_clk) {
> -		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
> -		if (IS_ERR(priv->gptp_clk)) {
> -			error = PTR_ERR(priv->gptp_clk);
> +	if (info->ptp && info->ptp->probe) {
> +		error = info->ptp->probe(ndev);
> +		if (error)
>  			goto out_reset_assert;

[Severity: High]
This is a pre-existing issue, but does adding this new failure point expand
a hazard window with the shared interrupt handler?

In ravb_probe(), devm_request_irq() is called to register a shared IRQ
before priv->addr is mapped. If a premature IRQ is delivered, could it force
a panic by accessing an unmapped register?

Additionally, if info->ptp->probe() fails here, the error path jumps to
out_reset_assert and eventually out_free_netdev:

out_free_netdev:
    free_netdev(ndev);
    return error;

Could this leave a live devm-managed IRQ handler targeting freed memory if
the IRQ fires before the devm cleanup removes the handler?


[Severity: Critical]
This is a pre-existing issue, but is there a risk of a double free in the
error recovery paths related to the ptp clock?

When ravb_tx_timeout_work() initiates a reset, it can fail to re-initialize
the DMA. If it takes the error path without neutralizing the clock pointer:

ravb_tx_timeout_work() {
    ...
    error = ravb_dmac_init(ndev);
    if (error) {
        ...
        goto out_unlock;
    }
}

The error path leaves the device up, and a secondary teardown in
ravb_ptp_stop() (such as during ravb_close) will attempt to unregister the
clock again:

ravb_ptp_stop() {
    ...
    if (priv->ptp.clock)
        ptp_clock_unregister(priv->ptp.clock);
}

Since the pointer is freed during the initial timeout recovery but not
set to NULL, could this result in a use-after-free and double free when
ravb_close() is subsequently executed?

> -		}
>  	}
>  
>  	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");

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