Re: [PATCH net v5 1/2] net: fec: Propagate PTP initialization errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free and crash during probe error paths due to devm_request_irq() lifecycle mismatch with manual free_netdev() and clk_disable_unprepare().
- [High] Missing synchronization in fec_ptp_stop causes a race condition leading to a use-after-free of the ptp_clock structure in the PPS interrupt handler.
--

commit 4b37ab3f35f3d2df1a812ad2d83b4453fa8b03f2
Author: bui duc phuc <phucduc.bui@gmail.com>

net: fec: Propagate PTP initialization errors

This patch changes fec_ptp_init() to return an error code instead of silently
ignoring failures during PTP initialization. It ensures that unavailable
optional IRQs are handled correctly, and propagates errors from
devm_request_irq() and ptp_clock_register().

Link v1: https://lore.kernel.org/all/20260820111416.97917-1-phucduc.bui@gmail.com/
Link v2: https://lore.kernel.org/all/20260903092430.354186-1-phucduc.bui@gmail.com/
Link v3: https://lore.kernel.org/all/20260904105542.70540-1-phucduc.bui@gmail.com/
Link v4: https://lore.kernel.org/all/20260907034502.12619-1-phucduc.bui@gmail.com/

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..c6e29b5c2abb4 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5384,8 +5384,11 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_reset;
>  
>  	irq_cnt = fec_enet_get_irq_cnt(pdev);
> -	if (fep->bufdesc_ex)
> -		fec_ptp_init(pdev, irq_cnt);
> +	if (fep->bufdesc_ex) {
> +		ret = fec_ptp_init(pdev, irq_cnt);
> +		if (ret)
> +			goto failed_reset;
> +	}
>  
>  	ret = fec_enet_init(ndev);

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..557a76797ebaa 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c

[ ... ]

> @@ -779,26 +779,32 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)

[ ... ]

> +	if (irq > 0) {
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>  				       0, pdev->name, ndev);
>  		if (ret < 0)
> -			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
> -				 ret);
> +			return ret;
>  	}
>  
>  	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>  	if (IS_ERR(fep->ptp_clock)) {
> +		ret = PTR_ERR(fep->ptp_clock);
>  		fep->ptp_clock = NULL;
>  		dev_err(&pdev->dev, "ptp_clock_register failed\n");
> +		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but could this new error path trigger a
use-after-free or a crash if an interrupt fires during probe teardown?

If ptp_clock_register() fails, we now return an error after the
PPS interrupt has already been requested via devm_request_irq().

Back in fec_probe(), this error causes a jump to the teardown path:

    ret = fec_ptp_init(pdev, irq_cnt);
    if (ret)
        goto failed_reset;

The teardown logic manually disables clocks and eventually frees the netdev:

failed_clk_ahb:
    clk_disable_unprepare(fep->clk_ipg);
...
    free_netdev(ndev);
    return ret;

Since the interrupt is managed by devm, it remains active until the probe
function fully returns. If the interrupt fires during this window, the
fec_pps_interrupt() handler can dereference the freed ndev structure and
read unclocked hardware registers.

Is it possible to explicitly release or disable the devm-managed interrupt
before returning the error here?

[Severity: High]
This is also a pre-existing issue, but does the teardown logic in 
fec_ptp_stop() contain a race condition with the PPS interrupt?

When cleaning up, fec_ptp_stop() unregisters the clock without
synchronizing against the active PPS interrupt:

    if (fep->pps_enable)
        fec_ptp_enable_pps(fep, 0);
    ...
    if (fep->ptp_clock)
        ptp_clock_unregister(fep->ptp_clock);

Because the IRQ is devm-managed and not explicitly disabled,
fec_pps_interrupt() could execute concurrently. It reads fep->pps_enable
without a lock or memory barrier, meaning it might see a stale value and
dereference fep->ptp_clock after it has been freed.

Would it be safer to disable the interrupt or call synchronize_irq()
before unregistering the clock?

>  
>  	schedule_delayed_work(&fep->time_keep, HZ);
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909064712.22539-1-phucduc.bui@gmail.com?part=1